Skip to content

Conversation

sdake
Copy link
Member

@sdake sdake commented Apr 20, 2018

The environment workgroup wants to deprecate the install/kubernetes/templates directory.

A few addons are left in the repository so they may be diffed against the Helm chart.

If there is any file you want to remain in repo, please speak up now.

Steven Dake added 2 commits April 19, 2018 17:30
The env wg is working to converge towards templating.

If there is a file in here you need, please weigh in on the PR.
Modify updateVersion.sh not to call updateVersion_orig.sh
@istio-testing
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

Assign the PR to them by writing /assign @linsun 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

@sdake sdake requested a review from mandarjog April 20, 2018 01:12
@sdake
Copy link
Member Author

sdake commented Apr 20, 2018

/assign @linsun

@istio-testing
Copy link
Collaborator

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

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

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.

@costinm
Copy link
Contributor

costinm commented Apr 20, 2018

Pilot tests seem to be failing due to '--kubeconfig=' ( no value ). Not sure how it relates to the change.

@costinm costinm added this to the Istio 0.8 milestone Apr 20, 2018
@costinm
Copy link
Contributor

costinm commented Apr 20, 2018

Added '0.8' label for tracking - I don't think we should ship 0.8 with the deprecated/unused templates, we must at least remove them from the release, but if possible from the code as well, to finish up the merging and shift to single templates.

@vadimeisenbergibm
Copy link
Contributor

Note that there is a reference to install/kubernetes/templates at https://github.com/istio/istio/blob/master/Makefile#L598

@@ -22,11 +22,6 @@ ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)/.."

cd ${ROOT}

# just run the old version
${ROOT}/install/updateVersion_orig.sh "$@"
Copy link
Member

Choose a reason for hiding this comment

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

@mandarjog I think had left this for one namespace or some other less major yaml ? have we migrated all tests to only use istio-auth.yaml / istio.yaml ?

@ldemailly
Copy link
Member

ldemailly commented Apr 20, 2018

as long as we still produce istio-*.yaml (and install/kubernetes/addons/*) that are compatible / same as before it seems great to have a unified way to do so and less confusion on the multiple templates

@ldemailly
Copy link
Member

where would I make the following fix, in the new world:

https://github.com/istio/istio/pull/5028/files#diff-17e443480eea200223e3547be4382604

@linsun
Copy link
Member

linsun commented Apr 21, 2018

Is there a way to see the release binary with this PR before this PR is merged? I 'd like to look at it to make sure we aren't missing anything major. cc @hklai

@sdake thanks for opening the PR to track this big concern.

@douglas-reid
Copy link
Contributor

In the PR description, we mention keeping the addons, but I think we actually want to remove some of them (and then transition the other ones). In particular, the duplicative grafana, prometheus, and zipkin templates should disappear (as they are outdated).

@costinm
Copy link
Contributor

costinm commented Apr 21, 2018 via email

@istio-testing
Copy link
Collaborator

@sdake: PR needs rebase.

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 istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Apr 21, 2018
@sdake
Copy link
Member Author

sdake commented Apr 22, 2018

@linsun I'm not sure how to produce a binary release. After having a good look at the codebase, this change may be dangerous as I don't entirely understand all of the variants in the templates directory and who needs what.

@douglas-reid i'll remove the addons as well, although I am not sure we have sorted out a solution for generating addons from via make generate_yaml.

@ldemailly agree that is the goal of this PR, however, it feels risky given that convergence on the helm charts has not completely finished as evidenced by the changes going into this directory.

@costinm
Copy link
Contributor

costinm commented Apr 22, 2018 via email

@sdake
Copy link
Member Author

sdake commented Apr 22, 2018

@costinm sounds good.

One tactical problem - I am loaded on documenting multicluster for the next 4 days and then i am traveling to Copenhagen returning May 5th. I can work on improving this PR after the docs are resolved, and even during travel but I'm not certain I have enough time to finish the job if the job involves convergence into the main helm chart (to enable a functional automated testing for example).

@sdake
Copy link
Member Author

sdake commented Apr 29, 2018

Here is citadel standalone - #5292.

Convergence of each of these templates into the main chart will take some time - I think not viable for 0.8.

@sdake
Copy link
Member Author

sdake commented May 28, 2018

abandoning this PR as most of the work has been done by other folks. I will continue forward to make citadel standalone work properly from the main helm chart.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-rebase Indicates a PR needs to be rebased before being merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants