Add 'Seperator' property to AWS IoT Firehose Rule #5734

bdoepf · 2018-08-30T13:42:47Z

Changes proposed in this pull request:

Add 'separator' property to AWS IoT Core Firehose topic rule action.

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAccAWSIoTTopicRule_firehose'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -run=TestAccAWSIoTTopicRule_firehose -timeout 120m
?   	github.com/terraform-providers/terraform-provider-aws	[no test files]
=== RUN   TestAccAWSIoTTopicRule_firehose
--- PASS: TestAccAWSIoTTopicRule_firehose (29.68s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	29.698s

bflad

Thanks for submitting this @bdoepf! 🚀 This was off to a good start and just needed some extra bits to finish up appropriately (since dealing with whitespace characters is a pain 😄 ), which I took care of in a followup commit so we can get this released today.

While verifying this change, I found it would generate a panic for any configuration that was missing the new separator argument by removing the separator argument from the existing test configuration. This is fixed now.

So looks good now (with new testing):

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSIoTTopicRule_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSIoTTopicRule_ -timeout 120m
=== RUN   TestAccAWSIoTTopicRule_importbasic
--- PASS: TestAccAWSIoTTopicRule_importbasic (10.57s)
=== RUN   TestAccAWSIoTTopicRule_basic
--- PASS: TestAccAWSIoTTopicRule_basic (8.47s)
=== RUN   TestAccAWSIoTTopicRule_cloudwatchalarm
--- PASS: TestAccAWSIoTTopicRule_cloudwatchalarm (9.02s)
=== RUN   TestAccAWSIoTTopicRule_cloudwatchmetric
--- PASS: TestAccAWSIoTTopicRule_cloudwatchmetric (9.31s)
=== RUN   TestAccAWSIoTTopicRule_elasticsearch
--- PASS: TestAccAWSIoTTopicRule_elasticsearch (9.43s)
=== RUN   TestAccAWSIoTTopicRule_firehose
--- PASS: TestAccAWSIoTTopicRule_firehose (9.35s)
=== RUN   TestAccAWSIoTTopicRule_firehose_separator
--- PASS: TestAccAWSIoTTopicRule_firehose_separator (14.01s)
=== RUN   TestAccAWSIoTTopicRule_kinesis
--- PASS: TestAccAWSIoTTopicRule_kinesis (9.69s)
=== RUN   TestAccAWSIoTTopicRule_lambda
--- PASS: TestAccAWSIoTTopicRule_lambda (8.47s)
=== RUN   TestAccAWSIoTTopicRule_republish
--- PASS: TestAccAWSIoTTopicRule_republish (8.10s)
=== RUN   TestAccAWSIoTTopicRule_s3
--- PASS: TestAccAWSIoTTopicRule_s3 (9.82s)
=== RUN   TestAccAWSIoTTopicRule_sns
--- PASS: TestAccAWSIoTTopicRule_sns (8.18s)
=== RUN   TestAccAWSIoTTopicRule_sqs
--- PASS: TestAccAWSIoTTopicRule_sqs (8.20s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	123.374s

bflad · 2018-09-12T18:33:53Z

aws/validators.go

+		return nil, nil
+	}
+
+	return nil, []error{fmt.Errorf("Separator must be one of ',' (comma), '\t' (tab) '\n' (newline) or '\r\n' (Windows newline)")}


When printing this error, it is returning the whitespace and not the intended characters:

aws_iot_topic_rule.rule: Separator must be one of ',' (comma), ' ' (tab) ' ' (newline) or ' ' (Windows newline)

They need to be escaped appropriately. 👍

bflad · 2018-09-12T18:37:01Z

aws/structure.go

@@ -2754,6 +2754,7 @@ func flattenIoTRuleFirehoseActions(actions []*iot.Action) []map[string]interface
 		if v != nil {
 			result["role_arn"] = *v.RoleArn
 			result["delivery_stream_name"] = *v.DeliveryStreamName
+			result["separator"] = *v.Separator


The API will omit this value in the response if parameter is not provided in the configuration and cause a panic:

panic: runtime error: invalid memory address or nil pointer dereference [signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x2e9cb09] goroutine 406 [running]: github.com/terraform-providers/terraform-provider-aws/aws.flattenIoTRuleFirehoseActions(0xc0000c9058, 0x1, 0x1, 0x379df40, 0xc000669520, 0x0) /Users/bflad/go/src/github.com/terraform-providers/terraform-provider-aws/aws/structure.go:2815 +0x1e9

We should prefer to use the AWS SDK provided functions to dereference pointers instead, e.g. result["separator"] = aws.StringValue(v.Separator)

* Prevent panic with omitted firehose separator in API response * Escape whitespace characters in firehose separator ValidateFunc error message * Add separate test for firehose separator ``` $ make testacc TEST=./aws TESTARGS='-run=TestAccAWSIoTTopicRule_' ==> Checking that code complies with gofmt requirements... TF_ACC=1 go test ./aws -v -run=TestAccAWSIoTTopicRule_ -timeout 120m === RUN TestAccAWSIoTTopicRule_importbasic --- PASS: TestAccAWSIoTTopicRule_importbasic (10.57s) === RUN TestAccAWSIoTTopicRule_basic --- PASS: TestAccAWSIoTTopicRule_basic (8.47s) === RUN TestAccAWSIoTTopicRule_cloudwatchalarm --- PASS: TestAccAWSIoTTopicRule_cloudwatchalarm (9.02s) === RUN TestAccAWSIoTTopicRule_cloudwatchmetric --- PASS: TestAccAWSIoTTopicRule_cloudwatchmetric (9.31s) === RUN TestAccAWSIoTTopicRule_elasticsearch --- PASS: TestAccAWSIoTTopicRule_elasticsearch (9.43s) === RUN TestAccAWSIoTTopicRule_firehose --- PASS: TestAccAWSIoTTopicRule_firehose (9.35s) === RUN TestAccAWSIoTTopicRule_firehose_separator --- PASS: TestAccAWSIoTTopicRule_firehose_separator (14.01s) === RUN TestAccAWSIoTTopicRule_kinesis --- PASS: TestAccAWSIoTTopicRule_kinesis (9.69s) === RUN TestAccAWSIoTTopicRule_lambda --- PASS: TestAccAWSIoTTopicRule_lambda (8.47s) === RUN TestAccAWSIoTTopicRule_republish --- PASS: TestAccAWSIoTTopicRule_republish (8.10s) === RUN TestAccAWSIoTTopicRule_s3 --- PASS: TestAccAWSIoTTopicRule_s3 (9.82s) === RUN TestAccAWSIoTTopicRule_sns --- PASS: TestAccAWSIoTTopicRule_sns (8.18s) === RUN TestAccAWSIoTTopicRule_sqs --- PASS: TestAccAWSIoTTopicRule_sqs (8.20s) PASS ok github.com/terraform-providers/terraform-provider-aws/aws 123.374s ```

bflad · 2018-09-13T02:52:18Z

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

bdoepf · 2018-09-15T08:24:33Z

@bflad Thanks for your detailed review and fixing the open issues 👍 :-)

ghost · 2020-04-03T17:35:33Z

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!

Add 'Seperator' property to AWS IoT Firehose Rule

d0464c9

bflad added enhancement Requests to existing resources that expand the functionality or scope. service/iot Issues and PRs that pertain to the iot service. labels Aug 30, 2018

bflad approved these changes Sep 12, 2018

View reviewed changes

bflad added this to the v1.36.0 milestone Sep 12, 2018

bflad merged commit d0464c9 into hashicorp:master Sep 12, 2018

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

Update CHANGELOG for #5734

13b35cb

ghost locked and limited conversation to collaborators Apr 3, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add 'Seperator' property to AWS IoT Firehose Rule #5734

Add 'Seperator' property to AWS IoT Firehose Rule #5734

bdoepf commented Aug 30, 2018

bflad left a comment

bflad Sep 12, 2018

bflad Sep 12, 2018

bflad commented Sep 13, 2018

bdoepf commented Sep 15, 2018

ghost commented Apr 3, 2020

Add 'Seperator' property to AWS IoT Firehose Rule #5734

Add 'Seperator' property to AWS IoT Firehose Rule #5734

Conversation

bdoepf commented Aug 30, 2018

bflad left a comment

Choose a reason for hiding this comment

bflad Sep 12, 2018

Choose a reason for hiding this comment

bflad Sep 12, 2018

Choose a reason for hiding this comment

bflad commented Sep 13, 2018

bdoepf commented Sep 15, 2018

ghost commented Apr 3, 2020