Skip to content
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

Store all config-specified DB params in state. #3182

Merged
merged 2 commits into from
Jan 29, 2018
Merged

Conversation

paddycarver
Copy link
Contributor

Prior to this commit, only database parameters that AWS recognised as
being user-altered were stored in state. This would be fine, except if
users specified the same value for a parameter as AWS' default for that
value, they would get a perpetual diff, because AWS would report it as a
non-user-changed parameter. Meaning it wouldn't be set in state, no
matter how many times the user applied.

This fixes the problem by retrieving all parameters, regardless of how
AWS reports them being set. We then select out from those parameters
only the ones that report that they're user-modified or those that are
found in a config, if a config is available.

This commit also adds a test which exhibits this behaviour, which failed
before this fix was applied, and passes now that it has been applied.

Test output:

make testacc TEST=./aws TESTARGS='-run=TestAccAWSDBParameterGroup'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSDBParameterGroup -timeout 120m
=== RUN   TestAccAWSDBParameterGroup_importBasic
2018/01/29 06:36:10 [WARN] Invalid log level: "1". Defaulting to level: TRACE. Valid levels are: [TRACE DEBUG INFO WARN ERROR]
--- PASS: TestAccAWSDBParameterGroup_importBasic (23.90s)
=== RUN   TestAccAWSDBParameterGroup_limit
--- PASS: TestAccAWSDBParameterGroup_limit (38.34s)
=== RUN   TestAccAWSDBParameterGroup_basic
--- PASS: TestAccAWSDBParameterGroup_basic (37.90s)
=== RUN   TestAccAWSDBParameterGroup_Disappears
--- PASS: TestAccAWSDBParameterGroup_Disappears (14.18s)
=== RUN   TestAccAWSDBParameterGroup_namePrefix
--- PASS: TestAccAWSDBParameterGroup_namePrefix (20.24s)
=== RUN   TestAccAWSDBParameterGroup_generatedName
--- PASS: TestAccAWSDBParameterGroup_generatedName (20.38s)
=== RUN   TestAccAWSDBParameterGroup_withApplyMethod
--- PASS: TestAccAWSDBParameterGroup_withApplyMethod (20.29s)
=== RUN   TestAccAWSDBParameterGroup_Only
--- PASS: TestAccAWSDBParameterGroup_Only (18.85s)
=== RUN   TestAccAWSDBParameterGroup_MatchDefault
--- PASS: TestAccAWSDBParameterGroup_MatchDefault (19.41s)
PASS
ok      github.com/terraform-providers/terraform-provider-aws/aws       213.508s

Fixes #593.

Prior to this commit, only database parameters that AWS recognised as
being user-altered were stored in state. This would be fine, except if
users specified the same value for a parameter as AWS' default for that
value, they would get a perpetual diff, because AWS would report it as a
non-user-changed parameter. Meaning it wouldn't be set in state, no
matter how many times the user applied.

This fixes the problem by retrieving all parameters, regardless of how
AWS reports them being set. We then select out from those parameters
only the ones that report that they're user-modified _or_ those that are
found in a config, if a config is available.

This commit also adds a test which exhibits this behaviour, which failed
before this fix was applied, and passes now that it has been applied.
@paddycarver paddycarver added the bug Addresses a defect in current functionality. label Jan 29, 2018
@ghost ghost added the size/L Managed by automation to categorize the size of a PR. label Jan 29, 2018
@radeksimko radeksimko added the service/rds Issues and PRs that pertain to the rds service. label Jan 29, 2018
Copy link
Contributor

@catsby catsby left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 one request otherwise looks great!

}
}

d.Set("parameter", flattenParameters(userParams))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check the error here please

Because our parameters are a set, let's just check the error when
setting them in state, just in case.
@ghost ghost added the size/L Managed by automation to categorize the size of a PR. label Jan 29, 2018
@paddycarver
Copy link
Contributor Author

Believe I addressed the request, happy with the resolution of it?

@paddycarver paddycarver merged commit 2ef79ee into master Jan 29, 2018
@radeksimko radeksimko added this to the v1.9.0 milestone Jan 31, 2018
@radeksimko radeksimko deleted the paddy_593 branch January 31, 2018 11:36
@bflad
Copy link
Contributor

bflad commented Feb 9, 2018

This has been released in terraform-provider-aws version 1.9.0. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

@ghost
Copy link

ghost commented Apr 8, 2020

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 ghost locked and limited conversation to collaborators Apr 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Addresses a defect in current functionality. service/rds Issues and PRs that pertain to the rds service. size/L Managed by automation to categorize the size of a PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

aws_db_parameter_group plan shows changes even after applying
4 participants