Implement error_action on aws_iot_topic_rule #11922
Conversation
ghost
added
size/XL
ghost
added
size/XXL
|
I am open to suggestion about how to do this better. Right now this PR has a lot of repeated logic. I think it would be cleaner if the actions where moved from the root of the resource into it's own block like this: |
|
@kurtmc I think that should be the plan long-term (e.g. deprecation of root attributes in 3.x and removal in 4.0.0) 👍 Since I recently reviewed the resource code as part of merging some actions additions, I will provide a review shortly. |
bflad
added
enhancement
There was a problem hiding this comment.
Hey @kurtmc 👋 Thank you for submitting this -- overall this looks to be on the right track and we can get this in with a few adjustments. I think our main change here would be to prefer leaving the schema statically generated for now, which is our preferred pattern for resources in this provider unless absolutely necessary. Thanks!
aws/resource_aws_iot_topic_rule.go
Outdated
| Required: true, | ||
| ValidateFunc: validateIoTTopicRuleCloudWatchAlarmStateValue, | ||
| }, | ||
| actionsSchema := map[string]*schema.Schema{ |
There was a problem hiding this comment.
Since we will very likely want to deprecate the root attributes and since we typically prefer static schema declarations, it would be great if the existing schema was left alone and the new error_action handling was additive (copy-paste is fine) for now. 👍
There was a problem hiding this comment.
@bflad Thanks for the review. One thing I did not know how to do for the error_actions block was to limit it to only 1 action, since the AWS API accepts multiple actions and only a single error action. Can you point me in the direction to add that validation to the schema?
There was a problem hiding this comment.
Absolutely! You can use a combination of a top level attribute of Type: schema.TypeList + MaxItems: 1 then each of the nested attributes can set ExactlyOneOf (this will require re-defining the same attribute list for each attribute since this validation functionality is per-attribute, but we have plans to move this to the resource level at some point), e.g.
// Quick conceptual schema implementation
"error_action": {
Type: schema.TypeList,
Optional: true,
MaxItems: 1,
Elem: &schema.Resource{
Schema: map[string]*schema.Schema{
"cloudwatch_alarm": {
Type: schema.TypeList,
Optional: true,
MaxItems: 1,
Elem: &schema.Resource{ /* ... */ },
ExactlyOneOf: []string{
"error_action.0.cloudwatch_alarm",
"error_action.0.dynamodb",
"error_action.0.dynamodbv2",
/* ... */
},
},
"dynamodb": {
Type: schema.TypeList,
Optional: true,
MaxItems: 1,
Elem: &schema.Resource{ /* ... */ },
ExactlyOneOf: []string{
"error_action.0.cloudwatch_alarm",
"error_action.0.dynamodb",
"error_action.0.dynamodbv2",
/* ... */
},
},
},
},
}As shown above for the new schema it would be nice to have the underlying configuration blocks using TypeList and MaxItems: 1 as well to prevent multiple configuration blocks. Just note that the attribute type assertions via .(*schema.Set).List() will become just .([]interface{}). 😄
aws/resource_aws_iot_topic_rule.go
Outdated
| errorAction := d.Get("error_action").(*schema.Set).List() | ||
| if len(errorAction) > 0 { | ||
| for k, v := range errorAction[0].(map[string]interface{}) { | ||
| if k == "cloudwatch_alarm" { |
There was a problem hiding this comment.
Nit: A switch statement might be clearer here
| @@ -76,6 +88,7 @@ EOF | |||
| * `enabled` - (Required) Specifies whether the rule is enabled. | |||
| * `sql` - (Required) The SQL statement used to query the topic. For more information, see AWS IoT SQL Reference (http://docs.aws.amazon.com/iot/latest/developerguide/iot-rules.html#aws-iot-sql-reference) in the AWS IoT Developer Guide. | |||
| * `sql_version` - (Required) The version of the SQL rules engine to use when evaluating the rule. | |||
| * `error_action` - (Optional) The error action to be associated with the rule. | |||
There was a problem hiding this comment.
Since we are adding a new configuration block, we should include all the details of the nested configuration blocks (or at least reference them). 👍
* `error_action` - (Optional) Configuration block with error action to be associated with the rule. See the documentation for `cloudwatch_alarm`, `dynamodb`, `dynamodbv2`, ....... configuration blocks for further configuration details.
…on associated. Update documentation
ghost
added
size/XL
|
Thanks for the review! I have adjusted my PR based on your comments and re-requested a review. |
ghost
removed
the
waiting-response
There was a problem hiding this comment.
This looks great! Thanks, @kurtmc 🚀
Output from acceptance testing:
--- PASS: TestAccAWSIoTTopicRule_iot_analytics (17.89s)
--- PASS: TestAccAWSIoTTopicRule_republish (19.37s)
--- PASS: TestAccAWSIoTTopicRule_kinesis (20.02s)
--- PASS: TestAccAWSIoTTopicRule_cloudwatchalarm (21.84s)
--- PASS: TestAccAWSIoTTopicRule_iot_events (22.06s)
--- PASS: TestAccAWSIoTTopicRule_sqs (21.99s)
--- PASS: TestAccAWSIoTTopicRule_basic (22.29s)
--- PASS: TestAccAWSIoTTopicRule_dynamoDbv2 (22.30s)
--- PASS: TestAccAWSIoTTopicRule_errorAction (22.98s)
--- PASS: TestAccAWSIoTTopicRule_cloudwatchmetric (22.87s)
--- PASS: TestAccAWSIoTTopicRule_s3 (23.61s)
--- PASS: TestAccAWSIoTTopicRule_elasticsearch (24.26s)
--- PASS: TestAccAWSIoTTopicRule_republish_with_qos (24.70s)
--- PASS: TestAccAWSIoTTopicRule_sns (24.71s)
--- PASS: TestAccAWSIoTTopicRule_lambda (24.91s)
--- PASS: TestAccAWSIoTTopicRule_firehose (24.83s)
--- PASS: TestAccAWSIoTTopicRule_step_functions (24.81s)
--- PASS: TestAccAWSIoTTopicRule_firehose_separator (29.99s)
--- PASS: TestAccAWSIoTTopicRule_Tags (39.72s)
--- PASS: TestAccAWSIoTTopicRule_dynamodb (41.09s)
|
This has been released in version 2.68.0 of the Terraform AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template for triage. Thanks! |
|
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! |
ghost
locked and limited conversation to collaborators
Community Note
Relates OR Closes #7147
Release note for CHANGELOG:
Output from acceptance testing: