-
Notifications
You must be signed in to change notification settings - Fork 7.8k
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
istioctl: add waypoint integration test #47615
Conversation
@GregHanson @istio/wg-test-and-release-maintainers PTAL, thanks |
return fmt.Errorf("gateway is not ready: %v", err) | ||
} | ||
} else if err == nil { | ||
return fmt.Errorf("gateway is not cleaned up") |
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: could this error and the one above for sa1
say something to state that it was the multi-delete that failed? Something like failed to remove multiple gateways at once, sa2 not cleaned up
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 looks better, fixed.
ab60800
to
a7de27b
Compare
kindly ping @istio/wg-test-and-release-maintainers |
"--service-account", | ||
sa, | ||
}) | ||
retry.UntilSuccessOrFail(t, func() error { |
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 this could run faster if you create all 3 in a loop and then check for them in a separate loop
@jwendell Thank you for the careful code review! I think I've addressed all the comments. PTAL |
Please provide a description of this PR:
Follow-up to #47539. I know we have
apply/delete
commands tested in other tests, but this is for the entire command which covers all of its features, to ensure that further changes don't break any functionnalities like other istioctl commands.