-
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
Add logging options to AWS MQ #6122
Conversation
a19085e
to
2c34c20
Compare
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.
Nit on the exposed HCL. My non-maintainer opinion can be taken with a grain of salt. 😄
Hi @elbuo8 👋 Are you still planning on changing the implementation to match the API? Here's some example code that might help: Schema attribute declaration (swap "logs": {
Type: schema.TypeList,
Optional: true,
MaxItems: 1,
// Ignore missing configuration block
DiffSuppressFunc: func(k, old, new string, d *schema.ResourceData) bool {
if old == "1" && new == "0" {
return true
}
return false
},
Elem: &schema.Resource{
Schema: map[string]*schema.Schema{
"audit": {
Type: schema.TypeBool,
Optional: true,
},
"general": {
Type: schema.TypeBool,
Optional: true,
},
},
}, Creating/updating: func expandMqLogs(l []interface{}) *mq.Logs {
if len(l) == 0 || l[0] == nil {
return nil
}
m := l[0].(map[string]interface{})
logs := &mq.Logs{
Audit: aws.Bool(m["audit"].(bool)),
General: aws.Bool(m["general"].(bool)),
}
return logs
}
// In Create func
Logs: expandMqLogs(d.Get("logs").([]interface{})) Reading: func flattenMqLogs(logs *mq.Logs) []interface{} {
if logs == nil {
return []interface{}{}
}
m := map[string]interface{}{
"audit": aws.BoolValue(logs.Audit),
"general": aws.BoolValue(logs.General),
}
return []interface{}{m}
}
// In Read func
if err := d.Set("logs", flattenMqLogs(out.Logs)); err != nil {
return fmt.Errorf("error setting logs: %s", err)
} Testing: resource.TestCheckResourceAttr(resourceName, "logs.#", "1"),
resource.TestCheckResourceAttr(resourceName, "logs.audit", "false"),
resource.TestCheckResourceAttr(resourceName, "logs.general", "false"), |
2c34c20
to
c93b476
Compare
c93b476
to
b3740a8
Compare
@bflad 🙇 updated with your changes. |
b3740a8
to
03ea52a
Compare
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.
Thanks so much for this, @elbuo8! LGTM with a few minor fixes, which I will put in a commit after yours. 🚀
--- PASS: TestAccAWSMqBroker_basic (1079.21s)
--- PASS: TestAccDataSourceAWSMqBroker_basic (1462.77s)
--- PASS: TestAccAWSMqBroker_updateUsers (1543.49s)
--- PASS: TestAccAWSMqBroker_allFieldsDefaultVpc (2039.54s)
--- PASS: TestAccAWSMqBroker_allFieldsCustomVpc (2091.40s)
@@ -277,13 +304,19 @@ func resourceAwsMqBrokerRead(d *schema.ResourceData, meta interface{}) error { | |||
d.Set("engine_version", out.EngineVersion) | |||
d.Set("host_instance_type", out.HostInstanceType) | |||
d.Set("publicly_accessible", out.PubliclyAccessible) | |||
d.Set("enable_logging", out.Logs.General) |
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.
These can be removed 😉
@@ -328,6 +361,16 @@ func resourceAwsMqBrokerUpdate(d *schema.ResourceData, meta interface{}) error { | |||
} | |||
} | |||
|
|||
if d.HasChange("logs") { | |||
_, err := conn.UpdateBroker(&mq.UpdateBrokerRequest{ |
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.
If there are multiple updates that call the same update API call, we generally combine the requests 👍
@@ -62,6 +62,7 @@ The following arguments are supported: | |||
* `security_groups` - (Required) The list of security group IDs assigned to the broker. | |||
* `subnet_ids` - (Optional) The list of subnet IDs in which to launch the broker. A `SINGLE_INSTANCE` deployment requires one subnet. An `ACTIVE_STANDBY_MULTI_AZ` deployment requires two subnets. | |||
* `maintenance_window_start_time` - (Optional) Maintenance window start time. See below. | |||
* `logging` - (Optional) Logging configuration of the broker. See below. |
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 was named logs
in the last code update. 😄
``` --- PASS: TestAccAWSMqBroker_basic (1079.21s) --- PASS: TestAccDataSourceAWSMqBroker_basic (1462.77s) --- PASS: TestAccAWSMqBroker_updateUsers (1543.49s) --- PASS: TestAccAWSMqBroker_allFieldsDefaultVpc (2039.54s) --- PASS: TestAccAWSMqBroker_allFieldsCustomVpc (2091.40s) ```
This has been released in version 1.43.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! |
Changes proposed in this pull request:
enable_logging
flag toaws_mq_broker
resource.enable_auditing
flag toaws_mq_broker
resource.instances
to theaws_mq_broker
data resource.Output from acceptance testing: