-
Notifications
You must be signed in to change notification settings - Fork 578
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
feat(rds-signer): add RDS Signer (#1823) #3084
Conversation
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 very solid start! The interface is mostly complatible with the v2 SDK, which is good. I only have a few suggestions/questions.
lib/lib-rds/package.json
Outdated
"license": "Apache-2.0", | ||
"dependencies": { | ||
"@aws-sdk/signature-v4": "3.40.0", | ||
"@aws-sdk/protocol-http": "3.40.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.
Some dependencies gone missing here: like @aws-sdk/credential-provider-node
, @aws-crypto/sha256-js
. This is causing the CI build to fail.
@jgoeglein Can you move this package to the |
Codecov Report
@@ Coverage Diff @@
## main #3084 +/- ##
===========================================
- Coverage 75.19% 58.72% -16.47%
===========================================
Files 474 571 +97
Lines 20721 30622 +9901
Branches 4755 7541 +2786
===========================================
+ Hits 15581 17984 +2403
- Misses 5140 12638 +7498
Continue to review full report at Codecov.
|
922f88d
to
2e11419
Compare
We're now using this implementation in our project. It'd be great to see this finally merged and rolled into the main SDK, so that we can re-shuffle our explicit dependencies 🙏 |
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.
Sorry for waiting so long before I reviewing it again! The PR is in a good shape now. I only have 2 very minor comments. Do you plan to take them? or I can push some commits to this branch.
/cc @jgoeglein
@AllanZhengYP Added you as a collaborator so you can push those changes - it might take me a few days to get back around to this, but happy to do so if needed. |
Okay, I will take it up. For easier interface, I plan to make this signer populating the default credential and region automatically. I post a separate PR #3588 to add an easier interface of Node.js default credential provider. I will update thie PR after that one merged. |
2e11419
to
867d97b
Compare
90a54ff
to
e2ed026
Compare
According to the latest offline discussion, we decided to standardize all the naming scheme to the signers packages like For this package, we decided to keep the name as |
Really looking forward to this addition – Not a fan of importing the entire Thanks for all your hard work! |
This reverts commit f0820841c7d59ddc98a27731eaf923f1fca93b7d.
e2ed026
to
2ec3765
Compare
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs and link to relevant comments in this thread. |
Issue
#1823
Description
Adds RDS Signer for generating a password that can be used for IAM authentication. Previously, this functionality existed in v2.
Testing
Additional context
n/a
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.