-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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): make rds secret name configurable #13626
Conversation
Title does not follow the guidelines of Conventional Commits. Please adjust title before merge. |
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.
Looks great @hedrall ! Despite being a small change, this is actually a really subtle PR. I've gone through it, and I think I understood everything finally (it definitely took me a while!).
I left some questions - can you confirm my understanding of the code is the same as yours?
Thanks,
Adam
* For "owned" secrets, this will be the full resource name (secret name + suffix), unless the | ||
* '@aws-cdk/aws-secetsmanager:parseOwnedSecretName' feature flag is set. |
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 have to say, I find this documentation super confusing 🤨.
This is an input property, right? So either you provide the name of the Secret, or it will be generated by CloudFormation.
What does this paragraph mean?
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'm sorry. I did not consider this point very deeply, and used the comments for the properties of Secret itself.
I would like to look into this more, including how it relates to the Feature flag.
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.
It's better to steal the documentation from SecretProps
, rather than from ISecret
.
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.
Thank you.
A name for the secret. Note that deleting secrets from SecretsManager does not happen immediately, but after a 7 to * 30 days blackout period. During that period, it is not possible to create another secret that shares the same name.
However, I just noticed that this seems to be an old specification.
I tried destroying the deployed secret and renaming the deployed secret, but in both cases the secret was deleted immediately.
I think that cloud formation
use a flag like as cli 'force-delete-without-recovery’.
So, I change comment simply.
( Should I also change the comment of SecretProps? )
@@ -221,6 +228,7 @@ export abstract class Credentials { | |||
return { | |||
username: username ?? secret.secretValueFromJson('username').toString(), | |||
password: secret.secretValueFromJson('password'), | |||
secretName: secret.secretName, |
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'm 99% sure this line is not actually needed.
@hedrall do you agree?
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.
Maybe Yes. I thought it may need to initialize and expose as a field like ‘encryptionKey’ below, so I wrote this line.
I not sure of any use case that uses the secretName of the object of Credentials created by fromSecret, so I will delete it now.
BTW, the build is failing because of
|
Co-authored-by: Adam Ruka <adamruka85@gmail.com>
Co-authored-by: Adam Ruka <adamruka85@gmail.com>
Co-authored-by: Adam Ruka <adamruka85@gmail.com>
Co-authored-by: Adam Ruka <adamruka85@gmail.com>
Co-authored-by: Adam Ruka <adamruka85@gmail.com>
Co-authored-by: Jonathan Goldwasser <jogold@users.noreply.github.com>
Thank you for all your suggestions. I’d fixed. |
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.
Looks great @hedrall, thanks so much for the contribution!
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
close #8984
As suggested by @njlynch in the first issue, I added the ability to set the secret name that RDS generates.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license