Skip to content
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

Merged
merged 3 commits into from
Nov 10, 2023

Conversation

hanxiaop
Copy link
Member

@hanxiaop hanxiaop commented Oct 27, 2023

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.

@hanxiaop hanxiaop requested a review from a team as a code owner October 27, 2023 08:48
@istio-policy-bot istio-policy-bot added area/test and release area/user experience release-notes-none Indicates a PR that does not require release notes. labels Oct 27, 2023
@istio-testing istio-testing added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 27, 2023
@hanxiaop
Copy link
Member Author

hanxiaop commented Nov 2, 2023

@GregHanson @istio/wg-test-and-release-maintainers PTAL, thanks

@hanxiaop hanxiaop requested a review from GregHanson November 2, 2023 06:29
return fmt.Errorf("gateway is not ready: %v", err)
}
} else if err == nil {
return fmt.Errorf("gateway is not cleaned up")
Copy link
Member

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks better, fixed.

@hanxiaop hanxiaop requested a review from GregHanson November 3, 2023 02:38
@hanxiaop
Copy link
Member Author

hanxiaop commented Nov 7, 2023

kindly ping @istio/wg-test-and-release-maintainers

tests/integration/ambient/waypoint_test.go Outdated Show resolved Hide resolved
tests/integration/ambient/waypoint_test.go Outdated Show resolved Hide resolved
"--service-account",
sa,
})
retry.UntilSuccessOrFail(t, func() error {
Copy link
Member

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

tests/integration/ambient/waypoint_test.go Outdated Show resolved Hide resolved
tests/integration/ambient/waypoint_test.go Outdated Show resolved Hide resolved
@hanxiaop
Copy link
Member Author

@jwendell Thank you for the careful code review! I think I've addressed all the comments. PTAL

@istio-testing istio-testing merged commit b08d9d1 into istio:master Nov 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/test and release area/user experience release-notes-none Indicates a PR that does not require release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants