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.

Sign up for GitHub

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

Jump to bottom

Remove ambient bookinfo integration tests and move necessary ones to baseline echo int tests #49353

Closed
wants to merge 5 commits into from
Closed

Remove ambient bookinfo integration tests and move necessary ones to baseline echo int tests #49353

wants to merge 5 commits into from

Conversation

saiskee
Copy link
Contributor

@saiskee saiskee commented Feb 13, 2024 •
edited by istio-policy-bot

Removes bookinfo ambient integration tests and moves the necessary ones (such as waypoint template changes) to the baseline ambient tests.

Addresses #46682

@saiskee
remove redundant bookinfo ambient tests and add waypoint template cha…
18346e7 
…nge test o baseline tests

Signed-off-by: Keerthan Ekbote <keerthan.ekbote@solo.io>
@saiskee saiskee requested a review from a team as a code owner February 13, 2024 22:56
@istio-policy-bot istio-policy-bot added area/ambient Issues related to ambient mesh release-notes-none Indicates a PR that does not require release notes. labels Feb 13, 2024
@istio-policy-bot
Copy link

😊 Welcome @saiskee! This is either your first contribution to the Istio istio repo, or it's been
a while since you've been here.

You can learn more about the Istio working groups, Code of Conduct, and contribution guidelines
by referring to Contributing to Istio.

Thanks for contributing!

Courtesy of your friendly welcome wagon.

@istio-testing istio-testing added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. needs-ok-to-test labels Feb 13, 2024
@istio-testing
Copy link
Collaborator

Hi @saiskee. Thanks for your PR.

I'm waiting for a istio member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@saiskee
format
a15435e 
Signed-off-by: Keerthan Ekbote <keerthan.ekbote@solo.io>
@bleggett
Copy link
Contributor

/ok-to-test

@istio-testing istio-testing added ok-to-test Set this label allow normal testing to take place for a PR not submitted by an Istio org member. and removed needs-ok-to-test labels Feb 14, 2024
bleggett
bleggett reviewed Feb 14, 2024
View reviewed changes
tests/integration/ambient/main_test.go Outdated
SidecarWaypoint echo.Instances
SidecarCaptured echo.Instances
Namespace namespace.Instance
// Waypoint echo service
Copy link
Contributor

@bleggett bleggett Feb 14, 2024 •
edited

Choose a reason for hiding this comment

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

If we are going to annotate these with comments, I would prefer if they are more explicit/verbose/obvious about what "kind" of echo service this is for people not familiar with the test suite - e.g.

  • // Traffic-captured echo services configured with L7 waypoint proxies
  • // Traffic-captured echo services
  • // Traffic-captured echo services with an injected sidecar

etc etc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

1b86c5b

tests/integration/ambient/bookinfo_test.go
})
})

t.NewSubTest("ingress receives waypoint updates").Run(func(t framework.TestContext) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we still covering this ingress scenario? maybe elsewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a test here

@istio-testing istio-testing added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 14, 2024
@saiskee
Copy link
Contributor Author

saiskee commented Feb 14, 2024

/retest

@istio-testing
Copy link
Collaborator

@saiskee: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
integ-pilot-istiodremote-mc_istio 1bb762e link true /test integ-pilot-istiodremote-mc
integ-security-multicluster_istio 1bb762e link true /test integ-security-multicluster
integ-ambient_istio 1bb762e link true /test integ-ambient

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@saiskee saiskee mentioned this pull request Feb 15, 2024
Merged
7 tasks
bleggett
bleggett reviewed Feb 19, 2024
View reviewed changes
tests/integration/ambient/baseline_test.go
@@ -2426,3 +2517,28 @@ func TestDirect(t *testing.T) {
})
})
}

func deleteWaypoints(t framework.TestContext, nsConfig namespace.Instance, sa string) {
Copy link
Contributor

@bleggett bleggett Feb 19, 2024 •
edited

Choose a reason for hiding this comment

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

I'm not an expert on the framework here but noting that deleting waypoints here seems to cause other tests to fail, we might be causing side effects by doing this.

Worth chasing down the side effects (they would be bugs or test bugs either way).

This func deleteWaypoints probably belongs in pkg/test/framework/components/ambient/waypoint.go?

and should probably either be a func on the WaypointProxy interface, or take an instance of WaypointProxy.

If other tests are still failing with the waypoint delete, it might be because they expect one to be in place but do not explicitly assert/create one.

@istio-testing istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Feb 29, 2024
@istio-testing
Copy link
Collaborator

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@saiskee
Copy link
Contributor Author

saiskee commented Mar 7, 2024

i've gotten pulled off onto something else, hope to get back to this but for now won't be working on this.

@bleggett bleggett mentioned this pull request Mar 15, 2024
Merged
@istio-policy-bot istio-policy-bot added the lifecycle/stale Indicates a PR or issue hasn't been manipulated by an Istio team member for a while label Apr 6, 2024
@howardjohn howardjohn mentioned this pull request Apr 15, 2024
Merged
@istio-policy-bot
Copy link

🚧 This issue or pull request has been closed due to not having had activity from an Istio team member since 2024-03-07. If you feel this issue or pull request deserves attention, please reopen the issue. Please see this wiki page for more information. Thank you for your contributions.

Created by the issue and PR lifecycle manager.

@istio-policy-bot istio-policy-bot added the lifecycle/automatically-closed Indicates a PR or issue that has been closed automatically. label Apr 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bleggett bleggett bleggett left review comments

Assignees
No one assigned
Labels
area/ambient Issues related to ambient mesh lifecycle/automatically-closed Indicates a PR or issue that has been closed automatically. lifecycle/stale Indicates a PR or issue hasn't been manipulated by an Istio team member for a while needs-rebase Indicates a PR needs to be rebased before being merged ok-to-test Set this label allow normal testing to take place for a PR not submitted by an Istio org member. release-notes-none Indicates a PR that does not require release notes. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@saiskee @istio-policy-bot @istio-testing @bleggett