-
Notifications
You must be signed in to change notification settings - Fork 50
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
Prevent FAILED peerings/attachments from failing deletes #394
Conversation
closes #393 |
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.
I think I am good with the logic but I am curious how you tested this? Did you create a failed peering on purpose and try to resolve it?
internal/provider/data_source_aws_transit_gateway_attachment.go
Outdated
Show resolved
Hide resolved
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 for the fix! If possible I'd like to encapsulate these failed state checks inside the peering helpers, rather than repeated over the data source functions.
return nil | ||
} | ||
|
||
// If it's in a state where it could later become ACTIVE, wait. | ||
for _, state := range clients.WaitForPeeringToBeActivePendingStates { | ||
waitState := 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.
Could we move this failed/delete check into the WaitFor*State helpers? We can check the state in the result
here:
result, err := stateChangeConfig.WaitForStateContext(ctx) |
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.
I'm a bit torn on DRY'ing this into the Waiters because of the shenanigans I'm trying to do here where:
- if it's a terminal state (FAILED/DELETING) at the start of the Read, throw a Warning
- if it's a terminal state after trying to wait in the Read, throw an Error
Also this is a specific (hack?) for the Peering data resources that I wouldn't want to add to WaitForPeeringToBeAccepted
for example because we always want it to throw an Error there:
peering, err = clients.WaitForPeeringToBeAccepted(ctx, client, peering.ID, hvn1Link.ID, loc, d.Timeout(schema.TimeoutCreate)) |
I'm not opposed necessarily, but because of two reasons above I left it within the data_resource_* Reads
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.
Hmm I see, that does make sense. In the interest of getting this in, we can leave it like this for now, and revisit whenever we add another peering resource. Maybe leave a comment with that context?
🛠️ Description
See: #393
I also bumped the AWS provider because anything < v4.0.0 fails out on ARM chips
🚢 Release Note
Release note for CHANGELOG:
🏗️ Acceptance tests
Output from acceptance testing: