Skip to content

Use latest 'stable' istio and 'getLatestIstio' script when installing#1944

Merged
geeknoid merged 2 commits intoistio:masterfrom
selmanj:use_release_instead_of_git
Dec 4, 2017
Merged

Use latest 'stable' istio and 'getLatestIstio' script when installing#1944
geeknoid merged 2 commits intoistio:masterfrom
selmanj:use_release_instead_of_git

Conversation

@selmanj
Copy link
Copy Markdown
Contributor

@selmanj selmanj commented Nov 30, 2017 •
edited
Loading

What this PR does / why we need it:
Istio 0.3.0 was released, but is not currently the 'stable' version (according to downloadIstioCandidate.sh). This change moves the default back to 0.2.12, and also uses the recommended method of installation rather than attempting to clone the repo.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #

Special notes for your reviewer:
Small changes to startup-script. Should have no impact on any e2e tests (as it's not covered in any way).

Release note:

/cc @salrashid123 @costinm

@istio-testing
Copy link
Copy Markdown
Collaborator

istio-testing commented Nov 30, 2017

@selmanj: GitHub didn't allow me to request PR reviews from the following users: salrashid123.

Note that only istio members can review this PR, and authors cannot review their own PRs.

Details

In response to this:

What this PR does / why we need it:
Istio 0.3.0 was released, but is not currently the 'stable' version (according to downloadIstioCandidate.sh. This change moves the default back to 0.2.12, and also uses the recommended method of installation rather than attempting to clone the repo.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #

Special notes for your reviewer:
Small changes to startup-script. Should have no impact on any e2e tests (as it's not covered in any way).

Release note:

/cc @salrashid123 @costinm

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.

@istio-testing
Copy link
Copy Markdown
Collaborator

istio-testing commented Nov 30, 2017

Hi @selmanj. Thanks for your PR.

I'm waiting for a istio member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

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.

@selmanj selmanj mentioned this pull request Nov 30, 2017
Closed
@codecov
Copy link
Copy Markdown

codecov bot commented Nov 30, 2017

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1944      +/-   ##
==========================================
- Coverage   81.75%   81.39%   -0.36%     
==========================================
  Files         191      178      -13     
  Lines       15618    14167    -1451     
==========================================
- Hits        12768    11531    -1237     
+ Misses       2406     2247     -159     
+ Partials      444      389      -55
Flag Coverage Δ
#broker 46.09% <ø> (-1.18%) ⬇️
#mixer 83.3% <ø> (+0.02%) ⬆️
#pilot 79.12% <ø> (-1.41%) ⬇️
#security 92.03% <ø> (+3.14%) ⬆️
Impacted Files Coverage Δ
mixer/adapter/stackdriver/metric/bufferedClient.go 90.32% <0%> (-3.23%) ⬇️
mixer/adapter/denier/denier.go 88.88% <0%> (-3.22%) ⬇️
pilot/model/conversion.go 86.44% <0%> (-1.06%) ⬇️
mixer/adapter/memquota/dedup.go 100% <0%> (ø) ⬆️
mixer/pkg/aspect/descriptors.go 100% <0%> (ø) ⬆️
mixer/adapter/list/stringList.go 100% <0%> (ø) ⬆️
mixer/adapter/memquota/keys.go 100% <0%> (ø) ⬆️
mixer/adapter/svcctrl/client.go 0% <0%> (ø) ⬆️
mixer/adapter/list/ipList.go 100% <0%> (ø) ⬆️
pilot/proxy/envoy/policy.go
... and 25 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 9386e6c...4634421. Read the comment docs.

@mattdelco
Copy link
Copy Markdown
Contributor

mattdelco commented Dec 1, 2017

#1940 will address the oversight on downloadIstioCandidate.sh and I wouldn't read into that file too much on indicating stable vs. not.

@salrashid123
Copy link
Copy Markdown
Contributor

salrashid123 commented Dec 1, 2017

since we're enumerating the version number now anyway on selection, maybe just wget the tar.gz direct?
eg:

@selmanj
Copy link
Copy Markdown
Contributor Author

selmanj commented Dec 1, 2017

@salrashid123 I initially considered that until I found the getLatestIstio script, which seems to be the recommended way to obtain an arbitrary version of istio. While it doesn't too much more than download from the releases page, you could imagine in the future that istio releases might be hosted elsewhere or packaged differently. In that case, it seems reasonable that the script would continue to operate the same.

I don't feel that strongly about this so if you disagree let me know.

@salrashid123
Copy link
Copy Markdown
Contributor

salrashid123 commented Dec 1, 2017

SGTM as-is (i was just offering an alternative)

@costinm
Copy link
Copy Markdown
Contributor

costinm commented Dec 2, 2017

Well, 0.3.0 was released - but there is a bug, so stick with 0.2 for now.

We're also discussing using a :latest_stable label in docker.

@geeknoid
Copy link
Copy Markdown
Contributor

geeknoid commented Dec 4, 2017

/approve

@istio-merge-robot
Copy link
Copy Markdown

istio-merge-robot commented Dec 4, 2017

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: geeknoid

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

@geeknoid geeknoid merged commit fdd68c9 into istio:master Dec 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@costinm costinm costinm approved these changes

+1 more reviewer

@geeknoid geeknoid geeknoid approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

do-not-merge/post-submit

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@selmanj @istio-testing @mattdelco @salrashid123 @costinm @geeknoid @istio-merge-robot @googlebot