resource/aws_db_option_group: Fix error when creating db_option_group with new IAM role #5389
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pvanbuijtene
changed the title
bflad
added
bug
bflad
reviewed
Jul 30, 2018
There was a problem hiding this comment.
This is looking really good, @pvanbuijtene! Left two notes below. Let me know if you have any questions or do not have time to implement the feedback. 👍
aws/resource_aws_db_option_group.go
Outdated
| @@ -273,12 +273,24 @@ func resourceAwsDbOptionGroupUpdate(d *schema.ResourceData, meta interface{}) er | |||
| } | |||
|
|
|||
| log.Printf("[DEBUG] Modify DB Option Group: %s", modifyOpts) | |||
| _, err = rdsconn.ModifyOptionGroup(modifyOpts) | |||
|
|
|||
| err = resource.Retry(5*time.Minute, func() *resource.RetryError { | |||
There was a problem hiding this comment.
IAM eventual consistency is usually on the magnitude of 1-2 minutes (very rarely the latter). Can you please lower this retry timeout to prevent bad user experience when there might be an actual issue?
aws/resource_aws_db_option_group.go
Outdated
|
|
||
| _, err = rdsconn.ModifyOptionGroup(modifyOpts) | ||
| if err != nil { | ||
| if isAWSErr(err, "InvalidParameterValue", "SQLSERVER_BACKUP_RESTORE") { |
There was a problem hiding this comment.
Two things:
- Can you please add a comment with the full error message?
- Can you adjust the string checking for the relevant IAM part, instead of just the parameter in question? We don't want to retry on non-IAM messages for
SQLSERVER_BACKUP_RESTOREand we also would love to proactively catch IAM issues for other parameters if we can.
e.g.
// InvalidParameterValue: IAM role ARN value is invalid or does not include the required permissions for: SQLSERVER_BACKUP_RESTORE
if isAWSErr(err, "InvalidParameterValue", "IAM role ARN value is invalid or does not include the required permissions") {
ghost
added
size/L
|
@bflad thanks for the review, review comments are applied. |
bflad
approved these changes
Jul 31, 2018
There was a problem hiding this comment.
Looks great @pvanbuijtene, thanks!! 🚀
12 tests passed (all tests)
=== RUN TestAccAWSDBOptionGroup_defaultDescription
--- PASS: TestAccAWSDBOptionGroup_defaultDescription (7.09s)
=== RUN TestAccAWSDBOptionGroup_generatedName
--- PASS: TestAccAWSDBOptionGroup_generatedName (7.18s)
=== RUN TestAccAWSDBOptionGroup_namePrefix
--- PASS: TestAccAWSDBOptionGroup_namePrefix (7.29s)
=== RUN TestAccAWSDBOptionGroup_timeoutBlock
--- PASS: TestAccAWSDBOptionGroup_timeoutBlock (7.36s)
=== RUN TestAccAWSDBOptionGroup_multipleOptions
--- PASS: TestAccAWSDBOptionGroup_multipleOptions (7.43s)
=== RUN TestAccAWSDBOptionGroup_importBasic
--- PASS: TestAccAWSDBOptionGroup_importBasic (7.98s)
=== RUN TestAccAWSDBOptionGroup_OptionSettings
--- PASS: TestAccAWSDBOptionGroup_OptionSettings (12.11s)
=== RUN TestAccAWSDBOptionGroup_OracleOptionsUpdate
--- PASS: TestAccAWSDBOptionGroup_OracleOptionsUpdate (13.44s)
=== RUN TestAccAWSDBOptionGroup_basic
--- PASS: TestAccAWSDBOptionGroup_basic (14.34s)
=== RUN TestAccAWSDBOptionGroup_sqlServerOptionsUpdate
--- PASS: TestAccAWSDBOptionGroup_sqlServerOptionsUpdate (26.08s)
=== RUN TestAccAWSDBOptionGroup_OptionSettingsIAMRole
--- PASS: TestAccAWSDBOptionGroup_OptionSettingsIAMRole (27.43s)
=== RUN TestAccAWSDBOptionGroup_basicDestroyWithInstance
--- PASS: TestAccAWSDBOptionGroup_basicDestroyWithInstance (483.67s)
|
This has been released in version 1.30.0 of the AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. |
|
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 feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks! |
ghost
locked and limited conversation to collaborators
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #763
Added retry to fix eventual consistency error when referring to new IAM role.
Output from acceptance testing: