-
Notifications
You must be signed in to change notification settings - Fork 192
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
Add Helm-based install / uninstall commands #1475
Conversation
f9e3978
to
29b051a
Compare
29b051a
to
1c27243
Compare
1c27243
to
56e8ce5
Compare
56e8ce5
to
9a06568
Compare
9a06568
to
dfccfbf
Compare
dfccfbf
to
a7a1c9d
Compare
a7a1c9d
to
2e33d15
Compare
- Don't fail the installation validation if --encryption flag is not specified. It should behave the same way as if the flag is set to "disabled". - Return the resolved Helm chart from ResolveHelmChartVersion so that Helm can use the chart later. - Expose RESTClientGetter. Helm client needs it. Signed-off-by: Michi Mutsuzaki <michi@isovalent.com>
2e33d15
to
a1c24f8
Compare
a1c24f8
to
03e2960
Compare
03e2960
to
385d2e8
Compare
This is an alternative implementation to manage Cilium installations. It uses Helm actions to manage Cilium instead of generating Kubernetes resources from the Helm chart and managing them using the Kubernetes APIs directly. These commands are disabled by default. Set CILIUM_CLI_MODE environment variable to "helm" to enable these commands. Ref: #1396 Signed-off-by: Michi Mutsuzaki <michi@isovalent.com>
385d2e8
to
fab588f
Compare
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.
✨
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.
This first stage LGTM, some questions below.
@@ -22,7 +22,13 @@ jobs: | |||
installation-and-connectivity: | |||
runs-on: ubuntu-22.04 | |||
timeout-minutes: 40 | |||
strategy: | |||
matrix: | |||
mode: ["classic", "helm"] |
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.
Am I correct in that later on we will drop the classic mode and only have Helm available?
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.
yeah we'll gradually transition to the helm mode, and then deprecate the classic mode ✨
- name: Install Cilium with IPsec Encryption | ||
if: ${{ matrix.mode == 'helm' }} | ||
run: | | ||
kubectl create -n kube-system secret generic cilium-ipsec-keys \ |
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.
Can't we still have this handled by the CLI even in Helm mode?
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.
yeah we could. i do want to have a discussion first though and come up with a general guideline on how cilium-cli should handle resources that are not part of the helm chart.
on the one hand, these flags are very convenient. on the other hand, i think it's a very slippery slope. we might end up adding more and more logic to cilium-cli without carefully considering:
- what resources should be managed by cilium, and what resources should be managed by the user.
- if cilium should be managing the resource, why it's not part of the helm chart.
please review commit by commit. there are 2 commits:
let's implement other commands in separate PRs to keep the PR size reasonable.
ref: #1396