-
Notifications
You must be signed in to change notification settings - Fork 3
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
Improvement/vltclt 37 remove arsenal dep #431
Improvement/vltclt 37 remove arsenal dep #431
Conversation
Hello williamlardier,My role is to assist you with the merge of this Status report is not available. |
f112cf1
to
95d8bc1
Compare
package.json
Outdated
@@ -14,13 +14,15 @@ | |||
}, | |||
"homepage": "https://github.com/scality/vaultclient#readme", | |||
"dependencies": { | |||
"arsenal": "git+https://github.com/scality/Arsenal#8.1.91", | |||
"@aws-crypto/sha256-universal": "^2.0.1", | |||
"@aws-sdk/signature-v4": "^3.130.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.
NPM is indicating that this package has been moved to @smithy/signature-v4
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.
Done here: 9fe22ac
lib/IAMClient.js
Outdated
@@ -1014,7 +1021,7 @@ class VaultClient { | |||
* @param {string} [contentType] - content type of body | |||
* @returns {undefined} | |||
*/ | |||
request(method, path, iamAuthenticate, callback, data, reqUid, | |||
async request(method, path, iamAuthenticate, callback, data, reqUid, |
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.
Adding the async
tag changes the return to a Promise, which in the case of signer.sign
throwing would result in a UnhandlePromiseRejection
and crash the process.
I'd suggest adding a _signRequest
helper that is async and then using then
and catch
in request.
Something like:
async _signRequest(iamAuthenticate, req, options)
That could then be used in request like
this._signRequest(iamAuthenticate, req, options)
.catch(callback)
.then(() => {
...
});
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 helper was created, and the logic structure was reworked a bit to keep a flat function:
1ce5626
91fe508
to
b522e29
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.
lgtm
b522e29
to
cdc9133
Compare
cdc9133
to
06083cb
Compare
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
|
06083cb
to
676a2a6
Compare
/approve |
In the queueThe changeset has received all authorizations and has been added to the The changeset will be merged in:
The following branches will NOT be impacted:
There is no action required on your side. You will be notified here once IMPORTANT Please do not attempt to modify this pull request.
If you need this pull request to be removed from the queue, please contact a The following options are set: 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 VLTCLT-37. Goodbye williamlardier. |
Remove arsenal from the prod dependencies, as it is only used to check errors (internal error, mostly), that can be stored as a const.
The main advantage being the decrease of the build duration, arsenal requires to build multiple modules, taking up to 5~10min every time in some CIs, depending on the load.