Skip to content

add PROXY_REPO_SHA removing overloaded meaning of PROXY_TAG #4158

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
Merged
merged 3 commits into from
Mar 10, 2018
Merged

add PROXY_REPO_SHA removing overloaded meaning of PROXY_TAG #4158

merged 3 commits into from
Mar 10, 2018

Conversation

rkpagadala
Copy link
Contributor

PROXY_TAG now refers to the docker image tag build from file pilot/docker/Dockerfile.proxy or pilot/docker/Dockerfile.proxy_debug
PROXY_REPO_SHA refers to the sha of istio/proxy, and is used to download the envoy binary used to build the proxy docker image referred above.
fixes #4157

@rkpagadala rkpagadala requested review from mattdelco and a team March 9, 2018 23:11
@mattdelco
Copy link
Contributor

Will there also be a corresponding change to the daily qualification tests, e.g. https://github.com/istio-releases/daily-release/blob/master/prow/daily-e2e-cluster_wide-auth-skew.sh#L39 ?

Otherwise, this change is a step in the right direction, though if PROXY_HUB/TAG is now for pilot's proxy then the rule for istio-archive in the Makefile probably needs to change what it passes to install/updateVersion.sh. At a minimum -r probably needs to be changed from $(VERSION) to $(VERSION), though really this parameter should be modified to behave like the other parameters and handle the hub,tag representation (so that PROXY_HUB can be set, which I think it can't currently be set via the command-line except via ALL_HUB_TAG. An option is to start having the Makefile use -a rather than the separate parameters for pilot, proxy, mixer, ca.

@codecov
Copy link

codecov bot commented Mar 9, 2018 •
edited
Loading

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #4158    +/-   ##
=======================================
- Coverage      73%     73%   -<1%     
=======================================
  Files         313     313            
  Lines       28759   28682    -77     
=======================================
- Hits        20901   20837    -64     
+ Misses       6537    6535     -2     
+ Partials     1321    1310    -11
Impacted Files Coverage Δ
...olarwinds/internal/papertrail/papertrail_logger.go 49% <0%> (-9%) ⬇️
mixer/adapter/servicecontrol/handler.go 39% <0%> (-7%) ⬇️
mixer/adapter/prometheus/prometheus.go 80% <0%> (-3%) ⬇️
mixer/adapter/servicecontrol/utils.go 86% <0%> (-3%) ⬇️
mixer/adapter/fluentd/fluentd.go 76% <0%> (-1%) ⬇️
mixer/tools/adapterlinter/main.go 81% <0%> (-1%) ⬇️
mixer/adapter/statsd/statsd.go 96% <0%> (-1%) ⬇️
pilot/pkg/proxy/envoy/v2/mesh.go 75% <0%> (ø) ⬇️
broker/pkg/platform/kube/crd/client.go 71% <0%> (ø) ⬇️
pilot/pkg/serviceregistry/kube/controller.go 69% <0%> (ø) ⬆️
... and 11 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 9fd19e3...a8913e2. Read the comment docs.

@mattdelco
Copy link
Contributor

Actually, I suppose the tag value that's passed to -r is already okay (my comment on "from $(VERSION) to $(VERSION)" was sadly nonsensical), and the only issue might be that PROXY_HUB isn't explicitly set when -r (et al) is used instead of -a. I'm not sure offhand where PROXY_HUB is set during a release build since I don't think the build sets it and a default isn't present in istio.VERSION (unless I'm overlooking on of those strange places where PROXY_HUB defaults to the value of PILOT_HUB.

@rkpagadala rkpagadala requested a review from a team March 10, 2018 00:15
@rkpagadala
Copy link
Contributor Author

rkpagadala commented Mar 10, 2018 •
edited
Loading

PILOT_HUB is used where PROXY_HUB should be logically. Fixed all that and got rid of istio.VERSION
PTAL

@rkpagadala
Copy link
Contributor Author

/test istio-unit-tests

@ldemailly
Copy link
Member

/lgtm

@istio-merge-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ldemailly

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

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

@rkpagadala
Copy link
Contributor Author

/test e2e-simple
/test e2e-bookInfo

@istio-merge-robot
Copy link

/test all [submit-queue is verifying that this PR is safe to merge]

@rkpagadala
Copy link
Contributor Author

/test istio-unit-tests

@istio-merge-robot
Copy link

/test all [submit-queue is verifying that this PR is safe to merge]

@istio-merge-robot
Copy link

Automatic merge from submit-queue.

@istio-merge-robot istio-merge-robot merged commit 6fe9379 into istio:master Mar 10, 2018
@istio-testing
Copy link
Collaborator

istio-testing commented Mar 10, 2018 •
edited
Loading

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

Test name Commit Details Rerun command
prow/e2e-bookInfoTests.sh a8913e2 link /test e2e-bookInfo
prow/e2e-simpleTests.sh a8913e2 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.

sebastienvas added a commit that referenced this pull request Mar 10, 2018
@sebastienvas
Revert "add PROXY_REPO_SHA removing overloaded meaning of PROXY_TAG (#…
e59a509 
…4158)"

This reverts commit 6fe9379.
@sebastienvas sebastienvas mentioned this pull request Mar 10, 2018
Closed
@sebastienvas
Copy link
Contributor

this PR got merged but it failed all e2e. Something is wrong with generated yaml

mandarjog added a commit to mandarjog/istio that referenced this pull request Mar 10, 2018
@mandarjog
Revert "replace PROXY_TAG by PROXY_REPO_SHA istio#4158"
008d8fc 
Revert "add PROXY_REPO_SHA removing overloaded meaning of PROXY_TAG (istio#4158)"

This reverts commit 6fe9379.
This was referenced Mar 10, 2018
Closed
Merged
rkpagadala pushed a commit that referenced this pull request Mar 10, 2018
@mandarjog @rkpagadala
Revert "replace PROXY_TAG by PROXY_REPO_SHA #4158" (#4173)
fd9b754 
* Revert "replace PROXY_TAG by PROXY_REPO_SHA #4158"

Revert "add PROXY_REPO_SHA removing overloaded meaning of PROXY_TAG (#4158)"

This reverts commit 6fe9379.

* Revert "renable aggregate_TestControllerEvents (#4149)"

This reverts commit c2d22a2.
@mattdelco
Copy link
Contributor

I know I'm beating a dead (or should I say reverted) horse, but on any subsequent attempt to delete istio.VERSION be sure to grep across the various for uses of the file (e.g., the releae build failed because the ${ISTIO_OUT}/archive rule in the Makefile has a dependency on the then-missing istio.VERSION).

@rkpagadala
Copy link
Contributor Author

The most important change from the previous diff is setting the defaults copied from istio.VERSION
https://github.com/istio/istio/pull/4179/files#diff-c864be78fb30a2c170271b1b64576a9dR52

@chxchx chxchx mentioned this pull request Mar 29, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattdelco mattdelco Awaiting requested review from mattdelco

Assignees

@ldemailly ldemailly

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Clear up confusion around proxy tag
7 participants
@rkpagadala @mattdelco @ldemailly @istio-merge-robot @istio-testing @sebastienvas @googlebot