#7741: Pinpoint App - Added resource tag support #9460

faishal · 2019-07-23T15:18:04Z

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

Release note for CHANGELOG:

resource/aws_pinpoint_app: Add tags argument (https://github.com/terraform-providers/terraform-provider-aws/issues/7741)

Output from acceptance testing:

$ make testacc TESTARGS='-run= TestAccAWSPinpointApp_Tags'

PASS

bflad

Hi @faishal 👋 Thanks so much for contributing this. Please see the below initial feedback. Reach out if you have any questions or do not have time to implement the items.

bflad · 2019-07-30T21:36:23Z

aws/resource_aws_pinpoint_app.go

@@ -128,6 +128,12 @@ func resourceAwsPinpointApp() *schema.Resource {
 					},
 				},
 			},
+			"arn": {
+				Type:     schema.TypeString,
+				Optional: true,


The ARN of the PinPoint Application cannot be set by the operator, so Optional: true should be removed. 👍

Suggested change

Optional: true,

bflad · 2019-07-30T21:38:24Z

aws/resource_aws_pinpoint_app.go

@@ -146,9 +154,14 @@ func resourceAwsPinpointAppCreate(d *schema.ResourceData, meta interface{}) erro

 	log.Printf("[DEBUG] Pinpoint create app: %s", name)

+	if v, ok := d.GetOk("tags"); ok {
+		tags = tagsFromMapPinPointApp(v.(map[string]interface{}))


Instead of creating a new variable to store the tags, this logic can be moved below the CreateAppInput declaration and call req.CreateApplicationRequest.Tags = directly.

req := &pinpoint.CreateAppInput{ CreateApplicationRequest: &pinpoint.CreateApplicationRequest{ Name: aws.String(name), }, } if v, ok := d.GetOk("tags"); ok { req.CreateApplicationRequest.Tags = tagsFromMapPinPointApp(v.(map[string]interface{})) }

Make sense, Implemented 👍

bflad · 2019-07-30T21:39:57Z

aws/resource_aws_pinpoint_app.go

@@ -193,6 +207,12 @@ func resourceAwsPinpointAppUpdate(d *schema.ResourceData, meta interface{}) erro
 		return err
 	}

+	if err := setTagsPinPointApp(conn, d); err != nil {
+		return fmt.Errorf(
+			"Error Updating PinPoint App tags: \"%s\"\n%s",


We prefer to not introduce extra newlines into our error messaging:

return fmt.Errorf("error updating PinPoint Application (%s) tags: %s", d.Id(), err)

Make sense, Implemented 👍

bflad · 2019-07-30T21:41:06Z

aws/resource_aws_pinpoint_app_test.go

@@ -2,6 +2,7 @@ package aws

 import (
 	"fmt"
+	"github.com/hashicorp/terraform/helper/acctest"


Nit: Go imports should be formatted as stdlib imports, new line, third party imports with each section alphabetically sorted. Tooling like goimports can help do this automatically in editors. 😄

Didn't know that, Move to correct position based on other files 👍

bflad · 2019-07-30T21:41:58Z

aws/tagsPinPointApp.go

+
+// getTags is a helper to get the tags for a resource. It expects the
+// tags field to be named "tags"
+func getTagsPinPointApp(conn *pinpoint.Pinpoint, d *schema.ResourceData, sn string) error {


The sn parameter to this function appears to be unused and should be removed.

Suggested change

func getTagsPinPointApp(conn *pinpoint.Pinpoint, d *schema.ResourceData, sn string) error {

func getTagsPinPointApp(conn *pinpoint.Pinpoint, d *schema.ResourceData) error {

Nice catch, Removed and updated function call.

bflad · 2019-07-30T21:43:29Z

aws/resource_aws_pinpoint_app.go

@@ -128,6 +128,12 @@ func resourceAwsPinpointApp() *schema.Resource {
 					},
 				},
 			},
+			"arn": {


This new attribute is missing documentation in website/docs/r/pinpoint_app.html.markdown, e.g. (under ## Attributes)

* `arn` - Amazon Resource Name (ARN) of the PinPoint Application

bflad · 2019-07-30T21:44:24Z

aws/resource_aws_pinpoint_app.go

+				Optional: true,
+				Computed: true,
+			},
+			"tags": tagsSchema(),


This new argument is missing documentation in website/docs/r/pinpoint_app.html.markdown, e.g. (under ## Arguments)

* `tags` - (Optional) Key-value mapping of resource tags

faishal · 2019-07-31T14:18:07Z

@bflad - Thanks for the feedback. I have fixed all of them and ready for another review 🎉

bflad

Great updates, @faishal, LGTM! 🚀

--- PASS: TestAccAWSPinpointApp_QuietTime (16.03s)
--- PASS: TestAccAWSPinpointApp_basic (16.87s)
--- PASS: TestAccAWSPinpointApp_Tags (33.26s)
--- PASS: TestAccAWSPinpointApp_CampaignHookLambda (38.13s)

ghost · 2019-08-01T19:49:09Z

This has been released in version 2.22.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-02T15:02:31Z

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!

hashicorp#7741: Pinpoint App - Added resource tag support

bf7c4a0

faishal requested a review from a team July 23, 2019 15:18

ghost added size/XL Managed by automation to categorize the size of a PR. service/pinpoint Issues and PRs that pertain to the pinpoint service. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. labels Jul 23, 2019

Merge branch 'master' into 7741-pinpoint-tags

1ba8e1e

bflad suggested changes Jul 30, 2019

View reviewed changes

bflad added enhancement Requests to existing resources that expand the functionality or scope. waiting-response Maintainers are waiting on response from community or contributor. labels Jul 30, 2019

bflad self-assigned this Jul 30, 2019

7741: Fix issues reported in initial feedback

a6a2594

ghost added the documentation Introduces or discusses updates to documentation. label Jul 31, 2019

ghost removed the waiting-response Maintainers are waiting on response from community or contributor. label Jul 31, 2019

bflad self-requested a review July 31, 2019 14:56

bflad added this to the v2.22.0 milestone Jul 31, 2019

bflad approved these changes Jul 31, 2019

View reviewed changes

bflad merged commit dd2d66a into hashicorp:master Jul 31, 2019

bflad added a commit that referenced this pull request Jul 31, 2019

Update CHANGELOG for #9460

12c1c45

faishal deleted the 7741-pinpoint-tags branch July 31, 2019 15:31

ghost locked and limited conversation to collaborators Nov 2, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#7741: Pinpoint App - Added resource tag support #9460

#7741: Pinpoint App - Added resource tag support #9460

faishal commented Jul 23, 2019

bflad left a comment

bflad Jul 30, 2019

faishal Jul 31, 2019

bflad Jul 30, 2019

faishal Jul 31, 2019

bflad Jul 30, 2019

faishal Jul 31, 2019

bflad Jul 30, 2019

faishal Jul 31, 2019

bflad Jul 30, 2019

faishal Jul 31, 2019

bflad Jul 30, 2019

faishal Jul 31, 2019

bflad Jul 30, 2019

faishal Jul 31, 2019

faishal commented Jul 31, 2019

bflad left a comment

ghost commented Aug 1, 2019

ghost commented Nov 2, 2019

	func getTagsPinPointApp(conn pinpoint.Pinpoint, d schema.ResourceData, sn string) error {
	func getTagsPinPointApp(conn pinpoint.Pinpoint, d schema.ResourceData) error {

#7741: Pinpoint App - Added resource tag support #9460

#7741: Pinpoint App - Added resource tag support #9460

Conversation

faishal commented Jul 23, 2019

Community Note

bflad left a 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

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

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

faishal commented Jul 31, 2019

bflad left a comment

Choose a reason for hiding this comment

ghost commented Aug 1, 2019

ghost commented Nov 2, 2019