Add s3_backup_mode option in Firehose Redshift destination #1830

ApsOps · 2017-10-06T13:14:25Z

http://docs.aws.amazon.com/cli/latest/reference/firehose/create-delivery-stream.html

radeksimko

Hi @ApsOps
thanks for the PR.

Do you mind adding an acceptance test or modifying an existing one to make use of this new field?

The test would normally go under https://github.com/terraform-providers/terraform-provider-aws/blob/master/aws/resource_aws_kinesis_firehose_delivery_stream_test.go

Let me know if you need any help in that context.

ApsOps · 2017-10-23T20:18:55Z

@radeksimko Added the acceptance tests. These are passing for me.
Please let me know if I missed something.

Thanks a lot for helping out with this!

ApsOps · 2017-10-24T06:29:00Z

aws/resource_aws_kinesis_firehose_delivery_stream.go

@@ -41,6 +41,59 @@ func cloudWatchLoggingOptionsSchema() *schema.Schema {
 	}
 }

+func S3BackupConfigurationSchema() *schema.Schema {
+	return &schema.Schema{
+		Type:     schema.TypeSet,


@radeksimko I have a doubt here. When do we use TypeSet vs TypeList?
I see S3ConfigurationSchema has TypeList with MaxItems: 1. Is this significant only during type assertion?

Set is used when order doesn't matter, list is used in all other cases and typically with MaxItems: 1 for easier referencing as we can predict the index, i.e. field_name.0.nested_field

In other words it would be better to use TypeList here.

Can't we call this s3ConfigurationSchema and use it in "s3_configuration" too to reduce the repetition a bit?

Can't we call this s3ConfigurationSchema and use it in "s3_configuration" too to reduce the repetition a bit?

That's what I was going for and noticed that the schemaTypes are different.

Set is used when order doesn't matter

@radeksimko Just curious. Why do we use TypeList here then? The order doesn't matter here, right?

Correct, it doesn't, but also there's really nothing to order 😃 as we only ever have a single item in the list and TypeList makes it more convenient for referencing purposes, i.e. field_name.0.nested_field instead of field_name.<computed-index>.nested_field.

radeksimko

Thanks for adding the test (it's also passing for me), this is starting to look pretty good. I left you a few more comments which are mostly nitpicks, the PR looks otherwise good.

Let me know if you need any further clarification for any of my comments.

radeksimko · 2017-10-25T11:39:25Z

aws/resource_aws_kinesis_firehose_delivery_stream.go

@@ -41,6 +41,59 @@ func cloudWatchLoggingOptionsSchema() *schema.Schema {
 	}
 }

+func S3BackupConfigurationSchema() *schema.Schema {
+	return &schema.Schema{
+		Type:     schema.TypeSet,


Set is used when order doesn't matter, list is used in all other cases and typically with MaxItems: 1 for easier referencing as we can predict the index, i.e. field_name.0.nested_field

radeksimko · 2017-10-25T11:41:01Z

aws/resource_aws_kinesis_firehose_delivery_stream.go

@@ -467,6 +536,34 @@ func createS3Config(d *schema.ResourceData) *firehose.S3DestinationConfiguration
 	return configuration
 }

+func createS3BackupConfig(d *schema.ResourceData) *firehose.S3DestinationConfiguration {


Do you mind reducing the scope here? i.e. the function doesn't need access to the whole schema (all fields), all it needs is a single field, so we can pass it as map[string]interface{} I think?

Also it is a convention throughout the codebase to call these functions "expanders" - i.e. expandS3BackupConfig

@radeksimko We're not using "expanders" for createS3Config and createExtendedS3Config - few lines above and below this snippet. Should I change those too?

I didn't notice that previously - good catch - we should certainly fix that, but I'd prefer to do it in a separate PR which may come afterwards to keep the context & LOC low here. 😉

radeksimko · 2017-10-25T12:26:39Z

aws/resource_aws_kinesis_firehose_delivery_stream.go

@@ -41,6 +41,59 @@ func cloudWatchLoggingOptionsSchema() *schema.Schema {
 	}
 }

+func S3BackupConfigurationSchema() *schema.Schema {
+	return &schema.Schema{
+		Type:     schema.TypeSet,


In other words it would be better to use TypeList here.

Can't we call this s3ConfigurationSchema and use it in "s3_configuration" too to reduce the repetition a bit?

radeksimko

👍

ghost · 2020-04-10T17:48:32Z

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!

radeksimko reviewed Oct 9, 2017

View reviewed changes

radeksimko added enhancement Requests to existing resources that expand the functionality or scope. waiting-response Maintainers are waiting on response from community or contributor. labels Oct 9, 2017

ApsOps force-pushed the firehose-redshift-dest-s3-backup-support branch 3 times, most recently from b32ed22 to db51754 Compare October 23, 2017 20:11

ApsOps added 2 commits October 24, 2017 01:46

Add s3_backup_mode option in Firehose Redshift destination

d8c4a87

Update docs for Firehose Redshift destination configuration

dba6601

ApsOps force-pushed the firehose-redshift-dest-s3-backup-support branch from db51754 to dba6601 Compare October 23, 2017 20:17

ApsOps commented Oct 24, 2017

View reviewed changes

radeksimko reviewed Oct 25, 2017

View reviewed changes

ApsOps added 2 commits October 25, 2017 20:31

Changes as per review comments

766a25a

Don't export a private function

7d7c6e6

radeksimko removed the waiting-response Maintainers are waiting on response from community or contributor. label Oct 25, 2017

radeksimko approved these changes Oct 25, 2017

View reviewed changes

radeksimko merged commit 44edced into hashicorp:master Oct 25, 2017

ghost locked and limited conversation to collaborators Apr 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add s3_backup_mode option in Firehose Redshift destination #1830

Add s3_backup_mode option in Firehose Redshift destination #1830

ApsOps commented Oct 6, 2017 •

edited

Loading

radeksimko left a comment

ApsOps commented Oct 23, 2017

ApsOps Oct 24, 2017

radeksimko Oct 25, 2017

radeksimko Oct 25, 2017

ApsOps Oct 25, 2017

ApsOps Oct 25, 2017

radeksimko Oct 25, 2017

radeksimko left a comment

radeksimko Oct 25, 2017

radeksimko Oct 25, 2017

radeksimko Oct 25, 2017

ApsOps Oct 25, 2017 •

edited

Loading

radeksimko Oct 25, 2017

radeksimko Oct 25, 2017

radeksimko left a comment

ghost commented Apr 10, 2020

Add s3_backup_mode option in Firehose Redshift destination #1830

Add s3_backup_mode option in Firehose Redshift destination #1830

Conversation

ApsOps commented Oct 6, 2017 • edited Loading

radeksimko left a comment

Choose a reason for hiding this comment

ApsOps commented Oct 23, 2017

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

radeksimko 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

ApsOps Oct 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

radeksimko left a comment

Choose a reason for hiding this comment

ghost commented Apr 10, 2020

ApsOps commented Oct 6, 2017 •

edited

Loading

ApsOps Oct 25, 2017 •

edited

Loading