-
Notifications
You must be signed in to change notification settings - Fork 80
Refine e2e implementation #477
Conversation
@@ -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 |
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 weird, whats the reason for this?
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.
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.
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.
maybe put a comment here, it does look odd.
@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 . |
@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 |
b86de61
to
16d3b6e
Compare
This is a refinement based upon the prior PRs in this area. Depends-On: istio/tools#483
/test e2e_operator |
/cc @incfly |
@morvencao please take a look and approve if not already merged. The gate is blocked. Cheers |
Refine e2e implementation
This is a refinement based upon the prior PRs in this area.
Depends-On: istio/tools#483