-
Notifications
You must be signed in to change notification settings - Fork 736
switch to code-generated waiters for remaining services #2994
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
Conversation
Madrigal
left a comment
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.
We have about 600 waiters that changed with this new re-gen, so it's impossible to review them all. I did a quick search for jmespath.Search and manually classified them into the following 6 types of waiters
Simple selector
jmespath.Search("AuditReportStatus", output)- Sample location service/acmpca/api_op_DescribeCertificateAuthorityAuditReport.go
Nested selector with list
jmespath.Search("Certificate.DomainValidationOptions[].ValidationStatus", output)- Sample location service/acm/api_op_DescribeCertificate.go
Wildcard selector (only one I saw)
jmespath.Search("VerificationAttributes.*.VerificationStatus", output)- Sample location service/ses/api_op_GetIdentityVerificationAttributes.go
Length selector
jmespath.Search("length(DBClusterSnapshots) ==0", output)- Sample location service/rds/api_op_DescribeDBClusterSnapshots.go
Complex filter expression
jmespath.Search("length(services[?!(length(deployments) ==1&& runningCount == desiredCount)]) ==0", output)- Location service/ecs/api_op_DescribeServices.go
AND simple selector
jmespath.Search("status != 'CANCELLING' && status != 'CANCELLED'", output)- Sample location service/neptunegraph/api_op_GetExportTask.go
Took a look at the generated code and ensured that the code is equivalent and correct. Left some comments for observations, but everything looks correct.
This is a great change to get rid of one of our last dependencies in the Go SDK 🎉
| for _, v := range v4 { | ||
| if string(v) != expectedValue { | ||
| match = false | ||
| break |
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.
nice extra optimization we got
| pathValue, err := jmespath.Search("VerificationAttributes.*.VerificationStatus", output) | ||
| if err != nil { | ||
| return false, fmt.Errorf("error evaluating waiter state: %w", err) | ||
| v1 := output.VerificationAttributes |
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.
[observation] This wildcard took a 2nd reading. The model is
VerificationAttributes map[string]types.IdentityVerificationAttributes
So iterating through the values gets us the same result as the wildcard
| } | ||
|
|
||
| if err == nil { | ||
| pathValue, err := jmespath.Search("length(services[?!(length(deployments) == `1` && runningCount == desiredCount)]) == `0`", output) |
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.
For my sanity reading this expression, this is what this means (which may be easier to reason in the newer version)
- is there an ongoing deployment ->
length(deployments) == 1 - do we have enough containers running already ->
runningCount == desiredCount - are there any services where this is not happening ->
[?!(a&&b)] - on this cluster, is the service stable, meaning there are no deployments and there are enough containers running? ->
length[] == 0
Closes #2977