Feature/add resource tags for appmesh mesh resource #8111

teraken0509 · 2019-03-28T10:04:50Z

Community Note

Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
Please do not leave "+1" comments, they generate extra noise for pull request followers and do not help prioritize the request

Reference #8101

Changes proposed in this pull request:

Add tag resource for appmesh service
Add tags attribute for aws_appmesh_mesh resource

Output from acceptance testing:

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSAppmesh/Mesh'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -parallel 20 -run=TestAccAWSAppmesh/Mesh -timeout 120m
=== RUN   TestAccAWSAppmesh
=== RUN   TestAccAWSAppmesh/Mesh
=== RUN   TestAccAWSAppmesh/Mesh/tags
=== RUN   TestAccAWSAppmesh/Mesh/basic
--- PASS: TestAccAWSAppmesh (108.16s)
    --- PASS: TestAccAWSAppmesh/Mesh (108.16s)
        --- PASS: TestAccAWSAppmesh/Mesh/tags (77.71s)
        --- PASS: TestAccAWSAppmesh/Mesh/basic (30.45s)
PASS
ok  	github.com/terraform-providers/tmp-terraform-provider-aws/aws	108.233s

teraken0509 · 2019-04-12T04:46:49Z

Output from acceptance testing:

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSAppmesh/Mesh'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -parallel 20 -run=TestAccAWSAppmesh/Mesh -timeout 120m
=== RUN   TestAccAWSAppmesh
=== RUN   TestAccAWSAppmesh/Mesh
=== RUN   TestAccAWSAppmesh/Mesh/basic
=== RUN   TestAccAWSAppmesh/Mesh/egressFilter
=== RUN   TestAccAWSAppmesh/Mesh/tags
--- PASS: TestAccAWSAppmesh (179.38s)
    --- PASS: TestAccAWSAppmesh/Mesh (179.38s)
        --- PASS: TestAccAWSAppmesh/Mesh/basic (37.31s)
        --- PASS: TestAccAWSAppmesh/Mesh/egressFilter (67.56s)
        --- PASS: TestAccAWSAppmesh/Mesh/tags (74.51s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	179.459s

ewbankkit · 2019-04-21T20:34:34Z

Can we get a review of this please?
tagsAppmesh.go is required for adding resource tags to the other App Mesh resources listed in #8101.

ewbankkit · 2019-04-21T20:47:15Z

aws/resource_aws_appmesh_mesh.go

@@ -127,6 +130,15 @@ func resourceAwsAppmeshMeshRead(d *schema.ResourceData, meta interface{}) error
 return fmt.Errorf("error setting spec: %s", err)
 }

+ if err := saveTagsAppmesh(conn, d, aws.StringValue(resp.Mesh.Metadata.Arn)); err != nil {


I think the preferred style for error handling is

err = saveTagsAppmesh(conn, d, aws.StringValue(resp.Mesh.Metadata.Arn)) if isAWSErr(err, appmesh.ErrCodeNotFoundException, "") { log.Printf("[WARN] App Mesh service mesh (%s) not found, removing from state", d.Id()) d.SetId("") return nil } if err != nil { return fmt.Errorf("error saving tags: %s", err) }

ewbankkit · 2019-04-21T20:48:02Z

aws/resource_aws_appmesh_mesh.go

@@ -147,6 +159,15 @@ func resourceAwsAppmeshMeshUpdate(d *schema.ResourceData, meta interface{}) erro
 }
 }

+ if err := setTagsAppmesh(conn, d, d.Get("arn").(string)); err != nil {


Same comment as above.

teraken0509 · 2019-04-22T04:15:38Z

Re-run tests.

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSAppmesh/Mesh'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -parallel 20 -run=TestAccAWSAppmesh/Mesh -timeout 120m
=== RUN   TestAccAWSAppmesh
=== RUN   TestAccAWSAppmesh/Mesh
=== RUN   TestAccAWSAppmesh/Mesh/basic
=== RUN   TestAccAWSAppmesh/Mesh/egressFilter
=== RUN   TestAccAWSAppmesh/Mesh/tags
--- PASS: TestAccAWSAppmesh (185.23s)
    --- PASS: TestAccAWSAppmesh/Mesh (185.23s)
        --- PASS: TestAccAWSAppmesh/Mesh/basic (36.70s)
        --- PASS: TestAccAWSAppmesh/Mesh/egressFilter (68.07s)
        --- PASS: TestAccAWSAppmesh/Mesh/tags (80.46s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	185.314s

teraken0509 · 2019-04-25T07:49:37Z

@ewbankkit could you review fixes?

ewbankkit · 2019-06-27T15:32:47Z

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSAppmesh/Mesh'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -parallel 20 -run=TestAccAWSAppmesh/Mesh -timeout 120m
=== RUN   TestAccAWSAppmesh
=== RUN   TestAccAWSAppmesh/Mesh
=== RUN   TestAccAWSAppmesh/Mesh/egressFilter
=== RUN   TestAccAWSAppmesh/Mesh/tags
=== RUN   TestAccAWSAppmesh/Mesh/basic
--- PASS: TestAccAWSAppmesh (115.91s)
    --- PASS: TestAccAWSAppmesh/Mesh (115.91s)
        --- PASS: TestAccAWSAppmesh/Mesh/egressFilter (44.72s)
        --- PASS: TestAccAWSAppmesh/Mesh/tags (50.00s)
        --- PASS: TestAccAWSAppmesh/Mesh/basic (21.19s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	115.996s

ewbankkit · 2019-06-27T15:45:33Z

aws/resource_aws_appmesh_mesh.go

@@ -157,7 +180,7 @@ func resourceAwsAppmeshMeshDelete(d *schema.ResourceData, meta interface{}) erro
 _, err := conn.DeleteMesh(&appmesh.DeleteMeshInput{
 MeshName: aws.String(d.Id()),
 })
- if isAWSErr(err, "NotFoundException", "") {
+ if isAWSErr(err, appmesh.ErrCodeNotFoundException, "") {


Nice. Could you also replace the hard-coded string value in resourceAwsAppmeshMeshRead()? https:/terraform-providers/terraform-provider-aws/blob/1bb0dead12829bf6c9da9e3170f52a8f24984497/aws/resource_aws_appmesh_mesh.go#L107

ewbankkit · 2019-06-27T15:51:10Z

aws/tagsAppmesh.go

+ }
+
+ // Build the list of what to remove
+ var remove []*string


Need to handle the overlapping tags case, e.g. from ECR https:/terraform-providers/terraform-provider-aws/blob/1bb0dead12829bf6c9da9e3170f52a8f24984497/aws/tagsECR.go#L79-L89

See #7052.

ewbankkit · 2019-06-27T15:54:34Z

aws/tagsAppmesh.go

+// compare a tag against a list of strings and checks if it should
+// be ignored or not
+func tagIgnoredAppmesh(t *appmesh.TagRef) bool {
+ filter := []string{"^aws:", "^appmesh:", "Name"}


Is there a need to exclude Name and appmesh: here?
In my testing these tags don't seem to be added automagically and I can add manually:

$ aws --region us-west-2 appmesh tag-resource --resource-arn "arn:aws:appmesh:us-west-2:000000000000:mesh/simpleapp" --tags "key=Name,value=test" $ aws --region us-west-2 appmesh tag-resource --resource-arn "arn:aws:appmesh:us-west-2:000000000000:mesh/simpleapp" --tags "key=appmesh:k,value=v" $ aws --region us-west-2 appmesh list-tags-for-resource --resource-arn "arn:aws:appmesh:us-west-2:000000000000:mesh/simpleapp" { "tags": [ { "value": "test", "key": "Name" }, { "value": "v", "key": "appmesh:k" } ] }

Thanks, I remove Name and appmesh: from ignore list

teraken0509 · 2019-06-27T22:39:10Z

Re-run acctest.

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSAppmesh/Mesh'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -parallel 20 -run=TestAccAWSAppmesh/Mesh -timeout 120m
=== RUN   TestAccAWSAppmesh
=== RUN   TestAccAWSAppmesh/Mesh
=== RUN   TestAccAWSAppmesh/Mesh/basic
=== RUN   TestAccAWSAppmesh/Mesh/egressFilter
=== RUN   TestAccAWSAppmesh/Mesh/tags
--- PASS: TestAccAWSAppmesh (142.94s)
    --- PASS: TestAccAWSAppmesh/Mesh (142.94s)
        --- PASS: TestAccAWSAppmesh/Mesh/basic (27.22s)
        --- PASS: TestAccAWSAppmesh/Mesh/egressFilter (52.34s)
        --- PASS: TestAccAWSAppmesh/Mesh/tags (63.38s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	143.056s

bflad

Thanks for helping review @ewbankkit and for this implementation @kterada0509! LGTM 🚀

--- PASS: TestAccAWSAppmesh (131.28s)

bflad · 2019-06-27T23:41:31Z

(Fixing the minor unit/linting error on merge so we can get this released today)

aws/tagsAppmesh_test.go:29:4: cannot use map[string]string literal (type map[string]string) as type []string in field value
aws/tagsAppmesh_test.go:43:4: cannot use map[string]string literal (type map[string]string) as type []string in field value
aws/tagsAppmesh_test.go:61:4: cannot use map[string]string literal (type map[string]string) as type []string in field value
aws/tagsAppmesh_test.go:76:4: cannot use map[string]string literal (type map[string]string) as type []string in field value

Reference: #8111 (comment)

bflad · 2019-06-28T01:46:37Z

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

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template for triage. Thanks!

ghost · 2019-11-03T15:08:07Z

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 added size/XL Managed by automation to categorize the size of a PR. documentation Introduces or discusses updates to documentation. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. labels Mar 28, 2019

This was referenced Mar 28, 2019

Tech debt: Consistently handle overlapping resource tags when updating #7052

Closed

AWS App Mesh tagging #8101

Closed

bflad added enhancement Requests to existing resources that expand the functionality or scope. service/appmesh Issues and PRs that pertain to the appmesh service. labels Mar 28, 2019

teraken0509 force-pushed the feature/add-resource-tags-for-appmesh-mesh-resource branch from 44bc6ef to 15c199a Compare April 12, 2019 04:40

ewbankkit reviewed Apr 21, 2019

View reviewed changes

teraken0509 force-pushed the feature/add-resource-tags-for-appmesh-mesh-resource branch from 15c199a to f1fe342 Compare April 22, 2019 04:12

aeschright requested review from a team and removed request for a team June 26, 2019 16:50

ewbankkit mentioned this pull request Jun 27, 2019

AWS App Mesh service discovery using AWS Cloud Map #8997

Closed

ewbankkit reviewed Jun 27, 2019

View reviewed changes

teraken0509 added 3 commits June 28, 2019 07:33

Add tag resources method for appmesh service

e752270

Add tags attribute for aws_appmesh_mesh resource

94cf6df

fix error handling

0aa114d

teraken0509 force-pushed the feature/add-resource-tags-for-appmesh-mesh-resource branch from f1fe342 to f1de04e Compare June 27, 2019 22:38

teraken0509 requested a review from a team June 27, 2019 22:38

refactor resource tag for appmesh

0aa99f7

teraken0509 force-pushed the feature/add-resource-tags-for-appmesh-mesh-resource branch from f1de04e to 0aa99f7 Compare June 27, 2019 22:58

bflad approved these changes Jun 27, 2019

View reviewed changes

bflad added this to the v2.17.0 milestone Jun 27, 2019

bflad merged commit 0aa99f7 into hashicorp:master Jun 28, 2019

bflad added a commit that referenced this pull request Jun 28, 2019

tests/service/appmesh: Fix Go type issue

a113046

Reference: #8111 (comment)

bflad added a commit that referenced this pull request Jun 28, 2019

Update CHANGELOG for #8111

4ddd73b

ghost locked and limited conversation to collaborators Nov 3, 2019

teraken0509 deleted the feature/add-resource-tags-for-appmesh-mesh-resource branch March 5, 2020 14:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/add resource tags for appmesh mesh resource #8111

Feature/add resource tags for appmesh mesh resource #8111

teraken0509 commented Mar 28, 2019 •

edited by bflad

Loading

teraken0509 commented Apr 12, 2019

ewbankkit commented Apr 21, 2019

ewbankkit Apr 21, 2019

teraken0509 Apr 22, 2019

ewbankkit Apr 21, 2019

teraken0509 Apr 22, 2019

teraken0509 commented Apr 22, 2019

teraken0509 commented Apr 25, 2019

ewbankkit commented Jun 27, 2019

ewbankkit Jun 27, 2019

teraken0509 Jun 27, 2019

ewbankkit Jun 27, 2019

teraken0509 Jun 27, 2019

ewbankkit Jun 27, 2019

teraken0509 Jun 27, 2019

teraken0509 commented Jun 27, 2019

bflad left a comment

bflad commented Jun 27, 2019

bflad commented Jun 28, 2019

ghost commented Nov 3, 2019

Feature/add resource tags for appmesh mesh resource #8111

Feature/add resource tags for appmesh mesh resource #8111

Conversation

teraken0509 commented Mar 28, 2019 • edited by bflad Loading

Community Note

teraken0509 commented Apr 12, 2019

ewbankkit commented Apr 21, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teraken0509 commented Apr 22, 2019

teraken0509 commented Apr 25, 2019

ewbankkit commented Jun 27, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teraken0509 commented Jun 27, 2019

bflad left a comment

Choose a reason for hiding this comment

bflad commented Jun 27, 2019

bflad commented Jun 28, 2019

ghost commented Nov 3, 2019

teraken0509 commented Mar 28, 2019 •

edited by bflad

Loading