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

diffTags should not identify existing tags as needing creation #5108

Merged
merged 1 commit into from
Nov 8, 2018

Conversation

domwong
Copy link
Contributor

@domwong domwong commented Jul 6, 2018

Changes proposed in this pull request:

  • diffTags incorrectly returns tags that are the same in oldTags and newTags as needing to be created again. This causes unnecessary calls to CreateTags. Fix this to return only tags that need to be deleted and created

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAccAWSInstance.*Tag'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -run=TestAccAWSInstance.*Tag -timeout 120m
?   	github.com/terraform-providers/terraform-provider-aws	[no test files]
=== RUN   TestAccAWSInstance_volumeTags
--- PASS: TestAccAWSInstance_volumeTags (186.28s)
=== RUN   TestAccAWSInstance_volumeTagsComputed
--- FAIL: TestAccAWSInstance_volumeTagsComputed (18.64s)
	testing.go:518: Step 0 error: Error applying: 1 error(s) occurred:
		
		* aws_instance.foo: 1 error(s) occurred:
		
		* aws_instance.foo: Error launching source instance: InstanceLimitExceeded: You have requested more instances (1) than your current instance limit of 0 allows for the specified instance type. Please visit http://aws.amazon.com/contact-us/ec2-request to request an adjustment to this limit.
			status code: 400, request id: ad836a70-ded9-4335-94f4-18871c6c96c0
=== RUN   TestAccAWSInstance_forceNewAndTagsDrift
--- PASS: TestAccAWSInstance_forceNewAndTagsDrift (290.62s)
FAIL
FAIL	github.com/terraform-providers/terraform-provider-aws/aws	495.584s
make: *** [testacc] Error 1

Note failures due to account limits

This PR split out from #5072.

@ghost ghost added the size/S Managed by automation to categorize the size of a PR. label Jul 6, 2018
@domwong
Copy link
Contributor Author

domwong commented Jul 6, 2018

@bflad - continuing on from our discussion on #5072 ...

Does this logic potentially exist to handle tag value updates for an existing tag key? Some service APIs do not support updating an existing tag so therefore require delete and create.

I misread this in my initial reply. This update scenario is actually covered in the test case I've added. e.g. updating foo:bar to foo:baz results in corresponding "create" and "remove" calls.

@bflad bflad added bug Addresses a defect in current functionality. service/ec2 Issues and PRs that pertain to the ec2 service. labels Jul 6, 2018
@bflad bflad added this to the v1.43.1 milestone Nov 8, 2018
Copy link
Contributor

@bflad bflad left a comment

Choose a reason for hiding this comment

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

Thanks so much for submitting this @domwong and sorry for the delayed review! 😅 LGTM also running this across a few of our EC2 resource acceptance tests that utilize tagging. 🚀

@bflad bflad merged commit fb91a49 into hashicorp:master Nov 8, 2018
bflad added a commit that referenced this pull request Nov 8, 2018
@bflad
Copy link
Contributor

bflad commented Nov 9, 2018

This has been released in version 1.43.1 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 2, 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 2, 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/ec2 Issues and PRs that pertain to the ec2 service. size/S Managed by automation to categorize the size of a PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants