-
Notifications
You must be signed in to change notification settings - Fork 2
Migration aws sdk v2 to v3 : cleanupBuckets #348
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Hello sylvainsenechal,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
5c10149
to
167833f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same comments as https://github.com/scality/s3utils/pull/349/files#r2333101487, looks so very similar.... Maybe we should create a ticket to clean this up and deduplicate the redundancy (maybe some sort of common processBucket
function, which calls it's parameter to "act" on matching object)
167833f
to
b06747d
Compare
cleanupBuckets.js
Outdated
const bodyContent = Buffer.from(args.request.body); | ||
const md5Hash = crypto.createHash('md5').update(bodyContent).digest('base64'); | ||
// eslint-disable-next-line no-param-reassign | ||
args.request.headers['Content-MD5'] = md5Hash; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"name":"BadDigest" error without this stuff
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this not the "known issue" about content-md5, on which @leif-scality worked and which @benzekrimaha further worked when bumping SDK in cloudserver?
do we have a better/simpler/... fix there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DeleteObjects, PutBucketCors and all the other /lib/api
routes don't require an MD5 checksum since b10a2d6eeb20118af0a8975ce471b8d438214641.
Note: some backbeat routes may require an MD5 sum today.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, doubled checked with Leif and Maha and tried on cloudserver local, no badDigest after I pulled the latest branch, I'll remove this
b06747d
to
e9dc1b2
Compare
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
|
e9dc1b2
to
da3f21f
Compare
da3f21f
to
0947a1b
Compare
CRR/ReplicationStatusUpdater.js
Outdated
)) | ||
.then(res => next(null, res.ReplicationConfiguration)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
)) | |
.then(res => next(null, res.ReplicationConfiguration)) | |
)).then(res => next(null, res.ReplicationConfiguration)) |
cleanupBuckets.js
Outdated
try { | ||
await Promise.all([ | ||
cleanupVersions(bucket), | ||
// abortAllMultipartUploads(bucket), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can be fully removed, or please add a comment to explain why it's commented
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad, it's needed, I commented for a test this afternoon and forgot to remove it..
cleanupBuckets.js
Outdated
const { http, https } = require('httpagent'); | ||
const async = require('async'); | ||
const AWS = require('aws-sdk'); | ||
const crypto = require('crypto'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The module doesn't seem to be used, we can remove it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really messed up this afternoon, removed something that became useless and forgot to cleanup some stuffs -_-
cleanupBuckets.js
Outdated
function _deleteVersions(bucket, objectsToDelete, cb) { | ||
// multi object delete can delete max 1000 objects | ||
async function _deleteVersions(bucket, objectsToDelete) { | ||
if (!objectsToDelete || objectsToDelete.length === 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bit redundant with the check already in cleanupVersions
? I think we should be able to merge them
0947a1b
to
eeb784a
Compare
/approve |
I have successfully merged the changeset of this pull request
The following branches have NOT changed:
Please check the status of the associated issue S3UTILS-203. Goodbye sylvainsenechal. The following options are set: approve |
ISSUE : S3UTILS-203