provider/aws: Manage Triggers for AWS CodeDeploy Event Notifications #5599
Conversation
- Create a Trigger to Send Notifications for AWS CodeDeploy Events - Update aws_codedeploy_deployment_group docs
|
Some additional comments:
For the Using just the |
|
|
||
| func validateTriggerEvent(v interface{}, k string) (ws []string, errors []error) { | ||
| value := v.(string) | ||
| if value != "DeploymentStart" && value != "DeploymentSuccess" && value != "DeploymentFailure" && value != "DeploymentStop" && value != "InstanceStart" && value != "InstanceSuccess" && value != "InstanceFailure" { |
There was a problem hiding this comment.
This feels pretty hacky - this conditional will need to continue to be expanded if newer values are added. A potential pattern to use here may be something like this:
This would mean the associated tests would then be able to be specified as follows:
rather than having 2 ranges within the test func
|
Hi @niclic Thanks so much for the work on this PR - sorry it has taken a while to get back to you. I just had a look at this and ran the tests. I got the following on the test run: I have left another few comments inline Thanks Paul |
stack72
added
the
waiting-response
|
@niclic can't wait to see this get merged in. Thank you for the work on this. 😀 |
|
Thanks for the comments @stack72 and @thegranddesign. I have a few new commits to push, as well as comments on issues I have encountered since revisiting this PR. There are some issues that need to be resolved. |
- also rename TestAccAWSCodeDeployDeploymentGroup_triggerConfiguration test
|
I refactored the |
|
RE: Frustratingly, this error doesn't always happen. The following information leads me to suspect this is some sort of eventual consistency issue.
For example: err = resource.Retry(2*time.Minute, func() *resource.RetryError {
resp, err = conn.CreateDeploymentGroup(&input)
if err != nil {
codedeployErr, ok := err.(awserr.Error)
if !ok {
return resource.NonRetryableError(err)
}
if codedeployErr.Code() == "InvalidRoleException" || codedeployErr.Code() == "InvalidTriggerConfigException" {
log.Printf("[DEBUG] Trying to create deployment group again: %q",
codedeployErr.Message())
return resource.RetryableError(err)
}
return resource.NonRetryableError(err)
}
return nil
})Here is some DEBUG log that show the retries. Here is another example: Note: I've removed the updates to the SNS topics and redacted my AWS ACCOUNT ID. I'm not sure of the best way to handle this issue. The retry logic seems to work as expected. However, there are legitimate |
|
Another issue... Reviewing DEBUG logs for test output, I notice updates are not applied to the I don't believe every set attribute must be included in the hash function, but I may be wrong. For example, compare resourceAwsSecurityGroupRuleHash and resourceAwsDynamoDbTable. But if I do include For example: func resourceAwsCodeDeployTriggerConfigHash(v interface{}) int {
var buf bytes.Buffer
m := v.(map[string]interface{})
buf.WriteString(fmt.Sprintf("%s-", m["trigger_name"].(string)))
buf.WriteString(fmt.Sprintf("%s-", m["trigger_target_arn"].(string)))
if triggerEvents, ok := m["trigger_events"]; ok {
names := triggerEvents.(*schema.Set).List()
strings := make([]string, len(names))
for i, raw := range names {
strings[i] = raw.(string)
}
sort.Strings(strings)
for _, s := range strings {
buf.WriteString(fmt.Sprintf("%s-", s))
}
}
return hashcode.String(buf.String())
}Otherwise, changes to I'd like to improve these tests to assert on the actual changes to resource attributes. I mentioned above that the existing "does the Deployment Group exist?" check does not test all possible attribute changes across CRUD operations and this is an example where better tests are needed. There are several ways to write these tests, but this is one reason I suggested removing the So something is not right with the code as it stands. To reproduce this error, comment out the line in func resourceAwsCodeDeployTriggerConfigHash(v interface{}) int {
var buf bytes.Buffer
m := v.(map[string]interface{})
buf.WriteString(fmt.Sprintf("%s-", m["trigger_name"].(string)))
// buf.WriteString(fmt.Sprintf("%s-", m["trigger_target_arn"].(string))) // comment this line out to cause errors
return hashcode.String(buf.String())
} |
- by using built in resource attribute helpers - these can get quite verbose and repetitive, so passing the resource to a function might be better - can't use these (yet) to assert trigger configuration state
…source schema - buildTriggerConfigs - triggerConfigsToMap
|
HI @niclic thanks so much for the work on this - the latest changes make it work consistently now :) This looks good :) |
stack72
removed
the
waiting-response
|
Just a FYI, to prove no eventual consistency problems, I reran the tests: |
|
Didn't expect this to get merged so quickly! Thanks @stack72 I believe there are still issues to be resolved.
I only illustrated possible code fixes in my comments above, but I didn't commit changes for these issues. It's always possible that commits to master will/did resolve the first issue. The second one bothers me because the Next steps? I will continue to poke around and propose a patch in a new PR. But it could help to have someone else take a look or help me understand the cause of these issues. |
|
I will continue to monitor master for failures in this area - please do continue to look around the code and see what can be changed. I will stay in touch on this Thanks again Paul |
…ashicorp#5599) * provider/aws: CodeDeploy Deployment Group Triggers - Create a Trigger to Send Notifications for AWS CodeDeploy Events - Update aws_codedeploy_deployment_group docs * Refactor validateTriggerEvent function and test - also rename TestAccAWSCodeDeployDeploymentGroup_triggerConfiguration test * Enhance existing Deployment Group integration tests - by using built in resource attribute helpers - these can get quite verbose and repetitive, so passing the resource to a function might be better - can't use these (yet) to assert trigger configuration state * Unit tests for conversions between aws TriggerConfig and terraform resource schema - buildTriggerConfigs - triggerConfigsToMap
|
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 have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
ghost
locked and limited conversation to collaborators
Manage Triggers for AWS CodeDeploy Event Notifications
All
resource_aws_codedeploy_deployment_group.goresource tests PASS.(During development, I sometimes got test failures, presumably because of eventual consistency errors.)
Relevant AWS links:
NOTES/THOUGHTS/QUESTIONS (feedback welcome):
validateTriggerEventvalidator function invalidators.go. However, it seems very specific to CodeDeploy so I kept it with the resource. If it were to go invalidators.go, it would have to be renamed to something likevalidateAwsCodeDeployTriggerEvent.validateTriggerEventfunction is a bit clumsy and could probably be written to be more idiomatic in go.trigger_configurationscould be developed as a separate resource, although this could also be said of the other resources attached to the Deployment Group. This implementation is consistent.trigger_configurationfields withtriggerseems a tiny bit redundant, but it adheres to the structure defined in the aws-sdk-go api, which I think is important. I briefly debated a more terse schema, like this:POSSIBLE ENHANCEMENTS
buildTriggerConfigsandtriggerConfigsToMapfunctions.