feat: add aws s3 provider #227

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

@@ -57,6 +57,14 @@ class ProviderFactory {

    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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add aws s3 provider #227

feat: add aws s3 provider #227

socaseinpoint commented May 26, 2021

AVVS May 26, 2021

socaseinpoint May 27, 2021

AVVS May 26, 2021

socaseinpoint May 27, 2021

AVVS May 26, 2021

AVVS May 26, 2021

socaseinpoint Jun 9, 2021

AVVS May 26, 2021

socaseinpoint Jun 9, 2021

AVVS May 26, 2021

socaseinpoint Jun 9, 2021

codecov bot commented May 26, 2021 •

edited

Loading

AVVS Aug 26, 2021

AVVS Aug 26, 2021

AVVS Aug 26, 2021

AVVS Aug 26, 2021

AVVS Aug 26, 2021

AVVS Aug 26, 2021

AVVS Aug 26, 2021

		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');

feat: add aws s3 provider #227

Are you sure you want to change the base?

feat: add aws s3 provider #227

Conversation

socaseinpoint commented May 26, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented May 26, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented May 26, 2021 •

edited

Loading