-
Notifications
You must be signed in to change notification settings - Fork 2
Migration aws sdk v2 to v3 : removeDeleteMarkers #350
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
base: development/1.16
Are you sure you want to change the base?
Conversation
Hello sylvainsenechal,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
VerifyReplication/storage/s3.js
Outdated
}; | ||
return cb(null, resp); | ||
}); | ||
} catch (err) { |
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.
catch must not wrap the callback: otherwise, if the callback throws, we will call it again....
best way is to stick to a single approach:
- if the function being implemented (i.e.
getObjMd
) is a callback API, probably best to use the promise directly :client.send().then(...)
- if the function is async/promise-based (or updated this), then we can and should indeed use await
and/or we can make a trampoline to ease transition (in the same or different functions):
async function getObjMd(params, cb) {
if (cb) {
return getObjMd(params).then(result -> cb(null, result), cb);
}
// no need for catch actually, since that is the way to return the error!
return await client.send(...);
}
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.
note: we need to make this PR correct, but to avoid too much changes we can also choose to be less ambitious (not moving to "proper" async yet, just use callbacks and promises), and upgrade to async later (in separate PR)
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 see, I reverted it
VerifyReplication/storage/s3.js
Outdated
} | ||
|
||
function listObjects(params, cb) { | ||
async function listObjects(params, cb) { |
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 here: catch must not wrap the callback: otherwise, if the callback throws, we will call it again....
VerifyReplication/storage/s3.js
Outdated
Delimiter: delimiter, | ||
ContinuationToken: nextContinuationToken, | ||
})); | ||
return cb(null, data); |
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 here: catch must not wrap the callback: otherwise, if the callback throws, we will call it again....
ed7bd36
to
ef6f505
Compare
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
|
ef6f505
to
cb852ce
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.
Not fully related to this pr whose goal is to migrate removeDeleteMarkers, but I made a mistake in one of the previous ones, mixing callback and catch, so I'm fixing it here
c284177
to
0ffbe21
Compare
Issue : S3UTILS-204