Skip to content
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: Add support for remote chart version #1078

Merged
merged 2 commits into from
Sep 19, 2022

Conversation

sayboras
Copy link
Member

@sayboras sayboras commented Sep 4, 2022

Description

Currently, the cilium charts are embedded inside cilium cli binary,
which will require a new cilium/charts bump in go.mod and then a new
release for cilium-cli. It's making sense to do so if some other code
changes are required to make installation working for new char version,
but most of the time, only go.mod upgrade is required.

This commit is to add the capability to install new chart version without
the need of releasing or installing new cilium-cli.

Signed-off-by: Tam Mach [email protected]

Testing

The changes are tested on top of commit 4fc8b22 (i.e. v1.13.0-rc0 is not
supported).

# Trying to download invalid version v1.14.0
$ ./cilium install --version 1.14.0   
chart "cilium" version "1.14.0" not found in https://helm.cilium.io repository

# Trying to install v1.13.0-rc0
$ ./cilium install --version 1.13.0-rc0
🔮 Auto-detected Kubernetes kind: minikube
✨ Running "minikube" validation checks
✅ Detected minikube version "1.26.1"
ℹ️  Using Cilium version 1.13.0-rc0
..

$ ./cilium status
    /¯¯\
 /¯¯\__/¯¯\    Cilium:         OK
 \__/¯¯\__/    Operator:       OK
 /¯¯\__/¯¯\    Hubble:         disabled
 \__/¯¯\__/    ClusterMesh:    disabled
    \__/

Deployment        cilium-operator    Desired: 1, Ready: 1/1, Available: 1/1
DaemonSet         cilium             Desired: 1, Ready: 1/1, Available: 1/1
Containers:       cilium             Running: 1
                  cilium-operator    Running: 1
Cluster Pods:     1/1 managed by Cilium
Image versions    cilium             quay.io/cilium/cilium-ci:latest: 1
                  cilium-operator    quay.io/cilium/operator-generic-ci:latest: 1
                  
 $ ./cilium hubble enable           
🔑 Found CA in secret cilium-ca
...
✨ Patching ConfigMap cilium-config to enable Hubble...
🚀 Creating ConfigMap for Cilium version 1.13.0-rc0...
♻️  Restarted Cilium pods
               

@sayboras sayboras temporarily deployed to ci September 4, 2022 10:38 Inactive
@sayboras sayboras force-pushed the tam/dynamic-helm-version branch from 6ed0b77 to f40adea Compare September 4, 2022 11:31
@sayboras sayboras temporarily deployed to ci September 4, 2022 11:31 Inactive
@sayboras sayboras force-pushed the tam/dynamic-helm-version branch from f40adea to 09199d4 Compare September 4, 2022 11:36
@sayboras sayboras temporarily deployed to ci September 4, 2022 11:36 Inactive
@sayboras sayboras force-pushed the tam/dynamic-helm-version branch from 09199d4 to 2b9f0ea Compare September 4, 2022 11:46
@sayboras sayboras temporarily deployed to ci September 4, 2022 11:46 Inactive
@sayboras sayboras marked this pull request as ready for review September 4, 2022 23:36
@sayboras sayboras requested a review from a team as a code owner September 4, 2022 23:36
@sayboras sayboras requested review from squeed and removed request for a team September 4, 2022 23:36
internal/helm/helm.go Outdated Show resolved Hide resolved
@sayboras sayboras force-pushed the tam/dynamic-helm-version branch from 2b9f0ea to 6f1a22c Compare September 5, 2022 10:46
@sayboras sayboras temporarily deployed to ci September 5, 2022 10:46 Inactive
@sayboras sayboras requested a review from kaworu September 5, 2022 10:47
Copy link
Member

@kaworu kaworu left a comment

Choose a reason for hiding this comment

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

Thanks for the update @sayboras. I took a closer look, and I still have a question about double fetching the charts (couple of nits on the way).

internal/helm/helm.go Show resolved Hide resolved
internal/helm/helm.go Show resolved Hide resolved
internal/helm/helm.go Outdated Show resolved Hide resolved
internal/helm/helm.go Outdated Show resolved Hide resolved
@squeed
Copy link
Contributor

squeed commented Sep 5, 2022

Does it make sense for cilium-cli to maintain a cache of downloaded charts? The standard is for it to live in $XDG_CACHE_HOME, defaulting to ~/.cache.

@sayboras
Copy link
Member Author

sayboras commented Sep 7, 2022

Does it make sense for cilium-cli to maintain a cache of downloaded charts? The standard is for it to live in $XDG_CACHE_HOME, defaulting to ~/.cache.

Good point, this will help to avoid double download (into temp dir) as what Alex mentioned above.

I will adjust the patch soon.

@sayboras sayboras marked this pull request as draft September 7, 2022 11:23
This commit is to rename newChartFromCiliumVersion function to
newChartFromEmbeddedFile.

Signed-off-by: Tam Mach <[email protected]>
Currently, the cilium charts are embedded inside cilium cli binary,
which will require a new cilium/charts bump in go.mod and then a new
release for cilium-cli. It's making sense to do so if some other code
changes are required to make installation working for new chart version,
but most of the time, only go.mod upgrade is required.

This commit is to add the capability to install new chart version without
the need of releasing or installing new cilium-cli. The new chart will
be downloaded and stored in user cache dir.

Signed-off-by: Tam Mach <[email protected]>
@sayboras sayboras force-pushed the tam/dynamic-helm-version branch from 6f1a22c to 9262f50 Compare September 10, 2022 14:23
@sayboras sayboras temporarily deployed to ci September 10, 2022 14:23 Inactive
@sayboras sayboras requested review from kaworu and squeed September 10, 2022 14:25
@sayboras sayboras marked this pull request as ready for review September 11, 2022 12:43
Copy link
Contributor

@squeed squeed left a comment

Choose a reason for hiding this comment

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

Looks great!

Copy link
Member

@kaworu kaworu left a comment

Choose a reason for hiding this comment

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

Thanks @sayboras!

internal/helm/helm.go Show resolved Hide resolved
@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Sep 19, 2022
@tklauser tklauser merged commit 7fb8e1f into cilium:master Sep 19, 2022
@sayboras sayboras deleted the tam/dynamic-helm-version branch September 19, 2022 08:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge This PR has passed all tests and received consensus from code owners to merge.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants