-
Notifications
You must be signed in to change notification settings - Fork 9.2k
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
Feature/add enabled cloudwatch logs exports param for DB Instances #4111
Feature/add enabled cloudwatch logs exports param for DB Instances #4111
Conversation
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.
Hi @kterada0509 thanks for this contribution! 🚀 This PR is almost there, but to save trouble I am going to fix up the d.Set()
issue in a followup commit so we can get this merged.
Tests failed: 1 (1 new), passed: 19
=== RUN TestAccAWSDBInstance_ec2Classic
--- PASS: TestAccAWSDBInstance_ec2Classic (423.78s)
=== RUN TestAccAWSDBInstance_iamAuth
--- PASS: TestAccAWSDBInstance_iamAuth (473.58s)
=== RUN TestAccAWSDBInstance_MinorVersion
--- PASS: TestAccAWSDBInstance_MinorVersion (473.68s)
=== RUN TestAccAWSDBInstance_diffSuppressInitialState
--- PASS: TestAccAWSDBInstance_diffSuppressInitialState (473.67s)
=== RUN TestAccAWSDBInstance_basic
--- PASS: TestAccAWSDBInstance_basic (474.85s)
=== RUN TestAccAWSDBInstance_generatedName
--- PASS: TestAccAWSDBInstance_generatedName (474.80s)
=== RUN TestAccAWSDBInstance_namePrefix
--- PASS: TestAccAWSDBInstance_namePrefix (483.84s)
=== RUN TestAccAWSDBInstance_kmsKey
--- PASS: TestAccAWSDBInstance_kmsKey (492.74s)
=== RUN TestAccAWSDBInstance_cloudwatchLogsExportConfiguration
--- PASS: TestAccAWSDBInstance_cloudwatchLogsExportConfiguration (497.76s)
=== RUN TestAccAWSDBInstance_importBasic
--- FAIL: TestAccAWSDBInstance_importBasic (535.11s)
=== RUN TestAccAWSDBInstance_optionGroup
--- PASS: TestAccAWSDBInstance_optionGroup (554.43s)
=== RUN TestAccAWSDBInstance_cloudwatchLogsExportConfigurationUpdate
--- PASS: TestAccAWSDBInstance_cloudwatchLogsExportConfigurationUpdate (556.54s)
=== RUN TestAccAWSDBInstance_portUpdate
--- PASS: TestAccAWSDBInstance_portUpdate (559.06s)
=== RUN TestAccAWSDBInstance_separate_iops_update
--- PASS: TestAccAWSDBInstance_separate_iops_update (619.21s)
=== RUN TestAccAWSDBInstance_noSnapshot
--- PASS: TestAccAWSDBInstance_noSnapshot (745.13s)
=== RUN TestAccAWSDBInstance_snapshot
--- PASS: TestAccAWSDBInstance_snapshot (756.37s)
=== RUN TestAccAWSDBInstance_enhancedMonitoring
--- PASS: TestAccAWSDBInstance_enhancedMonitoring (769.68s)
=== RUN TestAccAWSDBInstance_subnetGroup
--- PASS: TestAccAWSDBInstance_subnetGroup (888.40s)
=== RUN TestAccAWSDBInstance_replica
--- PASS: TestAccAWSDBInstance_replica (1535.38s)
=== RUN TestAccAWSDBInstance_MSSQL_TZ
--- PASS: TestAccAWSDBInstance_MSSQL_TZ (1627.67s)
@@ -351,6 +352,21 @@ func resourceAwsDbInstance() *schema.Resource { | |||
Computed: true, | |||
}, | |||
|
|||
"enabled_cloudwatch_logs_exports": { | |||
Type: schema.TypeList, | |||
Computed: 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.
Minor nitpick: Computed: false
is the default 👍
@@ -1151,10 +1189,55 @@ func resourceAwsDbInstanceStateRefreshFunc(id string, conn *rds.RDS) resource.St | |||
} | |||
} | |||
|
|||
func buildRDSARN(identifier, partition, accountid, region string) (string, 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 likely snuck back in during a rebase. I can remove it on merge 👍
cidr_block = "10.1.0.0/16" | ||
enable_dns_hostnames = true | ||
tags { | ||
Name = "terraform-testacc-db-instance-mssql-timezone" |
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.
Likely copy paste from other tests, will fix on merge. 👍
@@ -775,6 +803,10 @@ func resourceAwsDbInstanceRead(d *schema.ResourceData, meta interface{}) error { | |||
d.Set("monitoring_role_arn", v.MonitoringRoleArn) | |||
} | |||
|
|||
if v.EnabledCloudwatchLogsExports != nil { | |||
d.Set("enabled_cloudwatch_logs_exports", v.EnabledCloudwatchLogsExports) |
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 is silently failing as d.Set()
does not know how to handle a []*string
directly. You can see this from the import test:
--- FAIL: TestAccAWSDBInstance_importBasic (535.11s)
testing.go:518: Step 1 error: ImportStateVerify attributes not equivalent. Difference is shown below. Top is actual, bottom is expected.
(map[string]string) {
}
(map[string]string) (len=3) {
(string) (len=33) "enabled_cloudwatch_logs_exports.#": (string) (len=1) "2",
(string) (len=33) "enabled_cloudwatch_logs_exports.0": (string) (len=5) "audit",
(string) (len=33) "enabled_cloudwatch_logs_exports.1": (string) (len=5) "error"
}
A general recommendation is to always perform error checking when setting non-scalar types. This can be caught adding an import test step when working with new attributes or using TF_SCHEMA_PANIC_ON_ERROR=1
while running the acceptance testing.
This should be something like:
if err := d.Set("enabled_cloudwatch_logs_exports", flattenStringList(v.EnabledCloudwatchLogsExports)); err != nil {
return fmt.Errorf("error setting enabled_cloudwatch_logs_exports: %s", err)
}
I will fix this on merge 👍
This has been released in version 1.15.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! |
Description
This PR implementation a new param
enabled_cloudwatch_logs_exports
toaws_db_instance
for managing Exporting Cloudwatch logs Configuration.Test Results