-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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.
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
Helm V3 support #5385
Helm V3 support #5385
Conversation
Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA. It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.
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. |
Welcome @lohmag! |
/retest |
/check-cla |
@lohmag: Cannot trigger testing until a trusted user reviews the PR and leaves an In response to this:
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. |
/assign @mattymo |
include_tasks: "gen_helm_tiller_certs.yml" | ||
when: tiller_enable_tls | ||
|
||
- name: Helm | Install client on all masters |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this task should not be removed but modified to simply add stable repo when helm_stable_repo_url
is defined?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't get a point of this. If it doesn't need tiller then what for it repo in chart? I think to just delete helm_stable_repo_url
in next commit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In pre-v3, helm init
not only installed tiller but also initiated the helm client on members of the kube-master group (optionally including adding stable repo). That made helm directly usable to install charts from stable repo on masters for users who chose to enable helm_stable_repo_url
in their inventory.
Your changes make the current behaviour change.
/check-cla |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed odsolete vars
include_tasks: "gen_helm_tiller_certs.yml" | ||
when: tiller_enable_tls | ||
|
||
- name: Helm | Install client on all masters |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't get a point of this. If it doesn't need tiller then what for it repo in chart? I think to just delete helm_stable_repo_url
in next commit
/lgtm /approve |
Since Helm 3 has been released every recently, I think it's too early to include that in 2.12, therefore I mark it for 2.13. |
@Miouge1 Do you know why the PR has been merged despite my lgtm and approve? I guess it's because the PR may be based on an earlier commit of master than the one with me as approver, but your last comment made me doubt it... |
@mirwan the OWNERS file changes take effect immediately, I think it's because the commands have to be one per line. I would prefer to hold this for 2.13 but it's up for discussion. |
Is there a due date for 2.12 or is it when the related issues are done, just out of curiosity? Anyway, I'm fine with it. |
Yep one per line. |
/lgtm |
/hold |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lohmag, mirwan 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 |
@lohmag It seems that the support of helm v2 (as well as v3) would be a requirement until some other release so that users can migrate when they want. |
@mirwan Sounds ok except for those part where it should cleanup tiller pods when helm version is not v2. It's not that easy because it need to create secrets for helm3 from helm2 tiller and etcd configs. |
@mirwan I think it will be nice to add |
Anything I should add in the 2.12 release notes (see #5409) around Helm 2 and 3? |
@Miouge1 Not ready yet |
I'll copy part of the Slack discussion related to Helm v3 and how this is a bit different than #5441
|
@lohmag: 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. |
Thanks everyone involved in this PR, we just merged #5503 instead. |
Patch to support helm3, it doesn't need tiller anymore so this part is obsolete.
Several commands also changed. Helm2 not backward compatible with 3 version.
#4777