Refactor Pinpoint application resource to use keyvaluetags package #11368

ewbankkit · 2019-12-19T15:39:59Z

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" or other comments that do not add relevant new information or questions, they generate extra noise for pull request followers and do not help prioritize the request

Relates #10688.

Release note for CHANGELOG:

resource/aws_pinpoint_app: Add plan-time validation for `limit` configuration block's `daily`, `maximum_duration`, `messages_per_second` and `total` arguments

Output from acceptance testing:

# Resource in supported region:
$ make testacc TEST=./aws/ TESTARGS='-run=TestAccAWSPinpointApp_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws/ -v -count 1 -parallel 20 -run=TestAccAWSPinpointApp_ -timeout 120m
=== RUN   TestAccAWSPinpointApp_basic
=== PAUSE TestAccAWSPinpointApp_basic
=== RUN   TestAccAWSPinpointApp_CampaignHookLambda
=== PAUSE TestAccAWSPinpointApp_CampaignHookLambda
=== RUN   TestAccAWSPinpointApp_Limits
=== PAUSE TestAccAWSPinpointApp_Limits
=== RUN   TestAccAWSPinpointApp_QuietTime
=== PAUSE TestAccAWSPinpointApp_QuietTime
=== RUN   TestAccAWSPinpointApp_Tags
=== PAUSE TestAccAWSPinpointApp_Tags
=== CONT  TestAccAWSPinpointApp_basic
=== CONT  TestAccAWSPinpointApp_Tags
=== CONT  TestAccAWSPinpointApp_QuietTime
=== CONT  TestAccAWSPinpointApp_Limits
=== CONT  TestAccAWSPinpointApp_CampaignHookLambda
--- PASS: TestAccAWSPinpointApp_QuietTime (24.91s)
--- PASS: TestAccAWSPinpointApp_Limits (25.52s)
--- PASS: TestAccAWSPinpointApp_basic (25.77s)
--- PASS: TestAccAWSPinpointApp_CampaignHookLambda (49.29s)
--- PASS: TestAccAWSPinpointApp_Tags (58.52s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	58.586s
# Test in unsupported region:
$ AWS_DEFAULT_REGION=us-east-2 make testacc TEST=./aws/ TESTARGS='-run=TestAccAWSPinpointApp_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws/ -v -count 1 -parallel 20 -run=TestAccAWSPinpointApp_ -timeout 120m
=== RUN   TestAccAWSPinpointApp_basic
=== PAUSE TestAccAWSPinpointApp_basic
=== RUN   TestAccAWSPinpointApp_CampaignHookLambda
=== PAUSE TestAccAWSPinpointApp_CampaignHookLambda
=== RUN   TestAccAWSPinpointApp_Limits
=== PAUSE TestAccAWSPinpointApp_Limits
=== RUN   TestAccAWSPinpointApp_QuietTime
=== PAUSE TestAccAWSPinpointApp_QuietTime
=== RUN   TestAccAWSPinpointApp_Tags
=== PAUSE TestAccAWSPinpointApp_Tags
=== CONT  TestAccAWSPinpointApp_basic
=== CONT  TestAccAWSPinpointApp_QuietTime
=== CONT  TestAccAWSPinpointApp_Tags
=== CONT  TestAccAWSPinpointApp_Limits
=== CONT  TestAccAWSPinpointApp_CampaignHookLambda
--- SKIP: TestAccAWSPinpointApp_basic (1.94s)
    provider_test.go:349: skipping tests; region us-east-2 does not support pinpoint service
--- SKIP: TestAccAWSPinpointApp_Limits (1.94s)
    provider_test.go:349: skipping tests; region us-east-2 does not support pinpoint service
--- SKIP: TestAccAWSPinpointApp_Tags (1.94s)
    provider_test.go:349: skipping tests; region us-east-2 does not support pinpoint service
--- SKIP: TestAccAWSPinpointApp_CampaignHookLambda (1.94s)
    provider_test.go:349: skipping tests; region us-east-2 does not support pinpoint service
--- SKIP: TestAccAWSPinpointApp_QuietTime (1.96s)
    provider_test.go:349: skipping tests; region us-east-2 does not support pinpoint service
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	1.989s
# Test sweeper:
$ TEST=./aws SWEEP=us-east-1 SWEEPARGS=-sweep-run=aws_pinpoint_app make sweep
WARNING: This will destroy infrastructure. Use only in development accounts.
go test ./aws -v -sweep=us-east-1 -sweep-run=aws_pinpoint_app
2020/01/07 12:25:34 [DEBUG] Running Sweepers for region (us-east-1):
2020/01/07 12:25:34 [DEBUG] Running Sweeper (aws_pinpoint_app) in region (us-east-1)
2020/01/07 12:25:34 [INFO] Building AWS auth structure
2020/01/07 12:25:34 [INFO] Setting AWS metadata API timeout to 100ms
2020/01/07 12:25:35 [INFO] Ignoring AWS metadata API endpoint at default location as it doesn't return any instance-id
2020/01/07 12:25:35 [INFO] AWS Auth provider used: "EnvProvider"
2020/01/07 12:25:35 [DEBUG] Trying to get account information via sts:GetCallerIdentity
2020/01/07 12:25:36 [DEBUG] Trying to get account information via sts:GetCallerIdentity
2020/01/07 12:25:37 [INFO] Deleting Pinpoint app terraform-test-pinpointapp-quiet
2020/01/07 12:25:37 [INFO] Deleting Pinpoint app tf-k9xafc8yk1
2020/01/07 12:25:37 [INFO] Deleting Pinpoint app terraform-20200107154813751800000001
2020/01/07 12:25:37 Sweeper Tests ran successfully:
	- aws_pinpoint_app
ok  	github.com/terraform-providers/terraform-provider-aws/aws	3.526s
# S3 bucket acceptance that was edited:
$ make testacc TEST=./aws/ TESTARGS='-run=TestAccAWSS3Bucket_acceleration'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws/ -v -count 1 -parallel 20 -run=TestAccAWSS3Bucket_acceleration -timeout 120m
=== RUN   TestAccAWSS3Bucket_acceleration
=== PAUSE TestAccAWSS3Bucket_acceleration
=== CONT  TestAccAWSS3Bucket_acceleration
--- PASS: TestAccAWSS3Bucket_acceleration (74.88s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	74.909s

ewbankkit · 2019-12-19T19:19:01Z

Waiting for merge of #11352 to add the custom transformation for Pinpoint TagResource() method.

ewbankkit · 2020-01-07T19:02:05Z

aws/resource_aws_pinpoint_app.go

@@ -87,20 +88,24 @@ func resourceAwsPinpointApp() *schema.Resource {
 				Elem: &schema.Resource{
 					Schema: map[string]*schema.Schema{
 						"daily": {
-							Type:     schema.TypeInt,
-							Optional: true,
+							Type:         schema.TypeInt,


Add validations as described here for CampaignLimits.

ewbankkit · 2020-01-07T19:03:46Z

aws/resource_aws_pinpoint_app_test.go

@@ -43,23 +90,18 @@ func TestAccAWSPinpointApp_basic(t *testing.T) {
 }

 func TestAccAWSPinpointApp_CampaignHookLambda(t *testing.T) {
-	oldDefaultRegion := os.Getenv("AWS_DEFAULT_REGION")


Pinpoint is now available in regions other than us-east-1.

ewbankkit · 2020-01-07T19:04:49Z

aws/resource_aws_pinpoint_app_test.go

  handler       = "lambdapinpoint.handler"
-  runtime       = "nodejs8.10"
+  runtime       = "nodejs12.x"


Upgrade Nodejs runtime version - #11514.

ewbankkit · 2020-01-07T19:06:46Z

aws/resource_aws_pinpoint_app_test.go

@@ -267,76 +293,69 @@ EOF
 }

 data "aws_caller_identity" "aws" {}
+data "aws_region" "current" {}


Region agnostic - #8316.

ewbankkit · 2020-01-07T19:10:52Z

aws/resource_aws_pinpoint_app_test.go

+    daily               = 3
+    maximum_duration    = 600
+    messages_per_second = 50
+    total               = 100


Change total to 100 else we get an error like

--- FAIL: TestAccAWSPinpointApp_Limits (10.75s) testing.go:640: Step 0 error: errors during apply: Error: BadRequestException: Campaign Total limit needs to be less than or equal to 100. Value entered: 500 status code: 400, request id: 8ce2c134-a525-45cd-b9b4-f5c54f589e5a on /var/folders/4j/lf5jdq5j4y91g80ytf6qzjwmk6drp5/T/tf-test284836860/main.tf line 6: (source code not available)

ewbankkit · 2020-01-07T21:17:01Z

aws/provider_test.go

@@ -330,10 +330,23 @@ func testAccEC2VPCOnlyPreCheck(t *testing.T) {
 	}
 }

-func testAccHasServicePreCheck(service string, t *testing.T) {
+func testAccPartitionHasServicePreCheck(serviceId string, t *testing.T) {


Rename to better explain purpose.

ewbankkit · 2020-01-07T21:17:25Z

aws/provider_test.go

+	}
+}
+
+func testAccRegionHasServicePreCheck(serviceId string, t *testing.T) {


Checks that the acceptance test regions supports the specified service.

This addition is okay for this and other particular cases, but experience over time has shown the AWS Go SDK endpoints information continually being out of date for some services, which is why we do not use it in many places. We'll always want to prefer behavioral checks (attempting to call an API) over using the endpoint information. 👍 I will add a comment to this function on merge to note that preference.

ewbankkit · 2020-01-07T21:17:54Z

aws/resource_aws_pinpoint_app_test.go


  campaign_hook {
    lambda_function_name = "${aws_lambda_function.test.arn}"
    mode                 = "DELIVERY"
  }
+
+  depends_on = ["aws_lambda_permission.test"]


Prevents random test failures.

❤️ Thanks!

ewbankkit · 2020-01-07T21:18:41Z

Removing WIP.
Ready for review.

ewbankkit · 2020-01-10T12:17:13Z

aws/resource_aws_pinpoint_app_test.go


 	resource.ParallelTest(t, resource.TestCase{
-		PreCheck:      func() { testAccPreCheck(t) },
+		PreCheck:      func() { testAccPreCheck(t); testAccRegionHasServicePreCheck("pinpoint", t) },


Need to explicitly pre-check that the test region has the Pinpoint service instead of just failing the test as the Pinpoint APIs hang in unsupported regions rather than return errors.

bflad

LGTM, thanks @ewbankkit 🚀

Output from acceptance testing:

--- PASS: TestAccAWSPinpointApp_basic (15.45s)
--- PASS: TestAccAWSPinpointApp_CampaignHookLambda (47.58s)
--- PASS: TestAccAWSPinpointApp_Limits (22.38s)
--- PASS: TestAccAWSPinpointApp_QuietTime (19.65s)
--- PASS: TestAccAWSPinpointApp_Tags (47.07s)

--- PASS: TestAccAWSS3Bucket_acceleration (38.79s)

bflad · 2020-01-10T15:20:01Z

aws/provider_test.go

+	}
+}
+
+func testAccRegionHasServicePreCheck(serviceId string, t *testing.T) {


This addition is okay for this and other particular cases, but experience over time has shown the AWS Go SDK endpoints information continually being out of date for some services, which is why we do not use it in many places. We'll always want to prefer behavioral checks (attempting to call an API) over using the endpoint information. 👍 I will add a comment to this function on merge to note that preference.

bflad · 2020-01-10T15:20:50Z

aws/resource_aws_pinpoint_app.go

@@ -87,20 +88,24 @@ func resourceAwsPinpointApp() *schema.Resource {
 				Elem: &schema.Resource{
 					Schema: map[string]*schema.Schema{
 						"daily": {
-							Type:     schema.TypeInt,
-							Optional: true,
+							Type:         schema.TypeInt,


bflad · 2020-01-10T15:23:02Z

aws/resource_aws_pinpoint_app_test.go


  campaign_hook {
    lambda_function_name = "${aws_lambda_function.test.arn}"
    mode                 = "DELIVERY"
  }
+
+  depends_on = ["aws_lambda_permission.test"]


❤️ Thanks!

ghost · 2020-01-17T05:05:16Z

This has been released in version 2.45.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 · 2020-03-27T17:42:23Z

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!

ewbankkit requested a review from a team December 19, 2019 15:39

ewbankkit mentioned this pull request Dec 19, 2019

Refactor Kinesis Streams resource and data source to use keyvaluetags package #11352

Merged

ewbankkit mentioned this pull request Jan 7, 2020

Cannot create Lambda functions with Node.js 8.10 runtime #11514

Closed

ewbankkit force-pushed the pinpoint-keyvaluetags branch from 01e3e5e to 7175be4 Compare January 7, 2020 18:58

ewbankkit commented Jan 7, 2020

View reviewed changes

ewbankkit mentioned this pull request Jan 7, 2020

Tech Debt: Replace Hardcoded Testing us-east-1 Provider Configurations With Aliased Provider Configuration #8316

Closed

ewbankkit commented Jan 7, 2020

View reviewed changes

This comment has been minimized.

Sign in to view

ewbankkit commented Jan 7, 2020

View reviewed changes

ewbankkit force-pushed the pinpoint-keyvaluetags branch from 7175be4 to 615588f Compare January 7, 2020 19:39

ghost added the service/s3 Issues and PRs that pertain to the s3 service. label Jan 7, 2020

ewbankkit commented Jan 7, 2020

View reviewed changes

ewbankkit changed the title ~~[WIP] Refactor Pinpoint application resource to use keyvaluetags package~~ Refactor Pinpoint application resource to use keyvaluetags package Jan 7, 2020

Kit Ewbank added 4 commits January 8, 2020 07:15

Add tag listing and updating for Pinpoint to keyvaluetags.

641a495

r/aws_pinpoint_app: Refactor tagging logic to keyvaluetags package.

ac75ba0

Add ServiceTagInputCustomValue for pinpoint.

b1142a8

r/aws_pinpoint_app: Fix acceptance tests.

1194412

ewbankkit force-pushed the pinpoint-keyvaluetags branch from 615588f to 1194412 Compare January 8, 2020 12:22

ewbankkit commented Jan 10, 2020

View reviewed changes

bflad approved these changes Jan 10, 2020

View reviewed changes

bflad merged commit bba2d08 into hashicorp:master Jan 10, 2020

bflad added this to the v2.45.0 milestone Jan 10, 2020

bflad added technical-debt Addresses areas of the codebase that need refactoring or redesign. enhancement Requests to existing resources that expand the functionality or scope. and removed needs-triage Waiting for first response or review from a maintainer. labels Jan 10, 2020

bflad added a commit that referenced this pull request Jan 10, 2020

Update CHANGELOG for #11368

5872456

ewbankkit deleted the pinpoint-keyvaluetags branch January 10, 2020 19:28

ghost locked and limited conversation to collaborators Mar 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Pinpoint application resource to use keyvaluetags package #11368

Refactor Pinpoint application resource to use keyvaluetags package #11368

ewbankkit commented Dec 19, 2019 •

edited

ewbankkit commented Dec 19, 2019

ewbankkit Jan 7, 2020

bflad Jan 10, 2020

ewbankkit Jan 7, 2020

ewbankkit Jan 7, 2020

ewbankkit Jan 7, 2020

This comment has been minimized.

ewbankkit Jan 7, 2020

ewbankkit Jan 7, 2020

ewbankkit Jan 7, 2020

bflad Jan 10, 2020

ewbankkit Jan 7, 2020

bflad Jan 10, 2020

ewbankkit commented Jan 7, 2020

ewbankkit Jan 10, 2020

bflad left a comment

bflad Jan 10, 2020

bflad Jan 10, 2020

bflad Jan 10, 2020

ghost commented Jan 17, 2020

ghost commented Mar 27, 2020

Refactor Pinpoint application resource to use keyvaluetags package #11368

Refactor Pinpoint application resource to use keyvaluetags package #11368

Conversation

ewbankkit commented Dec 19, 2019 • edited

Community Note

ewbankkit commented Dec 19, 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

This comment has been minimized.

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

ewbankkit commented Jan 7, 2020

Choose a reason for hiding this comment

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

ghost commented Jan 17, 2020

ghost commented Mar 27, 2020

ewbankkit commented Dec 19, 2019 •

edited