New resource: aws_pinpoint_app #5956

marcoreni · 2018-09-21T14:50:20Z

This is the first step to address #4990 .

More will (slowly) follow.

Changes proposed in this pull request:

New resource aws_pinpoint_app with related acceptance tests and docs

NOTE: cloudwatch_metrics_enabled is currently not returned by GetApplicationSettings requests so it's not currently handled inside the resource.

Output from acceptance testing:

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSPinpointApp'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSPinpointApp -timeout 120m
=== RUN   TestAccAWSPinpointApp_basic
--- PASS: TestAccAWSPinpointApp_basic (19.40s)
=== RUN   TestAccAWSPinpointApp_CampaignHookLambda
--- PASS: TestAccAWSPinpointApp_CampaignHookLambda (41.61s)
=== RUN   TestAccAWSPinpointApp_Limits
--- PASS: TestAccAWSPinpointApp_Limits (17.05s)
=== RUN   TestAccAWSPinpointApp_QuietTime
--- PASS: TestAccAWSPinpointApp_QuietTime (24.47s)
PASS
ok      github.com/terraform-providers/terraform-provider-aws/aws       103.733s

bflad

Hi @marcoreni! Thanks for submitting this -- its off to a good start. Please see the below for initial comments. Let us know if you have any questions or do not have time to implement the feedback. 😄

bflad · 2018-09-28T11:58:55Z

aws/resource_aws_pinpoint_app.go

+			//	Default:  false,
+			//},
+			"campaign_hook": {
+				Type:     schema.TypeSet,


We prefer Type: schema.TypeList for MaxItems: 1 attributes, which allows dropping `Set, simplification of the attribute testing, and easier future maintenance. 👍

Fixed in fd77a5b

bflad · 2018-09-28T11:59:14Z

aws/resource_aws_pinpoint_app.go

+				},
+			},
+			"limits": {
+				Type:     schema.TypeSet,


Same here. We prefer Type: schema.TypeList for MaxItems: 1 attributes, which allows dropping `Set, simplification of the attribute testing, and easier future maintenance. 👍

Fixed in fd77a5b

bflad · 2018-09-28T11:59:28Z

aws/resource_aws_pinpoint_app.go

+				},
+			},
+			"quiet_time": {
+				Type:     schema.TypeSet,


Same here. We prefer Type: schema.TypeList for MaxItems: 1 attributes, which allows dropping `Set, simplification of the attribute testing, and easier future maintenance. 👍

Fixed in fd77a5b

bflad · 2018-09-28T12:02:18Z

aws/resource_aws_pinpoint_app.go

+
+	output, err := pinpointconn.CreateApp(req)
+	if err != nil {
+		return fmt.Errorf("[ERROR] creating Pinpoint app: %s", err)


Nitpick: Using the hclog style [ERROR] can be replaced with the human readable error as it will be placed in the middle of the error messaging from Terraform to operators

Fixed in 0b9d8ff

aws/resource_aws_pinpoint_app.go

bflad · 2018-09-28T12:29:19Z

aws/resource_aws_pinpoint_app.go

+
+	m := map[string]interface{}{}
+
+	if ch.LambdaFunctionName != nil {


You can remove the nil checks by using the AWS Go SDK helpers here, e.g.

m["lambda_function_name"] = aws.StringValue(ch.LambdaFunctionName)

Especially for ch.Mode below, which does not need the != "" check.

I've been struggling with this while testing the resource. The issue I've encountered is that GetApplicationSettings returns an empty object for campaign_hook, limits and quiet_time even if none of the children attributes is set.

In this case, if I used

m["lambda_function_name"] = aws.StringValue(ch.LambdaFunctionName) m["mode"] = aws.StringValue(ch.Mode) m["web_url"] = aws.StringValue(ch.WebUrl)

I'd get a map filled with empty values, but that would cause failures on the state diff (for example, on the _basic test):

DIFF: UPDATE: aws_pinpoint_app.test_app campaign_hook.#: "1" => "0" STATE: aws_pinpoint_app.test_app: ID = ff3857dd45734e7ab07d9cb5141f0221 provider = provider.aws application_id = ff3857dd45734e7ab07d9cb5141f0221 campaign_hook.# = 1 campaign_hook.0.lambda_function_name = campaign_hook.0.mode = campaign_hook.0.web_url = name = terraform-20180928131703355600000001

Any suggestions on how to handle this scenario?

Great question! We'll be looking to come up with a better solution to this problem in the future, but for now, you can add the following DiffSuppressFunc to the campaign_hook attribute that ignores that the API is always returning the "container" attribute:

DiffSuppressFunc: func(k, old, new string, d *schema.ResourceData) bool { if old == "1" && new == "0" { return true } return false },

Thanks! Totally missed DiffSuppressFunc while exploring the code looking for a workaround :D Fixed in 4d39884

bflad · 2018-09-28T12:32:21Z

aws/resource_aws_pinpoint_app.go

+	d.Set("name", app.ApplicationResponse.Name)
+	d.Set("application_id", app.ApplicationResponse.Id)
+
+	d.Set("campaign_hook", flattenCampaignHook(settings.ApplicationSettingsResource.CampaignHook))


We should error check d.Set() when attempting to set more complex types like lists and maps to ensure they are properly being read into the Terraform state:

if err := d.Set("campaign_hook", flattenCampaignHook(settings.ApplicationSettingsResource.CampaignHook)); err != nil { return fmt.Errorf("error setting campaign_hook: %s", err) } if err := d.Set("limits", flattenCampaignLimits(settings.ApplicationSettingsResource.Limits)); err != nil { return fmt.Errorf("error setting limits: %s", err) } if err := d.Set("quiet_time", flattenQuietTime(settings.ApplicationSettingsResource.QuietTime)); err != nil { return fmt.Errorf("error setting campaign_hook: %s", err) }

Fixed in 1ea7cda

bflad · 2018-09-28T12:33:54Z

aws/resource_aws_pinpoint_app.go

+	m := map[string]interface{}{}
+
+	if qt.End != nil {
+		m["end"] = qt.End


I believe this would be found by the d.Set() error checking, but this should be converting the string pointer to a string:

m["end"] = aws.StringValue(qt.End)

Fixed in ea1e963

bflad · 2018-09-28T12:40:44Z

website/docs/r/pinpoint_app.markdown

+
+## Import
+
+CodeBuild Project can be imported using the `application-id`, e.g.


Copy-paste typo (CodeBuild Project) 😎

Fixed in 96756bf

aws/resource_aws_pinpoint_app_test.go

…ion names

…turn on missing resource

bflad · 2018-09-28T14:24:34Z

I dropped a note about the DiffSuppressFunc which should help out with the one problem you're seeing and looks like you're working through everything else already -- thanks so much! Let me know if there's anything else and when you're done. 😄

added DiffSuppressFunc to work around AWS returning empty objects

marcoreni · 2018-09-28T14:46:03Z

Hey @bflad , thanks for the thorough review and all the inputs!

I applied all the requested changes. I committed them separately to ease the review. If you think it's better, I can squash them.

Regarding DiffSuppressFunc I decided to repeat it on all the attributes to make it more explicit to a future reader.

Let me know if there's anything else to be improved :)

bflad

Looks great, @marcoreni! 🚀

--- PASS: TestAccAWSPinpointApp_basic (9.91s)
--- PASS: TestAccAWSPinpointApp_QuietTime (10.30s)
--- PASS: TestAccAWSPinpointApp_Limits (10.49s)
--- PASS: TestAccAWSPinpointApp_CampaignHookLambda (26.57s)

ghost · 2018-10-03T18:54:38Z

This has been released in version 1.39.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example:

provider "aws" {
	version = "~> 1.39.0"
}
# ... other configuration ...

ghost · 2020-04-03T17:17:36Z

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!

marcoreni added 3 commits September 21, 2018 16:37

resource/aws_pinpoint_app

df7b612

resource/aws_pinpoint_app: tests

bcf4511

resource/aws_pinpoint_app: docs

b015844

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 Sep 21, 2018

bflad added the service/pinpoint Issues and PRs that pertain to the pinpoint service. label Sep 24, 2018

bflad reviewed Sep 28, 2018

View reviewed changes

bflad added the waiting-response Maintainers are waiting on response from community or contributor. label Sep 28, 2018

marcoreni added 10 commits September 28, 2018 15:08

Docs: fix typo

96756bf

goimports

293a621

Refactoring TypeSet into TypeList and including service name in funct…

fd77a5b

…ion names

Change error log style

0b9d8ff

Check for errors while setting complex types

1ea7cda

Fix tests

ea1e963

Avoiod error if app is already deleted

be7dc00

tests: remove unused attributes, expose API Object instead & avoid re…

ca3d43d

…turn on missing resource

Simplify expand methods

d66ab63

add import TestStep

a40c8ce

Simplify flatten functions

4d39884

added DiffSuppressFunc to work around AWS returning empty objects

bflad approved these changes Sep 28, 2018

View reviewed changes

bflad added this to the v1.39.0 milestone Sep 28, 2018

bflad merged commit a75fc98 into hashicorp:master Sep 28, 2018

marcoreni deleted the feature/4990_pinpoint_app branch September 28, 2018 15:44

bflad added a commit that referenced this pull request Sep 28, 2018

Update CHANGELOG for #5956

4a1cbce

marcoreni mentioned this pull request Oct 1, 2018

Amazon Pinpoint #4990

Closed

ghost locked and limited conversation to collaborators Apr 3, 2020

breathingdust removed the waiting-response Maintainers are waiting on response from community or contributor. label Sep 17, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New resource: aws_pinpoint_app #5956

New resource: aws_pinpoint_app #5956

marcoreni commented Sep 21, 2018

bflad left a comment

bflad Sep 28, 2018

marcoreni Sep 28, 2018

bflad Sep 28, 2018

marcoreni Sep 28, 2018

bflad Sep 28, 2018

marcoreni Sep 28, 2018

bflad Sep 28, 2018

marcoreni Sep 28, 2018

bflad Sep 28, 2018

marcoreni Sep 28, 2018

bflad Sep 28, 2018

marcoreni Sep 28, 2018

bflad Sep 28, 2018

marcoreni Sep 28, 2018

bflad Sep 28, 2018

marcoreni Sep 28, 2018

bflad Sep 28, 2018

marcoreni Sep 28, 2018

bflad commented Sep 28, 2018

marcoreni commented Sep 28, 2018

bflad left a comment

ghost commented Oct 3, 2018

ghost commented Apr 3, 2020


		m := map[string]interface{}{}

		if ch.LambdaFunctionName != nil {


		## Import

		CodeBuild Project can be imported using the `application-id`, e.g.

New resource: aws_pinpoint_app #5956

New resource: aws_pinpoint_app #5956

Conversation

marcoreni commented Sep 21, 2018

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

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

bflad commented Sep 28, 2018

marcoreni commented Sep 28, 2018

bflad left a comment

Choose a reason for hiding this comment

ghost commented Oct 3, 2018

ghost commented Apr 3, 2020