-
Notifications
You must be signed in to change notification settings - Fork 226
feat!: upgrade to Helm 4 #4350
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
base: main
Are you sure you want to change the base?
feat!: upgrade to Helm 4 #4350
Conversation
Signed-off-by: Austin Abro <[email protected]>
Signed-off-by: Austin Abro <[email protected]>
✅ Deploy Preview for zarf-docs canceled.
|
Signed-off-by: Austin Abro <[email protected]>
Signed-off-by: Austin Abro <[email protected]>
Signed-off-by: Austin Abro <[email protected]>
Signed-off-by: Austin Abro <[email protected]>
Signed-off-by: Austin Abro <[email protected]>
Signed-off-by: Austin Abro <[email protected]>
Signed-off-by: Austin Abro <[email protected]>
Signed-off-by: Austin Abro <[email protected]>
Signed-off-by: Austin Abro <[email protected]>
Signed-off-by: Austin Abro <[email protected]>
Signed-off-by: Austin Abro <[email protected]>
Signed-off-by: Austin Abro <[email protected]>
Signed-off-by: Austin Abro <[email protected]>
Signed-off-by: Austin Abro <[email protected]>
Signed-off-by: Austin Abro <[email protected]>
Signed-off-by: Austin Abro <[email protected]>
Signed-off-by: Austin Abro <[email protected]>
Signed-off-by: Austin Abro <[email protected]>
Signed-off-by: Austin Abro <[email protected]>
Signed-off-by: Austin Abro <[email protected]>
Signed-off-by: Austin Abro <[email protected]>
Signed-off-by: Austin Abro <[email protected]>
Signed-off-by: Austin Abro <[email protected]>
Signed-off-by: Austin Abro <[email protected]>
Signed-off-by: Austin Abro <[email protected]>
Signed-off-by: Austin Abro <[email protected]>
Signed-off-by: Austin Abro <[email protected]>
Signed-off-by: Austin Abro <[email protected]>
Signed-off-by: Austin Abro <[email protected]>
Signed-off-by: Austin Abro <[email protected]>
| {{ if not .Values.autoscaling.enabled }} | ||
| replicas: {{ .Values.replicaCount }} | ||
| {{ end }} |
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.
The reason this was added is because with server side apply if autoscaling is enabled then it owns the replicas field, so we will run into an error if we re-apply the chart with a set amount of replicas and auto-scaling.
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.
While not a zarf issue directly (more k8s migration to SSA issue) - this will be exposed to through zarf to consumers.
Are there other known instances of this ownership issue and do we need to document any of them for users to discover?
Signed-off-by: Austin Abro <[email protected]>
Signed-off-by: Austin Abro <[email protected]>
Signed-off-by: Austin Abro <[email protected]>
Signed-off-by: Austin Abro <[email protected]>
Signed-off-by: Austin Abro <[email protected]>
brandtkeller
left a comment
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.
Looking over helm generically in this pass - continuing to dig into the integration. Couple minor questions for helm is isolation.
Excited to see helm vendored fully.
Signed-off-by: Austin Abro <[email protected]>
Signed-off-by: Austin Abro <[email protected]>
Signed-off-by: Austin Abro <[email protected]>
Signed-off-by: Austin Abro <[email protected]>
brandtkeller
left a comment
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.
Review nearly complete - working through a few permutations of local testing. Otherwise I think you nailed this. My comments are largely either questions for my education or reinforcing improvements.
| useSSA := zarfChart.ServerSideApply != "false" | ||
| client.ServerSideApply = useSSA | ||
| // Only force conflicts if SSA is enabled and the option is set | ||
| client.ForceConflicts = useSSA && opts.ForceConflicts |
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 like this distinction for compatibility - given backwards CSA potential.
| } | ||
| client.ServerSideApply = zarfChart.ServerSideApply | ||
| // Only force conflicts if SSA is enabled and the option is set | ||
| useSSA := zarfChart.ServerSideApply != "false" && zarfChart.ServerSideApply != "" |
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.
Is the latter conditional possible given the preceding lines?
| useSSA := zarfChart.ServerSideApply != "false" && zarfChart.ServerSideApply != "" | |
| useSSA := zarfChart.ServerSideApply != "false" |
| } | ||
|
|
||
| func rollbackChart(name string, version int, actionConfig *action.Configuration, timeout time.Duration) error { | ||
| func rollbackChart(zarfChart v1alpha1.ZarfChart, version int, actionConfig *action.Configuration, timeout time.Duration) error { |
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.
Is there a permutation of scenarios where we need to set client.ForceConflicts on rollback?
| func migrateDeprecatedAPIs(ctx context.Context, c *cluster.Cluster, actionConfig *action.Configuration, latestRelease *release.Release) error { | ||
| func migrateDeprecatedAPIs(ctx context.Context, c *cluster.Cluster, actionConfig *action.Configuration, latestReleaser release.Releaser) error { | ||
| // We can re-evaluate handling this functionality for chart v3 / release v2 when available. | ||
| // Potentially we stop doing this functionality for future chart APIs, as it could be outside of the intended scope of Zarf. |
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 would agree - I believe this workflow also incurs a decent dependency footprint if I recall correctly.
| }(l) | ||
|
|
||
| saved, _, err := chartDownloader.DownloadTo(chartURL, pull.Version, temp) | ||
| saved, _, err := chartDownloader.DownloadToCache(chartURL, pull.Version) |
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.
neat improvement under-the-hood for zarf (caching).
Description
User facing changes:
zarf tools helmis now a true copy of the Helm CLI. Previously, this command was a quasi fork of a subset of the Helm CLI and was not updated automatically. This functionality was made possible through a Zarf maintainer contribution #13617zarf dev find-imagesno longer includes images from Helm testsWe will have to be very careful about merging this as there may be impacts to charts that we do not test for in the migration from Helm 3 to Helm 4. We want the experience to be seamless for users. I intend to make a R.C. release from this branch before merging into main.
Note that vendoring in the entirety of Helm instead of the current quasi fork bring binary size up about 5MB from 173M on main to 178M on this branch
Something interesting is that server side apply default to true during new installs, but "auto" during upgrades and rollbacks meaning that existing charts will use the existing strategy. I decided to add a new addition to the schema, serverSideApply, which accepts the field "true", "false", and "auto".
zarf dev find-imagesandzarf dev inspect manifestsno longer include files helm tests (from the chart/templates/test directory).`. I believe this is a good, since Zarf doesn't have a way to run these tests anyway so we now exclude unnecessary images. Would look to understand the reasoningRelated Issue
Fixes #2818
Fixes #2756
Fixes #4437
Checklist before merging