feat(rds): make rds secret name configurable #13626
Conversation
|
|
github-actions
bot
added
the
@aws-cdk/aws-rds
|
Title does not follow the guidelines of Conventional Commits. Please adjust title before merge. |
hedrall
changed the title
There was a problem hiding this comment.
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.
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.
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.
It's better to steal the documentation from SecretProps, rather than from ISecret.
There was a problem hiding this comment.
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.
I'm 99% sure this line is not actually needed.
@hedrall do you agree?
There was a problem hiding this comment.
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. |
|
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