resource/aws_kinesis_firehose_delivery_stream: Support data_format_conversion_configuration #4842

bflad · 2018-06-15T03:25:54Z

Changes proposed in this pull request:

Add extended S3 destination data_format_conversion_configuration and sub-arguments

References:

Output from acceptance testing:

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSKinesisFirehoseDeliveryStream_ExtendedS3_DataFormatConversionConfiguration'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSKinesisFirehoseDeliveryStream_ExtendedS3_DataFormatConversionConfiguration -timeout 120m
=== RUN   TestAccAWSKinesisFirehoseDeliveryStream_ExtendedS3_DataFormatConversionConfiguration_Enabled
--- PASS: TestAccAWSKinesisFirehoseDeliveryStream_ExtendedS3_DataFormatConversionConfiguration_Enabled (136.24s)
=== RUN   TestAccAWSKinesisFirehoseDeliveryStream_ExtendedS3_DataFormatConversionConfiguration_HiveJsonSerDe_Empty
--- PASS: TestAccAWSKinesisFirehoseDeliveryStream_ExtendedS3_DataFormatConversionConfiguration_HiveJsonSerDe_Empty (122.24s)
=== RUN   TestAccAWSKinesisFirehoseDeliveryStream_ExtendedS3_DataFormatConversionConfiguration_OpenXJsonSerDe_Empty
--- PASS: TestAccAWSKinesisFirehoseDeliveryStream_ExtendedS3_DataFormatConversionConfiguration_OpenXJsonSerDe_Empty (111.75s)
=== RUN   TestAccAWSKinesisFirehoseDeliveryStream_ExtendedS3_DataFormatConversionConfiguration_OrcSerDe_Empty
--- PASS: TestAccAWSKinesisFirehoseDeliveryStream_ExtendedS3_DataFormatConversionConfiguration_OrcSerDe_Empty (125.72s)
=== RUN   TestAccAWSKinesisFirehoseDeliveryStream_ExtendedS3_DataFormatConversionConfiguration_ParquetSerDe_Empty
--- PASS: TestAccAWSKinesisFirehoseDeliveryStream_ExtendedS3_DataFormatConversionConfiguration_ParquetSerDe_Empty (122.14s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	618.134s

…nversion_configuration

… and remove extraneous schema comments

… from API defaults for data_format_conversion_configuration

mbfrahry

This is looking good. I have a few questions about checking for Optional values, setting multiple serializers in a configuration, swapping between serializers (whether or not that is possible), and updating different attributes within a serializer. Also there are a ton of attributes that should be constants. I'm wondering if we could petition to have those available in the sdk instead of manually setting them here

mbfrahry · 2018-06-15T18:40:39Z

aws/resource_aws_kinesis_firehose_delivery_stream.go

+	// API omits default values
+	// Return defaults that are not type zero values to prevent extraneous difference
+
+	m["block_size_bytes"] = 268435456


There are a lot of values that could be better as constants in this PR. Do you think the aws sdk team would be down to put all of these defaults as constants in their sdk?

Unfortunately the AWS SDK constants do not usually contain integer values and especially not bytes amounts. I can try submitting an upstream issue into https://github.com/aws/aws-sdk-go/ but it would be dependent on all the (distributed) AWS service teams implementing those constants into their JSON service models.

In this scenario, I'd almost wish for something more like 256 * XXX.Mebibytes, similar to 1 * time.Minute, but I'm not sure its worth vendoring in an external dependency for that simplification.

mbfrahry · 2018-06-15T18:48:17Z

aws/resource_aws_kinesis_firehose_delivery_stream.go

+													MaxItems: 1,
+													Elem: &schema.Resource{
+														Schema: map[string]*schema.Schema{
+															"hive_json_ser_de": {


What happens when you set both hive_json_ser_de and open_x_json_ser_de

It returns an error. 😄 I added ConflictsWith to both attributes here.

mbfrahry · 2018-06-15T18:49:09Z

aws/resource_aws_kinesis_firehose_delivery_stream.go

+													MaxItems: 1,
+													Elem: &schema.Resource{
+														Schema: map[string]*schema.Schema{
+															"orc_ser_de": {


Same here. What happens when you set multiple ..._ser_de

It returns an error. 😄 I added ConflictsWith to both attributes here.

mbfrahry · 2018-06-15T18:54:28Z

aws/resource_aws_kinesis_firehose_delivery_stream_test.go

@@ -147,6 +147,168 @@ func TestAccAWSKinesisFirehoseDeliveryStream_ExtendedS3basic(t *testing.T) {
 	})
 }

+func TestAccAWSKinesisFirehoseDeliveryStream_ExtendedS3_DataFormatConversionConfiguration_Enabled(t *testing.T) {


This test updates disabled -> enabled but I'd love to see some other tests that update the different configurations and maybe swaps from one serializer to another if that is even an option. Hopefully that doesn't take too much work but I think it's necessary

I certainly agree. Let me write up tests that switch up the serializers and deserializers via update 👍

Added two tests for updating deserializer and serializer:

make testacc TEST=./aws TESTARGS='-run=TestAccAWSKinesisFirehoseDeliveryStream_ExtendedS3_DataFormatConversionConfiguration_\(Des\|S\)erializer_Update' ==> Checking that code complies with gofmt requirements... TF_ACC=1 go test ./aws -v -run=TestAccAWSKinesisFirehoseDeliveryStream_ExtendedS3_DataFormatConversionConfiguration_\(Des\|S\)erializer_Update -timeout 120m === RUN TestAccAWSKinesisFirehoseDeliveryStream_ExtendedS3_DataFormatConversionConfiguration_Deserializer_Update --- PASS: TestAccAWSKinesisFirehoseDeliveryStream_ExtendedS3_DataFormatConversionConfiguration_Deserializer_Update (316.20s) === RUN TestAccAWSKinesisFirehoseDeliveryStream_ExtendedS3_DataFormatConversionConfiguration_Serializer_Update --- PASS: TestAccAWSKinesisFirehoseDeliveryStream_ExtendedS3_DataFormatConversionConfiguration_Serializer_Update (284.81s) PASS ok github.com/terraform-providers/terraform-provider-aws/aws 601.050s

mbfrahry · 2018-06-15T18:58:10Z

aws/resource_aws_kinesis_firehose_delivery_stream.go

+	config := &firehose.SchemaConfiguration{
+		DatabaseName: aws.String(m["database_name"].(string)),
+		RoleARN:      aws.String(m["role_arn"].(string)),
+		TableName:    aws.String(m["table_name"].(string)),


This is optional with no default. We should check to make sure it's actually in the config

mbfrahry · 2018-06-15T18:59:59Z

aws/resource_aws_kinesis_firehose_delivery_stream.go

+
+	config := &firehose.SchemaConfiguration{
+		DatabaseName: aws.String(m["database_name"].(string)),
+		RoleARN:      aws.String(m["role_arn"].(string)),


This is optional with no default. We should check to make sure it's actually in the config. It is set in all the tests so is it actually required when you specify schema_configuration

mbfrahry · 2018-06-15T19:01:13Z

aws/resource_aws_kinesis_firehose_delivery_stream.go

+	m := l[0].(map[string]interface{})
+
+	config := &firehose.SchemaConfiguration{
+		DatabaseName: aws.String(m["database_name"].(string)),


This is optional with no default. We should check to make sure it's actually in the config. It is set in all the tests so is it actually required when you specify schema_configuration

This is an odd case where we are following the API definition of required vs optional attributes according to: https://docs.aws.amazon.com/firehose/latest/APIReference/API_SchemaConfiguration.html

In these cases we generally prefer to follow the API versus making our own determination since there might be some implementation that does not require these details within the schema configuration (in this case, one that I personally do not know about). The API and user guide documentation do not provide any guidance why these are not required.

This comment applies to all the below as well.

Followup for anyone looking at these comments, we decided to opt for Required: true on these attributes for now. If there are any use cases for not providing the database/role/table in the schema configuration, hopefully a bug report will suss that out.

…h for serializers/deserializers Previously just an API error: aws_kinesis_firehose_delivery_stream.test: error creating Kinesis Firehose Delivery Stream: InvalidArgumentException: More than one deserializer specified. Only one may be chosen. Now a Terraform validation error: - aws_kinesis_firehose_delivery_stream.test: "extended_s3_configuration.0.data_format_conversion_configuration.0.input_format_configuration.0.deserializer.0.hive_json_ser_de": conflicts with extended_s3_configuration.0.data_format_conversion_configuration.0.input_format_configuration.0.deserializer.0.open_x_json_ser_de - aws_kinesis_firehose_delivery_stream.test: "extended_s3_configuration.0.data_format_conversion_configuration.0.input_format_configuration.0.deserializer.0.open_x_json_ser_de": conflicts with extended_s3_configuration.0.data_format_conversion_configuration.0.input_format_configuration.0.deserializer.0.hive_json_ser_de

…o serializer and deserializer make testacc TEST=./aws TESTARGS='-run=TestAccAWSKinesisFirehoseDeliveryStream_ExtendedS3_DataFormatConversionConfiguration_\(Des\|S\)erializer_Update' ==> Checking that code complies with gofmt requirements... TF_ACC=1 go test ./aws -v -run=TestAccAWSKinesisFirehoseDeliveryStream_ExtendedS3_DataFormatConversionConfiguration_\(Des\|S\)erializer_Update -timeout 120m === RUN TestAccAWSKinesisFirehoseDeliveryStream_ExtendedS3_DataFormatConversionConfiguration_Deserializer_Update --- PASS: TestAccAWSKinesisFirehoseDeliveryStream_ExtendedS3_DataFormatConversionConfiguration_Deserializer_Update (316.20s) === RUN TestAccAWSKinesisFirehoseDeliveryStream_ExtendedS3_DataFormatConversionConfiguration_Serializer_Update --- PASS: TestAccAWSKinesisFirehoseDeliveryStream_ExtendedS3_DataFormatConversionConfiguration_Serializer_Update (284.81s) PASS ok github.com/terraform-providers/terraform-provider-aws/aws 601.050s

mbfrahry

LGTM!

…rsion_configuration schema_configuration database/role/table as required for now

bflad · 2018-06-25T11:47:32Z

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

ghost · 2020-04-05T17:18:03Z

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!

resource/aws_kinesis_firehose_delivery_stream: Support data_format_co…

a6e08dd

…nversion_configuration

bflad added enhancement Requests to existing resources that expand the functionality or scope. service/firehose Issues and PRs that pertain to the firehose service. labels Jun 15, 2018

ghost added the size/XXL Managed by automation to categorize the size of a PR. label Jun 15, 2018

bflad mentioned this pull request Jun 15, 2018

Feature Request: Manage Record Format Conversion In AWS Kinesis Firehose Stream #4510

Closed

bflad requested a review from a team June 15, 2018 03:34

resource/aws_kinesis_firehose_delivery_stream: Fix imports formatting…

13ba7c3

… and remove extraneous schema comments

ghost added the size/XXL Managed by automation to categorize the size of a PR. label Jun 15, 2018

resource/aws_kinesis_firehose_delivery_stream: Remove else statements…

0ce7959

… from API defaults for data_format_conversion_configuration

mbfrahry suggested changes Jun 15, 2018

View reviewed changes

bflad added 2 commits June 15, 2018 15:24

mbfrahry approved these changes Jun 18, 2018

View reviewed changes

resource/aws_kinesis_firehose_delivery_stream: Mark data_format_conve…

697196a

…rsion_configuration schema_configuration database/role/table as required for now

bflad added this to the v1.24.0 milestone Jun 18, 2018

bflad merged commit 1afb788 into master Jun 18, 2018

bflad deleted the f-aws_kinesis_firehose_delivery_stream-data_format_conversion branch June 18, 2018 15:12

bflad added a commit that referenced this pull request Jun 18, 2018

Update CHANGELOG for #4842

2fa1b9f

darrenhaken pushed a commit to darrenhaken/terraform-provider-aws that referenced this pull request Jun 20, 2018

Update CHANGELOG for hashicorp#4842

26fb58c

ghost locked and limited conversation to collaborators Apr 5, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

resource/aws_kinesis_firehose_delivery_stream: Support data_format_conversion_configuration #4842

resource/aws_kinesis_firehose_delivery_stream: Support data_format_conversion_configuration #4842

bflad commented Jun 15, 2018

mbfrahry left a comment

mbfrahry Jun 15, 2018

bflad Jun 18, 2018

mbfrahry Jun 15, 2018

bflad Jun 18, 2018

mbfrahry Jun 15, 2018

bflad Jun 18, 2018

mbfrahry Jun 15, 2018

bflad Jun 18, 2018

bflad Jun 18, 2018

mbfrahry Jun 15, 2018

mbfrahry Jun 15, 2018

mbfrahry Jun 15, 2018

bflad Jun 18, 2018

bflad Jun 18, 2018

mbfrahry left a comment

bflad commented Jun 25, 2018

ghost commented Apr 5, 2020

resource/aws_kinesis_firehose_delivery_stream: Support data_format_conversion_configuration #4842

resource/aws_kinesis_firehose_delivery_stream: Support data_format_conversion_configuration #4842

Conversation

bflad commented Jun 15, 2018

mbfrahry 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

mbfrahry left a comment

Choose a reason for hiding this comment

bflad commented Jun 25, 2018

ghost commented Apr 5, 2020