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

resource/aws_ssm_parameter: ForceNew on ssm_parameter rename #1022

Merged
merged 1 commit into from
Jul 6, 2017

Conversation

stack72
Copy link
Contributor

@stack72 stack72 commented Jun 30, 2017

Fixes: #1008

There doesn't seem to be a way to change the name of an AWS SSM
Parameter. Therefore, it should be marked as ForceNew.

% make testacc TEST=./aws TESTARGS='-run=TestAccAWSSSMParameter_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSSSMParameter_ -timeout 120m
=== RUN   TestAccAWSSSMParameter_basic
--- PASS: TestAccAWSSSMParameter_basic (25.06s)
=== RUN   TestAccAWSSSMParameter_update
--- PASS: TestAccAWSSSMParameter_update (43.21s)
=== RUN   TestAccAWSSSMParameter_changeNameForcesNew
--- PASS: TestAccAWSSSMParameter_changeNameForcesNew (40.84s)
=== RUN   TestAccAWSSSMParameter_secure
--- PASS: TestAccAWSSSMParameter_secure (37.90s)
=== RUN   TestAccAWSSSMParameter_secure_with_key
--- PASS: TestAccAWSSSMParameter_secure_with_key (73.26s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	220.297s

@stack72 stack72 added the bug Addresses a defect in current functionality. label Jun 30, 2017
Copy link
Member

@radeksimko radeksimko left a comment

Choose a reason for hiding this comment

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

I'm 👌 with the one-line change of the schema, that makes total sense to me.

I left you some comments about the test though.

@@ -183,6 +202,16 @@ resource "aws_ssm_parameter" "foo" {
`, rName, value)
}

func testAccAWSSSMParameterBasicConfigChangeName(rName string, value string) string {
Copy link
Member

Choose a reason for hiding this comment

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

What is the reason for having a copy of testAccAWSSSMParameterBasicConfig with a different name instead of just passing a different name as argument? 🙂

{
Config: testAccAWSSSMParameterBasicConfigChangeName(name, "baz1"),
PlanOnly: true,
ExpectNonEmptyPlan: true,
Copy link
Member

Choose a reason for hiding this comment

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

I think this test isn't testing the new behaviour. 🤔 (a) It is changing the value, not the name. (b) even if it was changing the name, we'd still need to check existence of the old SSM parameter via API as it gets wiped from state, so testAccCheckAWSSSMParameterDestroy wouldn't catch it.

@radeksimko radeksimko added the waiting-response Maintainers are waiting on response from community or contributor. label Jul 3, 2017
@stack72
Copy link
Contributor Author

stack72 commented Jul 6, 2017

@radeksimko you are correct - the test wasn't correct. This has been updated now and it checks the API to make sure that the old Parameter has been removed :)

Fixes: #1008

There doesn't seem to be a way to change the name of an AWS SSM
Parameter. Therefore, it should be marked as ForceNew.

```
% make testacc TEST=./aws TESTARGS='-run=TestAccAWSSSMParameter_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSSSMParameter_ -timeout 120m
=== RUN   TestAccAWSSSMParameter_basic
--- PASS: TestAccAWSSSMParameter_basic (25.06s)
=== RUN   TestAccAWSSSMParameter_update
--- PASS: TestAccAWSSSMParameter_update (43.21s)
=== RUN   TestAccAWSSSMParameter_changeNameForcesNew
--- PASS: TestAccAWSSSMParameter_changeNameForcesNew (40.84s)
=== RUN   TestAccAWSSSMParameter_secure
--- PASS: TestAccAWSSSMParameter_secure (37.90s)
=== RUN   TestAccAWSSSMParameter_secure_with_key
--- PASS: TestAccAWSSSMParameter_secure_with_key (73.26s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	220.297s
```
Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

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

otherwise LGTM :)

}

resp, err := conn.GetParameters(paramInput)
if err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

as mentioned on Slack - I'm surprised this doesn't return a 404 but ¯_(ツ)_/¯

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It will return an empty response object :) But a response none the less

@stack72 stack72 merged commit 0e47e25 into master Jul 6, 2017
@stack72 stack72 deleted the b-aws-ssm-param-forcenew branch July 6, 2017 11:40
@ghost
Copy link

ghost commented Apr 11, 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 11, 2020
@breathingdust breathingdust removed the waiting-response Maintainers are waiting on response from community or contributor. label Sep 17, 2021
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Changing the name option for a aws_ssm_parameter creates a new parameter in the Parameter Store
4 participants