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.

Sign up for GitHub

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

Jump to bottom

resource/aws_iam_group_policy: Properly handle generated policy name updates #4379

Merged
merged 1 commit into from
May 2, 2018
Merged

resource/aws_iam_group_policy: Properly handle generated policy name updates #4379

merged 1 commit into from
May 2, 2018

Conversation

bflad
Copy link
Contributor

@bflad bflad commented Apr 27, 2018

Fixes #4377

After testing updates but before code changes:

make testacc TEST=./aws TESTARGS='-run=TestAccAWSIAMGroupPolicy_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSIAMGroupPolicy_ -timeout 120m
=== RUN   TestAccAWSIAMGroupPolicy_basic
--- PASS: TestAccAWSIAMGroupPolicy_basic (123.65s)
=== RUN   TestAccAWSIAMGroupPolicy_namePrefix
--- FAIL: TestAccAWSIAMGroupPolicy_namePrefix (66.59s)
	testing.go:518: Step 1 error: Check failed: Check 2/2 error: IAM Group Policy name did not match
	testing.go:579: Error destroying resource! WARNING: Dangling resources
		may exist. The full state and error is shown below.

		Error: Error applying: 1 error(s) occurred:

		* aws_iam_group.test (destroy): 1 error(s) occurred:

		* aws_iam_group.test: Error deleting IAM Group test_group_6453479404941257412: DeleteConflict: Cannot delete entity, must delete policies first.
			status code: 409, request id: 4197118c-4a18-11e8-a475-37a7836ae511

		State: aws_iam_group.test:
		  ID = test_group_6453479404941257412
		  provider = provider.aws
		  arn = arn:aws:iam::187416307283:group/test_group_6453479404941257412
		  name = test_group_6453479404941257412
		  path = /
		  unique_id = AGPAIVYQCFZDGLRBR3NXS
=== RUN   TestAccAWSIAMGroupPolicy_generatedName
--- FAIL: TestAccAWSIAMGroupPolicy_generatedName (101.27s)
	testing.go:518: Step 1 error: Check failed: Check 2/2 error: IAM Group Policy name did not match
	testing.go:579: Error destroying resource! WARNING: Dangling resources
		may exist. The full state and error is shown below.

		Error: Error applying: 1 error(s) occurred:

		* aws_iam_group.test (destroy): 1 error(s) occurred:

		* aws_iam_group.test: Error deleting IAM Group test_group_4300747935912896569: DeleteConflict: Cannot delete entity, must delete policies first.
			status code: 409, request id: 7df6644f-4a18-11e8-bf04-d919114d6f8f

		State: aws_iam_group.test:
		  ID = test_group_4300747935912896569
		  provider = provider.aws
		  arn = arn:aws:iam::187416307283:group/test_group_4300747935912896569
		  name = test_group_4300747935912896569
		  path = /
		  unique_id = AGPAJN2FJ6BIMRBYTKK2E
FAIL
FAIL	github.com/terraform-providers/terraform-provider-aws/aws	291.541s
make: *** [testacc] Error 1

After code changes:

make testacc TEST=./aws TESTARGS='-run=TestAccAWSIAMGroupPolicy_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSIAMGroupPolicy_ -timeout 120m
=== RUN   TestAccAWSIAMGroupPolicy_basic
--- PASS: TestAccAWSIAMGroupPolicy_basic (102.28s)
=== RUN   TestAccAWSIAMGroupPolicy_namePrefix
--- PASS: TestAccAWSIAMGroupPolicy_namePrefix (80.25s)
=== RUN   TestAccAWSIAMGroupPolicy_generatedName
--- PASS: TestAccAWSIAMGroupPolicy_generatedName (71.56s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	254.121s

@bflad bflad added bug Addresses a defect in current functionality. service/iam Issues and PRs that pertain to the iam service. labels Apr 27, 2018
@bflad bflad added this to the v1.17.0 milestone Apr 27, 2018
@bflad bflad requested a review from a team April 27, 2018 12:53
@ghost ghost added the size/L Managed by automation to categorize the size of a PR. label Apr 27, 2018
@bflad bflad mentioned this pull request Apr 27, 2018
Closed
@bflad
Copy link
Contributor Author

bflad commented Apr 27, 2018

IAM Role policy was fine, but added additional testing as well that passes:

make testacc TEST=./aws TESTARGS='-run=TestAccAWSIAMRolePolicy_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSIAMRolePolicy_ -timeout 120m
=== RUN   TestAccAWSIAMRolePolicy_importBasic
--- PASS: TestAccAWSIAMRolePolicy_importBasic (11.28s)
=== RUN   TestAccAWSIAMRolePolicy_basic
--- PASS: TestAccAWSIAMRolePolicy_basic (26.58s)
=== RUN   TestAccAWSIAMRolePolicy_namePrefix
--- PASS: TestAccAWSIAMRolePolicy_namePrefix (17.46s)
=== RUN   TestAccAWSIAMRolePolicy_generatedName
--- PASS: TestAccAWSIAMRolePolicy_generatedName (15.94s)
=== RUN   TestAccAWSIAMRolePolicy_invalidJSON
--- PASS: TestAccAWSIAMRolePolicy_invalidJSON (0.91s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	72.204s

catsby
catsby reviewed Apr 27, 2018
View reviewed changes
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.

reviewing , hold merge for a minute...

@bflad bflad force-pushed the b-aws_iam_group_policy-updates branch from 5e124b7 to 681482a Compare April 27, 2018 20:06
@ghost ghost added the size/L Managed by automation to categorize the size of a PR. label Apr 27, 2018
@bflad
Copy link
Contributor Author

bflad commented Apr 27, 2018

Updated to just the d.Set() required changes. (Sorry out of office today)

make testacc TEST=./aws TESTARGS='-run=TestAccAWSIAMGroupPolicy_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSIAMGroupPolicy_ -timeout 120m
=== RUN   TestAccAWSIAMGroupPolicy_basic
--- PASS: TestAccAWSIAMGroupPolicy_basic (15.43s)
=== RUN   TestAccAWSIAMGroupPolicy_namePrefix
--- PASS: TestAccAWSIAMGroupPolicy_namePrefix (15.89s)
=== RUN   TestAccAWSIAMGroupPolicy_generatedName
--- PASS: TestAccAWSIAMGroupPolicy_generatedName (15.01s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	46.376s

 make testacc TEST=./aws TESTARGS='-run=TestAccAWSIAMRolePolicy_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSIAMRolePolicy_ -timeout 120m
=== RUN   TestAccAWSIAMRolePolicy_importBasic
--- PASS: TestAccAWSIAMRolePolicy_importBasic (12.33s)
=== RUN   TestAccAWSIAMRolePolicy_basic
--- PASS: TestAccAWSIAMRolePolicy_basic (21.37s)
=== RUN   TestAccAWSIAMRolePolicy_namePrefix
--- PASS: TestAccAWSIAMRolePolicy_namePrefix (20.19s)
=== RUN   TestAccAWSIAMRolePolicy_generatedName
--- PASS: TestAccAWSIAMRolePolicy_generatedName (16.81s)
=== RUN   TestAccAWSIAMRolePolicy_invalidJSON
--- PASS: TestAccAWSIAMRolePolicy_invalidJSON (0.96s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	71.704s

tombuildsstuff
tombuildsstuff approved these changes May 2, 2018
View reviewed changes
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 👍

aws/resource_aws_iam_group_policy_test.go
return func(s *terraform.State) error {
if aws.StringValue(i.PolicyName) != aws.StringValue(j.PolicyName) {
return errors.New("IAM Group Policy name did not match")
}
Copy link
Contributor

Choose a reason for hiding this comment

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

both of these ignore casings, but that's probably fine

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm pretty sure in terms of the acceptance testing we can allow it to be more strict, but if it presents some sort of problem we can certainly implement the strings.ToLower() calls 👍

@bflad bflad merged commit bfaaf12 into master May 2, 2018
@bflad bflad deleted the b-aws_iam_group_policy-updates branch May 2, 2018 14:18
bflad added a commit that referenced this pull request May 2, 2018
@bflad
Update CHANGELOG for #4379
e028ab4
@bflad
Copy link
Contributor Author

bflad commented May 2, 2018

This has been released in version 1.17.0 of the AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

@ghost
Copy link

ghost commented Apr 6, 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 6, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@catsby catsby catsby left review comments

@tombuildsstuff tombuildsstuff tombuildsstuff approved these changes

Assignees
No one assigned
Labels
bug Addresses a defect in current functionality. service/iam Issues and PRs that pertain to the iam service. size/L Managed by automation to categorize the size of a PR.
Projects
None yet
Milestone
v1.17.0
Development

Successfully merging this pull request may close these issues.

aws_iam_group_policy updates without name field leave orphaned attached policies
3 participants
@bflad @catsby @tombuildsstuff