Skip to content
This repository was archived by the owner on Oct 7, 2020. It is now read-only.

Conversation

sdake
Copy link
Member

@sdake sdake commented Oct 28, 2019

Refine e2e implementation

This is a refinement based upon the prior PRs in this area.

Depends-On: istio/tools#483

@sdake sdake requested a review from a team as a code owner October 28, 2019 22:53
@istio-testing istio-testing added the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Oct 28, 2019
@googlebot googlebot added the cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. label Oct 28, 2019
@istio-testing istio-testing added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 28, 2019
@@ -86,7 +86,7 @@ update-goldens:
GOARCH=$(TARGET_ARCH) GOOS=$(TARGET_OS) REFRESH_GOLDEN=true go test ./cmd/mesh/...

e2e:
@tests/e2e/e2e.sh
@HUB=$(HUB) TAG=$(TAG) bash -c tests/e2e/e2e.sh
Copy link
Member

Choose a reason for hiding this comment

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

This looks weird, whats the reason for this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Shell scripts will not take environment variables. I tried HUB=$(HUB) TAG=$(TAG) ./tests/e2e/e2e.sh with no joy. Quick Internet search shows this is the best solution to the problem.

Copy link
Member

Choose a reason for hiding this comment

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

maybe put a comment here, it does look odd.

@irisdingbj
Copy link
Member

@sdake operator e2e already been fixed via #456. The reason why e2e fails in that PR is because it has not got the fix from istio/istio#18352 .

@sdake
Copy link
Member Author

sdake commented Oct 29, 2019

@irisdingbj e2e did not work locally for me. Additionally, there were several unaddressed comments - which should hopefully be resolved when this work merges.

But my apologies for not baackporting your work earlier and thinking e2e was broken in the checks. Clearly an error on my part. Would you submit a test-infra PR to make the check voting now?

Cheers
-steve

@sdake sdake force-pushed the e2e/fix branch 3 times, most recently from b86de61 to 16d3b6e Compare October 29, 2019 11:19
@istio-testing istio-testing added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 29, 2019
This is a refinement based upon the prior PRs in this area.

Depends-On: istio/tools#483
@sdake sdake changed the title WIP: Make e2e work correctly WIP: Refine e2e implementation Oct 29, 2019
@sdake
Copy link
Member Author

sdake commented Oct 29, 2019

/test e2e_operator

@sdake sdake changed the title WIP: Refine e2e implementation Refine e2e implementation Oct 29, 2019
@istio-testing istio-testing removed the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Oct 29, 2019
@incfly
Copy link

incfly commented Oct 29, 2019

/cc @incfly

@sdake
Copy link
Member Author

sdake commented Oct 29, 2019

@morvencao please take a look and approve if not already merged. The gate is blocked.

Cheers
-steve

@istio-testing istio-testing merged commit 33fd23e into istio:master Oct 30, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. 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.

7 participants