Skip to content

use proxy version from Dockerfile, don't prepend GCS_PATH#3676

Merged
istio-merge-robot merged 1 commit intoistio:masterfrom
rkpagadala:master
Feb 22, 2018
Merged

use proxy version from Dockerfile, don't prepend GCS_PATH#3676
istio-merge-robot merged 1 commit intoistio:masterfrom
rkpagadala:master

Conversation

@rkpagadala
Copy link
Copy Markdown
Contributor

No description provided.

@rkpagadala rkpagadala requested review from a team, hklai, mandarjog and mattdelco February 22, 2018 02:55
@@ -62,7 +62,7 @@ done

DEFAULT_GCS_PATH="https://storage.googleapis.com/istio-release/releases/${TAG_NAME}"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It seems like the "https://storage.googleapis.com/" should also be removed from the DEFAULT_GCS_PATH line.

I assume this change has to do with the "ISTIO_URL" line in the Makefile. An alternative is to use "ISTIO_URL=$TEST_PATH" rather than "ISTIO_GCS=$TEST_PATH" in this script, but I suppose ISTIO_GCS is more appropriate since the motivation for the ISTIO_URL option was to provide a way for people to be able to do a build without relying on GCS.

@ldemailly
Copy link
Copy Markdown
Member

sgtm but how do you test/know that it works ?

@ldemailly
Copy link
Copy Markdown
Member

/lgtm
assuming you manually tested and matt is ok with the change

@istio-merge-robot
Copy link
Copy Markdown

[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.

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

@istio-merge-robot
Copy link
Copy Markdown

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

@istio-merge-robot
Copy link
Copy Markdown

Automatic merge from submit-queue.

@istio-merge-robot istio-merge-robot merged commit fca4054 into istio:master Feb 22, 2018
@mattdelco
Copy link
Copy Markdown
Contributor

The most straightforward test is to run:

mkdir /tmp/scratch
release/cloud_builder.sh -b -c -h gcr-hub -p gcs-bucket/gcs-path -o /tmp/scratch

and then look at the istio.VERSION in the tar files in /tmp/scratch (specifically the test tars for this change). I'm okay with the change, though the default could be fixed too (though in the common cases the default isn't used).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants