Skip to content

Add e2e test for automatic sidecar injection #4926

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
Closed
wants to merge 4 commits into from
Closed

Add e2e test for automatic sidecar injection #4926

wants to merge 4 commits into from

Conversation

yusuoh
Copy link

@yusuoh yusuoh commented Apr 13, 2018

No description provided.

@yusuoh yusuoh requested review from ayj and chxchx April 13, 2018 01:22
@codecov
Copy link

codecov bot commented Apr 13, 2018 •
edited
Loading

Codecov Report

Merging #4926 into master will increase coverage by 1%.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #4926     +/-   ##
========================================
+ Coverage      73%     74%     +1%     
========================================
  Files         322     310     -12     
  Lines       27618   26104   -1514     
========================================
- Hits        20141   19123   -1018     
+ Misses       6689    6206    -483     
+ Partials      788     775     -13
Impacted Files Coverage Δ
pilot/pkg/model/jwks_resolver.go 56% <0%> (-12%) ⬇️
istioctl/cmd/istioctl/convert/cmd.go 40% <0%> (-6%) ⬇️
mixer/adapter/stackdriver/metric/metric.go 82% <0%> (-5%) ⬇️
mixer/adapter/stackdriver/helper/common.go 79% <0%> (-5%) ⬇️
pilot/pkg/serviceregistry/kube/queue.go 86% <0%> (-3%) ⬇️
pilot/pkg/proxy/envoy/v2/lds.go 59% <0%> (-3%) ⬇️
pilot/cmd/pilot-discovery/main.go 75% <0%> (-3%) ⬇️
pilot/pkg/proxy/envoy/v2/ads.go 75% <0%> (-3%) ⬇️
pilot/pkg/proxy/envoy/v2/eds.go 77% <0%> (-3%) ⬇️
pilot/pkg/proxy/envoy/v1/watcher.go 52% <0%> (-2%) ⬇️
... and 95 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 26262e8...896a456. Read the comment docs.

@istio-merge-robot
Copy link

@yusuoh PR needs rebase

@istio-merge-robot istio-merge-robot added the needs-rebase Indicates a PR needs to be rebased before being merged label Apr 13, 2018
@istio-testing istio-testing removed the needs-rebase Indicates a PR needs to be rebased before being merged label Apr 13, 2018
@istio-testing istio-testing added needs-rebase Indicates a PR needs to be rebased before being merged and removed needs-rebase Indicates a PR needs to be rebased before being merged labels Apr 13, 2018
@chxchx
Copy link
Contributor

chxchx commented Apr 13, 2018

/lgtm

@chxchx
Copy link
Contributor

chxchx commented Apr 13, 2018

/hold
Would like to have Jason review this before it gets merged.

Also, you may want to update prow/e2e-cluster_wide-auth.sh in the daily-release repo as well. The tricky thing is after adding the flag the test will not pass until a new daily candidate that actually have this flag defined is cut.

@istio-testing istio-testing added the do-not-merge/hold Block automatic merging of a PR. label Apr 13, 2018
ayj
ayj reviewed Apr 13, 2018
View reviewed changes
tests/e2e/framework/kubernetes.go Outdated
} else {
if *authEnable {
istioYaml = authInstallFileNamespace
}
if *useAutomaticInjection {
Copy link
Contributor

Choose a reason for hiding this comment

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

+1

ayj
ayj approved these changes Apr 13, 2018
View reviewed changes
Copy link
Contributor

@ayj ayj left a comment

Choose a reason for hiding this comment

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

/lgtm

@chxchx
Copy link
Contributor

chxchx commented Apr 13, 2018

/hold cancel

@istio-testing istio-testing removed the do-not-merge/hold Block automatic merging of a PR. label Apr 13, 2018
@istio-merge-robot
Copy link

@yusuoh PR needs rebase

@istio-merge-robot istio-merge-robot added the needs-rebase Indicates a PR needs to be rebased before being merged label Apr 13, 2018
@istio-testing istio-testing removed lgtm needs-rebase Indicates a PR needs to be rebased before being merged labels Apr 13, 2018
@chxchx
Copy link
Contributor

chxchx commented Apr 16, 2018

ping

@istio-testing istio-testing added needs-rebase Indicates a PR needs to be rebased before being merged and removed needs-rebase Indicates a PR needs to be rebased before being merged labels Apr 16, 2018
@yusuoh
Copy link
Author

yusuoh commented Apr 17, 2018

/test istio-unit-tests

Fix sidecar injector in e2e tests
1b5ef4c 
add yaml

enable webhook in cluser wide test

fix lint

fix lint

fix lint
@chxchx
Copy link
Contributor

chxchx commented Apr 18, 2018

/lgtm

@istio-testing
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ayj, chxchx
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: geeknoid

Assign the PR to them by writing /assign @geeknoid in a comment when ready.

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

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@chxchx
Copy link
Contributor

chxchx commented Apr 18, 2018

/assign @geeknoid

@yusuoh yusuoh requested a review from costinm April 18, 2018 20:01
@ayj ayj mentioned this pull request Apr 19, 2018
Closed
@istio-testing istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Apr 27, 2018
@istio-testing
Copy link
Collaborator

New changes are detected. LGTM label has been removed.

@istio-testing istio-testing removed lgtm needs-rebase Indicates a PR needs to be rebased before being merged labels May 14, 2018
@istio-testing
Copy link
Collaborator

@yusuoh: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
prow/istio-pilot-e2e-envoyv2-v1alpha3.sh 896a456 link /test istio-pilot-e2e-envoyv2-v1alpha3
prow/istio-pilot-e2e.sh 896a456 link /test istio-pilot-e2e
prow/e2e-bookInfoTests.sh 896a456 link /test e2e-bookInfo
prow/e2e-bookInfoTests-v1alpha3.sh 896a456 link /test e2e-bookInfo-envoyv2-v1alpha3
prow/e2e-simpleTests.sh 896a456 link /test e2e-simple

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.

@rshriram rshriram closed this May 22, 2018
@ostromart ostromart mentioned this pull request Jun 8, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chxchx chxchx Awaiting requested review from chxchx

@costinm costinm Awaiting requested review from costinm

1 more reviewer

@ayj ayj ayj approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees

@ayj ayj

@chxchx chxchx

@geeknoid geeknoid

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@yusuoh @istio-merge-robot @chxchx @istio-testing @ayj @googlebot @rshriram @geeknoid