-
Notifications
You must be signed in to change notification settings - Fork 9.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added support for Kinesis Extended S3 Configuration Backup Mode #2987
Conversation
Is there anything remaining on this that I need to do to make it to a release? |
|
@bflad @radeksimko Is there anything else I need to do to get this pull request merged? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few minor things I will fix post-merge (trying to get a crash prevention PR in afterwards). Running the acceptance testing now with #3944 merged 👍
if err != nil { | ||
return err | ||
} | ||
d.Set("extended_s3_configuration", extendedS3ConfList) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please keep the err
check when setting complex attributes (e.g. non-string)
"s3_backup_mode": { | ||
Type: schema.TypeString, | ||
Optional: true, | ||
Default: "Disabled", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor nitpick: Prefer usage of the SDK provided constant firehose.S3BackupModeDisabled
Type: schema.TypeString, | ||
Optional: true, | ||
Default: "Disabled", | ||
ValidateFunc: func(v interface{}, k string) (ws []string, errors []error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be simplified with:
ValidateFunc: validation.StringInSlice([]string{
firehose.S3BackupModeDisabled,
firehose.S3BackupModeEnabled,
}, false),
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - thanks! 🚀
14 tests passed (all tests)
=== RUN TestAccAWSKinesisFirehoseDeliveryStream_ExtendedS3InvalidParameterName
--- PASS: TestAccAWSKinesisFirehoseDeliveryStream_ExtendedS3InvalidParameterName (1.90s)
=== RUN TestAccAWSKinesisFirehoseDeliveryStream_ExtendedS3InvalidProcessorType
--- PASS: TestAccAWSKinesisFirehoseDeliveryStream_ExtendedS3InvalidProcessorType (2.00s)
=== RUN TestAccAWSKinesisFirehoseDeliveryStream_s3WithCloudwatchLogging
--- PASS: TestAccAWSKinesisFirehoseDeliveryStream_s3WithCloudwatchLogging (98.14s)
=== RUN TestAccAWSKinesisFirehoseDeliveryStream_s3KinesisStreamSource
--- PASS: TestAccAWSKinesisFirehoseDeliveryStream_s3KinesisStreamSource (99.69s)
=== RUN TestAccAWSKinesisFirehoseDeliveryStream_missingProcessingConfiguration
--- PASS: TestAccAWSKinesisFirehoseDeliveryStream_missingProcessingConfiguration (105.17s)
=== RUN TestAccAWSKinesisFirehoseDeliveryStream_importBasic
--- PASS: TestAccAWSKinesisFirehoseDeliveryStream_importBasic (107.63s)
=== RUN TestAccAWSKinesisFirehoseDeliveryStream_ExtendedS3basic
--- PASS: TestAccAWSKinesisFirehoseDeliveryStream_ExtendedS3basic (108.18s)
=== RUN TestAccAWSKinesisFirehoseDeliveryStream_ExtendedS3Updates
--- PASS: TestAccAWSKinesisFirehoseDeliveryStream_ExtendedS3Updates (113.09s)
=== RUN TestAccAWSKinesisFirehoseDeliveryStream_s3basic
--- PASS: TestAccAWSKinesisFirehoseDeliveryStream_s3basic (119.20s)
=== RUN TestAccAWSKinesisFirehoseDeliveryStream_ExtendedS3KmsKeyArn
--- PASS: TestAccAWSKinesisFirehoseDeliveryStream_ExtendedS3KmsKeyArn (124.86s)
=== RUN TestAccAWSKinesisFirehoseDeliveryStream_SplunkConfigUpdates
--- PASS: TestAccAWSKinesisFirehoseDeliveryStream_SplunkConfigUpdates (143.22s)
=== RUN TestAccAWSKinesisFirehoseDeliveryStream_s3ConfigUpdates
--- PASS: TestAccAWSKinesisFirehoseDeliveryStream_s3ConfigUpdates (183.37s)
=== RUN TestAccAWSKinesisFirehoseDeliveryStream_ElasticsearchConfigUpdates
--- PASS: TestAccAWSKinesisFirehoseDeliveryStream_ElasticsearchConfigUpdates (866.85s)
=== RUN TestAccAWSKinesisFirehoseDeliveryStream_RedshiftConfigUpdates
--- PASS: TestAccAWSKinesisFirehoseDeliveryStream_RedshiftConfigUpdates (1246.56s)
This has been released in version 1.14.0 of the AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. |
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! |
Provides support for
s3_backup_mode
ands3_backup_configuration
options under the extended_s3 destination type. Refer to Issue: #2699. Note: this uses some fixes from #2970 so might require updates before merging.