Skip to content

Remove shell e2e test#736

Merged
rshriram merged 5 commits intoistio:masterfrom
sebastienvas:e2e-remove
Sep 14, 2017
Merged

Remove shell e2e test#736
rshriram merged 5 commits intoistio:masterfrom
sebastienvas:e2e-remove

Conversation

@sebastienvas
Copy link
Copy Markdown
Contributor

@sebastienvas sebastienvas commented Sep 11, 2017 •
edited
Loading

Removing deprecated e2e test framework in favor of the golang framework

Fixes #645
Fixes #185

none

@andraxylia
Copy link
Copy Markdown
Contributor

andraxylia commented Sep 11, 2017

/LGTM

@istio-merge-robot
Copy link
Copy Markdown

istio-merge-robot commented Sep 11, 2017

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andraxylia

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@rshriram
Copy link
Copy Markdown
Member

rshriram commented Sep 11, 2017

Please wait. @frankbu FYI.

@frankbu
Copy link
Copy Markdown
Contributor

frankbu commented Sep 12, 2017

The shell e2e is the only one that works on minikube, so before it's removed the go version should be fixed to work in minikube too.

@sebastienvas
Copy link
Copy Markdown
Contributor Author

sebastienvas commented Sep 12, 2017

@frankbu Have you tried the --use-local-cluster when running the tests ?

@sebastienvas
Copy link
Copy Markdown
Contributor Author

sebastienvas commented Sep 12, 2017

I don't think this is any different:

istio/tests/kubeUtils.sh

Line 84 in c2208fe

if [[ ! -z ${OUT_OF_CLUSTER} ]]; then

https://github.com/istio/istio/blob/master/tests/e2e/util/kubeUtils.go#L165

I think the golang framework changes loadbalander to nodePort such that it does not create the ingress.

@kyessenov
Copy link
Copy Markdown
Contributor

kyessenov commented Sep 12, 2017

@sebastienvas please make sure it runs first, maybe ask someone with minikube (I don't have on installed yet, sorry). The flag name out-of-cluster can mean either GKE or minikube, and those have differences between them.

@sebastienvas
Copy link
Copy Markdown
Contributor Author

sebastienvas commented Sep 12, 2017

@kyessenov I am asking @ZackButcher to test, I am just stating that the shell framework does not have anything different to make it work on minikube.

@frankbu
Copy link
Copy Markdown
Contributor

frankbu commented Sep 12, 2017

@sebastienvas I had not tried --use-local-cluster, but it does look like it will do the right thing. I think the problem is that you changed the default behavior. With kubeTest.sh, the default was some kind of not-not-out-of-cluster (not sure what that meant), but it worked on minikube without specifying any args. Just running plain tests/e2e.sh does not work on minikube. Maybe you can change the default, or at least document clearly how to run on minikube, because I think that plenty of people will be using it.

@istio-merge-robot
Copy link
Copy Markdown

istio-merge-robot commented Sep 12, 2017

/lgtm cancel //PR changed after LGTM, removing LGTM. @andraxylia @sebastienvas

@sebastienvas
Copy link
Copy Markdown
Contributor Author

sebastienvas commented Sep 12, 2017

@frankbu I added some documentation. Please review.

frankbu
frankbu reviewed Sep 12, 2017
View reviewed changes
tests/e2e/README.md
In order to talk to istio ingress, we use the ingress IP by default. If your
cluster is on the same local network and cannot provide external IP, use the `--use-local-cluster` flag.
In that case, the framework will not create a LoadBalancer and talk directly to the Pod running istio-ingress.

Copy link
Copy Markdown
Contributor

@frankbu frankbu Sep 12, 2017

Choose a reason for hiding this comment

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

This looks good but I would suggest "... cannot provide external IP (for example, minikube), use ...".

I also think you should remove the this sentence from the top of the README: "We recommend GKE cluster although miniKube works for some people too". It works fine for minikube.

@istio-testing
Copy link
Copy Markdown
Collaborator

istio-testing commented Sep 14, 2017 •
edited
Loading

@sebastienvas: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
prow/new-e2e-rbac_no_auth.sh a0a9963 link /test new-e2e-rbac_no_auth
Details

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.

@sebastienvas
Copy link
Copy Markdown
Contributor Author

sebastienvas commented Sep 14, 2017

/retest

@rshriram rshriram merged commit 34f530e into istio:master Sep 14, 2017
mandarjog pushed a commit to mandarjog/istio that referenced this pull request Oct 30, 2017
@sebastienvas
Tagging minor version when creating a new tag (istio#736)
e2956cd 
* Tagging minor version when creating a new tag

* Removed unused date

* Added a check for the version

* Fixes matcher


Former-commit-id: de23c84208cc9fe71a1fdd63b1cdebf4d3d22ae1
rshriram pushed a commit that referenced this pull request Oct 30, 2017
@sebastienvas @rshriram
Remove shell e2e test (#736)
edcb627 
* Remove shell e2e test

* Documents -use-local-cluster

* Fix documentation


Former-commit-id: 34f530e
mandarjog pushed a commit that referenced this pull request Oct 31, 2017
@sebastienvas
Tagging minor version when creating a new tag (#736)
a565ecc 
* Tagging minor version when creating a new tag

* Removed unused date

* Added a check for the version

* Fixes matcher


Former-commit-id: 8f4e88c18acda2daf158df1a49599c0bebbf1e78
vbatts pushed a commit to vbatts/istio that referenced this pull request Oct 31, 2017
@sebastienvas @rshriram
Remove shell e2e test (istio#736)
4abb5fa 
* Remove shell e2e test

* Documents -use-local-cluster

* Fix documentation


Former-commit-id: 34f530e
mandarjog pushed a commit that referenced this pull request Nov 2, 2017
@sebastienvas @rshriram
Remove shell e2e test (#736)
176ef79 
* Remove shell e2e test

* Documents -use-local-cluster

* Fix documentation


Former-commit-id: 34f530e
kyessenov pushed a commit to kyessenov/istio that referenced this pull request Aug 13, 2018
@qiwzhang
Add tsan and asan (istio#736)
0b01fba 
* Add tsan and asan

* remove @
kyessenov pushed a commit to kyessenov/istio that referenced this pull request Aug 13, 2018
@lizan @istio-merge-robot
Bring back accidentally deleted build options in istio#736 (istio#947)
8eae794 
Automatic merge from submit-queue.

Bring back accidentally deleted build options in istio#736

**What this PR does / why we need it**:

istio#736 accidentally remove `-c opt` from release build and a couple of compile options, bring them back.

cc @costinm @ldemailly 

**Special notes for your reviewer**:

**Release note**:
```release-note
None
```
howardjohn pushed a commit to howardjohn/istio that referenced this pull request Jan 12, 2020
@istio-testing
add serviceAnnotation to KubernetesResourcesSpec. (istio#736)
2f621f7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@frankbu frankbu frankbu left review comments

@andraxylia andraxylia andraxylia approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@andraxylia andraxylia

Labels

area/test and release

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@sebastienvas @andraxylia @istio-merge-robot @rshriram @frankbu @kyessenov @istio-testing @googlebot