-
Notifications
You must be signed in to change notification settings - Fork 9.6k
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
provider/aws: aws_db_option_group normalizes name to lowercase #14192
provider/aws: aws_db_option_group normalizes name to lowercase #14192
Conversation
just a quick documentation nit and then backs away slowly from go |
@feministy Unless I'm missing something, I think you forgot to add whatever comment you meant to there. :) What's the nit? |
@thrashr888 it's inline with the review comments .... that i didnt submit, thanks github |
@@ -38,10 +38,10 @@ resource "aws_db_option_group" "bar" { | |||
|
|||
The following arguments are supported: | |||
|
|||
* `name` - (Optional, Forces new resource) The name of the option group. If omitted, Terraform will assign a random, unique name. | |||
* `name_prefix` - (Optional, Forces new resource) Creates a unique name beginning with the specified prefix. Conflicts with `name`. | |||
* `name` - (Optional, Forces new resource) The name of the option group. If omitted, Terraform will assign a random, unique name. This is converted to lowercase. |
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 might note here why it's converted to lowercase so users aren't like "?????"
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 good to me!
Agreed with @feministy that we should clarify that this is done because RDS itself requires the value to be lowercase.
After that, assuming the acceptance tests are passing for this, merge away!
(Oh, and when you do merge, remember to add a note about this to |
This is a fix for PR #11040. The code here lowercases the name and/or prefix before sending it to the AWS API and the terraform state. This means the state will match the actual resource name and be able to converge the diff.
7e1f158
to
ccae937
Compare
Thanks guys! |
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
This fixes #11040. The code here lowercases the name and/or prefix before sending it to the AWS API and the terraform state. This means the state will match the actual resource name and be able to converge the diff.
Other providers might validate and throw an error or warning instead of just letting it work, but this is consistent with
aws_db_instance
. If this is the wrong approach, I can use a validator instead.I've also updated all the tests to include uppercase letters, which might be overkill.