feat: add aws s3 provider by socaseinpoint · Pull Request #227 · makeomatic/ms-files

socaseinpoint · 2021-05-26T16:03:52Z

No description provided.

AVVS · 2021-05-26T16:05:57Z

src/providers/aws.js

+    } catch (e) {
+      this._logger.warn('failed to load aws-sdk/clients/s3', e);
+    }


Suggested change

} catch (e) {

this._logger.warn('failed to load aws-sdk/clients/s3', e);

}

} catch (err) {

this._logger.warn({ err }, 'failed to load aws-sdk/clients/s3');

}

otherwise error will not get serialized - this is an optimization in the logger

AVVS · 2021-05-26T16:06:41Z

src/providers/aws.js

+  connect() {
+    return Promise
+      .bind(this)
+      .then(this.findOrCreateBucket);
+  }


we can use native promises now:

Suggested change

connect() {

return Promise

.bind(this)

.then(this.findOrCreateBucket);

}

async connect() {

return this.findOrCreateBucket();

}

AVVS · 2021-05-26T16:08:08Z

src/providers/aws.js

+    const params = {
+      Bucket: this._config.bucketName,
+      Expires: action === 'putObject' ? UPLOAD_URL_EXPIRES_IN_SEC : DOWNLOAD_URL_EXPIRES_IN_SEC,
+      Key: input.key,
+    };


not familiar with s3 signed URLs, but on google you have to sign headers as well, especially for put action.
headers include object size, any custom headers, etc - could this be required here?

AVVS · 2021-05-26T16:08:51Z

src/providers/aws.js

+  }
+
+  connect() {
+    return Promise


Promise.bind is specific to bluebird (http://bluebirdjs.com/docs/getting-started.html)

AVVS · 2021-05-26T16:09:20Z

src/providers/factory.js

    return provider;
  }
+
+  // eslint-disable-next-line class-methods-use-this


can use static modifier since we are not using this

AVVS · 2021-05-26T16:10:08Z

test/suites/providers/aws.js

+        accessKeyId: 'AKIASS5V3WV23VA4KYOF',
+        accessKeySecret: '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',


these keys must be revoked now as they were commited to open source code - they must be passed on via .env and never be commited

codecov · 2021-05-26T16:16:44Z

Codecov Report

Merging #227 (348f597) into master (a6f2e0d) will decrease coverage by 0.28%.
The diff coverage is 20.00%.

❗ Current head 348f597 differs from pull request most recent head 2414957. Consider uploading reports for the commit 2414957 to get more accurate results

@@            Coverage Diff             @@
##           master     #227      +/-   ##
==========================================
- Coverage   92.78%   92.50%   -0.29%     
==========================================
  Files          49       49              
  Lines        1289     1294       +5     
==========================================
+ Hits         1196     1197       +1     
- Misses         93       97       +4

Impacted Files	Coverage Δ
src/providers/factory.js	`73.07% <0.00%> (-9.54%)`	⬇️
src/providers/index.js	`96.00% <50.00%> (-4.00%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3966e90...2414957. Read the comment docs.

AVVS · 2021-08-26T17:46:40Z

src/providers/aws.js

+  async subscribe() {
+    const { Topics: topics } = await new SNS({ region: this._config.aws.credentials.region }).listTopics({}).promise();
+
+    const topicArn = `arn:aws:sns:us-west-2:178085672309:${this._config.aws.credentials.topicName}`;


move to configuration

AVVS · 2021-08-26T17:47:13Z

src/providers/aws.js

+
+    async function handleListBuckets(err, data) {
+      if (err) {
+        this.log.warn('s3 bucket can not be created');


Suggested change

this.log.warn('s3 bucket can not be created');

this.log.warn({ err }, 's3 bucket can not be created');

AVVS · 2021-08-26T17:47:46Z

src/providers/aws.js

+          await aws.createBucket(bucketParams, function handleCreateBucket(_err) {
+            if (_err) {
+              this.log.warn('s3 bucket can not be created');
+            }
+          });


promise & callback?

AVVS · 2021-08-26T17:48:04Z

src/providers/aws.js

+    };
+
+    return new Promise((resolve, reject) => {
+      this._aws.getSignedUrl('putObject', params, (err, url) => {


AVVS · 2021-08-26T17:49:03Z

test/configs/generic/core.js

+//   },
+// ];
+
+exports.transport = env.PROVIDER === 'aws' ? awsTransport : awsTransport;


aws or gcs?

AVVS · 2021-08-26T17:50:12Z

test/suites/providers/aws/access.js

+      return this.files
+        .postProcess(0, Date.now())
+        .reflect()
+        .then(inspectPromise());


this is a legacy implementstion, its best to use the following now:

Suggested change

return this.files

.postProcess(0, Date.now())

.reflect()

.then(inspectPromise());

await this.files.postProcess(0, Date.now())

AVVS · 2021-08-26T17:50:51Z

test/suites/providers/aws/access.js

+    it('rejects to show direct only file without proper username', function test() {
+      return downloadFile
+        .call(this, { uploadId: this.response.uploadId })
+        .reflect()
+        .then(inspectPromise(false));
+    });


for these things its best to do this:

Suggested change

it('rejects to show direct only file without proper username', function test() {

return downloadFile

.call(this, { uploadId: this.response.uploadId })

.reflect()

.then(inspectPromise(false));

});

it('rejects to show direct only file without proper username', async function test() {

await assert.rejects(downloadFile.call(this, { uploadId: this.response.uploadId }));

});

AVVS reviewed May 26, 2021

View reviewed changes

socaseinpoint force-pushed the feat/aws-provider branch from 773348a to 348f597 Compare May 26, 2021 16:11

socaseinpoint added 3 commits June 29, 2021 20:51

chore: add sdk dependency

9fa79f6

feat: add aws provider

02107de

feat: add aws provider implementation

99a1a80

socaseinpoint force-pushed the feat/aws-provider branch from 348f597 to 99a1a80 Compare June 29, 2021 14:56

feat: add aws provider implementation

99c7a1c

socaseinpoint force-pushed the feat/aws-provider branch from 09e6621 to 77cc20c Compare July 20, 2021 14:58

feat: add aws test suites

2414957

socaseinpoint force-pushed the feat/aws-provider branch from 77cc20c to 2414957 Compare August 24, 2021 14:45

AVVS reviewed Aug 26, 2021

View reviewed changes

src/providers/aws.js

};

return new Promise((resolve, reject) => {

this._aws.getSignedUrl('putObject', params, (err, url) => {

Copy link

Member

AVVS Aug 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

promisify

AVVS reviewed Aug 26, 2021

View reviewed changes

test/configs/generic/core.js

// },

// ];

exports.transport = env.PROVIDER === 'aws' ? awsTransport : awsTransport;

Copy link

Member

AVVS Aug 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

aws or gcs?

AVVS reviewed Aug 26, 2021

View reviewed changes

		accessKeyId: 'AKIASS5V3WV23VA4KYOF',
		accessKeySecret: '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',

	this.log.warn('s3 bucket can not be created');
	this.log.warn({ err }, 's3 bucket can not be created');

Conversation

socaseinpoint commented May 26, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented May 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov bot commented May 26, 2021 •

edited

Loading