Updating aws sdk to v3 #34
Conversation
| @@ -1,4 +1,4 @@ | |||
| var s3urls = require('@mapbox/s3urls'); | |||
| var s3urls = require('./lib/s3url-parser'); | |||
There was a problem hiding this comment.
This is a deprecated library so I just implemented the necessary functions in s3url-parser
lib/delete.js
Outdated
|
|
||
| function removed(err) { | ||
| if (err && err.statusCode !== 404) return callback(awsError(err, params)); | ||
| if (err && err.name !== 'NoSuchKey') return callback(awsError(err, params)); |
There was a problem hiding this comment.
I feel like 404 would capture more than just NoSuchKey, can we add NoSuchBucket too?
lib/delete.js
Outdated
|
|
||
| function removed(err) { | ||
| if (err && err.name !== 'NoSuchKey') return callback(awsError(err, params)); | ||
| if (err && err.name !== 'NoSuchKey' && err.name !== 'NoSuchBucket' && err.statusCode !== 404) return callback(awsError(err, params)); |
There was a problem hiding this comment.
And maybe also check if its an S3ServiceError exception?
lib/delete.js
Outdated
| const s3config = { | ||
| maxRetries: 10, | ||
| httpOptions: { connectTimeout: 3000 } | ||
| maxAttempts: 10, |
There was a problem hiding this comment.
would we want to make this 11 to account for the base attempt? (the maxRetries is 10 above)
|
|
||
| const s3config = { | ||
| maxRetries: 10, | ||
| httpOptions: { connectTimeout: 3000 } |
There was a problem hiding this comment.
connectTimeout and requestTimeout would not be the same thing. Please refer to here for example https://github.com/mapbox/aws-logs/blob/main/cdk/lib/forwarders/clients/s3.ts#L39-L43
| requestTimeout: 3000, | ||
| requestHandler: new NodeHttpHandler({ | ||
| httpAgent: new Agent({}), | ||
| connectionTimeout: 20 * 1000, |
There was a problem hiding this comment.
should we keep this 3000 like before?
2 participants
What Changed?
Testing
I have tested and confirmed this works this with each of the bin commands:
./bin/s3scan.js s3://<redacted>/./bin/s3keys.js s3://redacted/./bin/s3purge.js s3://redacted/Since this is a public repo, I have removed the s3 paths and information