S3 integration #45
S3 integration #45
Conversation
c2ca9ee to
af4edfe
Compare
|
We should keep the file system version so we can dev without aws credentials. Lets factor out the storage bits into its own module. I sketched something up as a start here: https://gist.github.com/dannycoates/f75e37df25968ff74475156b894669d4 |
…ion, but if there is either no aws bucket or bitly key specified as env vars, it defaults back to local storage
server/storage.js
Outdated
| }; | ||
| } | ||
|
|
||
| function LocalLength(id) { |
There was a problem hiding this comment.
Traditionally, in js this case style is used for constructors. Let's use camelCase for these function names
server/portal_server.js
Outdated
| const redis = require('redis'), | ||
| client = redis.createClient(); | ||
| const redis = require('redis'); | ||
| const redis_client = redis.createClient(); |
There was a problem hiding this comment.
can we move all this redis code into storage.js?
server/portal_server.js
Outdated
| conf.bitly_key !== 'localhost'; | ||
|
|
||
| const AWS = require('aws-sdk'); | ||
| const s3 = new AWS.S3(); |
There was a problem hiding this comment.
We can remove AWS and s3 from this file now
server/portal_server.js
Outdated
| const fetch = require('node-fetch'); | ||
| const storage = require('./storage.js'); | ||
|
|
||
| let isProduction = |
There was a problem hiding this comment.
It's confusing that isProduction can be false when NODE_ENV=production. Perhaps naming it something like useRemoteServices or localhostOnly.
There was a problem hiding this comment.
Nit: I don't think this variable is used in this file anymore.
But @dannycoates comment still applies to server/storage.js:10 below.
server/storage.js
Outdated
| return new Promise((resolve, reject) => { | ||
| client.hget(id, 'delete', (err, reply) => { | ||
| if (!reply || delete_token !== reply) { | ||
| resolve( |
There was a problem hiding this comment.
this block can just be reject()
| return new Promise((resolve, reject) => { | ||
| s3.upload(params, function(err, data) { | ||
| if (err) { | ||
| console.log(err, err.stack); // an error occurred |
There was a problem hiding this comment.
We should call reject() here
server/storage.js
Outdated
| ); | ||
| } else { | ||
| resolve( | ||
| new Promise((resolve, reject) => { |
There was a problem hiding this comment.
we shouldn't need this Promise, just resolve the unlink
|
This looks good to merge. I don't think we necessarily need index to be a template but NBD |
No description provided.