⚠️ Make helm chart values extensible per component #638

rbjorklin · 2024-11-25T23:20:38Z

What this PR does / why we need it:
When deploying clusters to AKS using Workload Identity an annotation is required on the service account and a label is required on the pod. By extending the chart to expose the already existing functionality to patch provider manifests this PR allows for setting up AKS clusters using Workload Identity.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

k8s-ci-robot · 2024-11-25T23:20:47Z

Welcome @rbjorklin!

It looks like this is your first PR to kubernetes-sigs/cluster-api-operator 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/cluster-api-operator has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2024-11-25T23:20:47Z

Hi @rbjorklin. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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-sigs/prow repository.

netlify · 2024-11-25T23:20:58Z

✅ Deploy Preview for kubernetes-sigs-cluster-api-operator ready!

Name	Link
🔨 Latest commit	`02019c7`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-cluster-api-operator/deploys/67e1e7ab17301c0008a002e2
😎 Deploy Preview	https://deploy-preview-638--kubernetes-sigs-cluster-api-operator.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

furkatgofurov7 · 2025-01-27T10:45:55Z

/ok-to-test

furkatgofurov7 · 2025-02-06T21:23:41Z

@rbjorklin hey, would you be able to look into failing CI tests and fix them, so that we can start reviewing it ?

rbjorklin · 2025-02-12T23:05:12Z

@rbjorklin hey, would you be able to look into failing CI tests and fix them, so that we can start reviewing it ?

Fixed, please take a look!

hack/charts/cluster-api-operator/values.yaml

rbjorklin · 2025-02-19T19:13:58Z

@furkatgofurov7 given that this changes the structure of values.yaml should I introduce a values.schema.json to make consumers of the chart aware of the change when they try rendering it?

furkatgofurov7 · 2025-02-21T07:52:15Z

@furkatgofurov7 given that this changes the structure of values.yaml should I introduce a values.schema.json to make consumers of the chart aware of the change when they try rendering it?

@rbjorklin good idea, it seems to be useful when using helm lint?

furkatgofurov7 · 2025-02-21T13:09:58Z

cc @alexander-demicev @Danil-Grigorev @Fedosin

rbjorklin · 2025-03-07T23:51:26Z

@furkatgofurov7 I have just pushed a minimal values.schema.json to catch the breaking changes to the values.yaml file introduced in this PR. When trying to template the chart a user will be faced with something like this:

❯ helm template test hack/charts/cluster-api-operator
Error: values don't meet the specifications of the schema(s) in the following chart(s):
cluster-api-operator:
- bootstrap: Invalid type. Expected: array, given: string
- controlPlane: Invalid type. Expected: array, given: string
- infrastructure: Invalid type. Expected: array, given: string
- addon: Invalid type. Expected: array, given: string
- ipam: Invalid type. Expected: array, given: string
- core: Invalid type. Expected: object, given: string

hack/charts/cluster-api-operator/values.schema.json

rbjorklin · 2025-03-19T19:53:52Z

@furkatgofurov7 @afarbos reworked and tests are passing. Please take a look! 🙏

This introduces a minimal values.schema.json file to catch the breaking structural changes made to the values.yaml file in the related commits to this PR. This will cause the 'helm template' or 'helm lint' commands to throw errors and require consumers of the chart to fix their values.yaml before deploying.

This is required because iterating over a list and a dict does not necessarily result in the same order of elements.

rbjorklin · 2025-03-25T00:37:00Z

I've addressed the review comment by @afarbos. I'd like to believe that this is in merge:able shape now!

furkatgofurov7 · 2025-03-25T09:28:21Z

hack/charts/cluster-api-operator/values.yaml

-core: ""
-bootstrap: ""
-controlPlane: ""
-infrastructure: ""
-ipam: ""
-addon: ""


So, now options have been updated to use structured objects for providers instead of plain strings. Could it be potentially a breaking change since existing deployments rely on the old string format and need to be updated to the new format?

It is, this would be a major upgrade for the helm chart.

Yeah this is a breaking change which is why I have put the ⚠️ symbol on the PR. It is also why I introduced the values.schema.json file as that will catch the breaking change and provide an appropriate warning when users try to render. This is what they will see.

afarbos · 2025-03-25T18:40:12Z

hack/charts/cluster-api-operator/values.yaml

-core: ""
-bootstrap: ""
-controlPlane: ""
-infrastructure: ""
-ipam: ""
-addon: ""


It is, this would be a major upgrade for the helm chart.

afarbos · 2025-03-25T18:43:12Z

hack/charts/cluster-api-operator/templates/addon.yaml

+{{- range $name, $addon := $.Values.addon }}
+  {{- $addonNamespace := default ( printf "%s-%s" $name "addon-system" ) (get $addon "namespace") }}
+  {{- $addonName := $name }}
+  {{- $addonVersion := get $addon "version" }}


Suggested change

{{- $addonVersion := get $addon "version" }}

{{- $addonVersion := get $addon "version" }}

{{- $addonEnabled := default (get $addon "enabled") true }}

{{- if not $addonEnabled }}

{{- continue }}

{{- end }}

I would have use enabled, no?

I figured rangeing over the the dict was sufficient. If the dict is empty no manifests will be created. I can add the enabled check if you prefer though.

Sidenote: Helm does not actually have a continue keyword.

alexander-demicev

/approve

k8s-ci-robot · 2025-04-04T12:35:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alexander-demicev

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:

~~OWNERS~~ [alexander-demicev]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

furkatgofurov7

Thanks @rbjorklin for patience and pushing this to the finish line.

/retitle ⚠️ Make helm chart values extensible per component
/lgtm

k8s-ci-robot · 2025-04-04T13:08:00Z

LGTM label has been added.

Git tree hash: 4d06ee6100c17372717d5b17a680038d8669e43f

furkatgofurov7 · 2025-04-25T08:31:14Z

@rbjorklin hey, I noticed we forgot to have some docs around this breaking change. Would you be willing to contribute to document it, which would greatly help existing/new users facing challenges with the new values schema.

k8s-ci-robot requested review from damdo and Danil-Grigorev November 25, 2024 23:20

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 25, 2024

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 25, 2024

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 27, 2025

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 12, 2025

rbjorklin force-pushed the revamp-chart branch from 0eeb087 to d297aca Compare February 12, 2025 19:49

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 12, 2025

rbjorklin force-pushed the revamp-chart branch 3 times, most recently from 1691bc3 to d22f12b Compare February 12, 2025 22:38

rbjorklin commented Feb 18, 2025

View reviewed changes

hack/charts/cluster-api-operator/values.yaml Outdated Show resolved Hide resolved

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 7, 2025

rbjorklin force-pushed the revamp-chart branch from b7f6424 to bbdf09c Compare March 7, 2025 23:56

rbjorklin commented Mar 7, 2025

View reviewed changes

hack/charts/cluster-api-operator/values.schema.json Outdated Show resolved Hide resolved

rbjorklin force-pushed the revamp-chart branch from bbdf09c to 18a9903 Compare March 8, 2025 00:09

k8s-ci-robot removed the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Mar 8, 2025

furkatgofurov7 moved this to PR to be reviewed in CAPI / Turtles Mar 24, 2025

furkatgofurov7 added this to CAPI / Turtles Mar 24, 2025

rbjorklin force-pushed the revamp-chart branch from c5b20c1 to bdfb268 Compare March 24, 2025 23:03

rbjorklin added 3 commits March 24, 2025 16:15

fix: align test value declaration with chart changes

f23f00c

fix: reorder namespace & infraProvider to please test suite

02019c7

This is required because iterating over a list and a dict does not necessarily result in the same order of elements.

rbjorklin force-pushed the revamp-chart branch from bdfb268 to 02019c7 Compare March 24, 2025 23:15

furkatgofurov7 requested a review from afarbos March 25, 2025 09:06

furkatgofurov7 reviewed Mar 25, 2025

View reviewed changes

afarbos reviewed Mar 25, 2025

View reviewed changes

salasberryfin moved this from PR to be reviewed to Blocked in CAPI / Turtles Apr 1, 2025

alexander-demicev moved this from Blocked to PR to be reviewed in CAPI / Turtles Apr 4, 2025

alexander-demicev approved these changes Apr 4, 2025

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 4, 2025

furkatgofurov7 reviewed Apr 4, 2025

View reviewed changes

k8s-ci-robot changed the title ~~⚠️ ✨ Make helm chart values extensible per component~~ ⚠️ Make helm chart values extensible per component Apr 4, 2025

k8s-ci-robot assigned furkatgofurov7 Apr 4, 2025

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 4, 2025

k8s-ci-robot merged commit 215e425 into kubernetes-sigs:main Apr 4, 2025
15 of 16 checks passed

github-project-automation bot moved this from PR to be reviewed to Done in CAPI / Turtles Apr 4, 2025

rbjorklin mentioned this pull request Apr 5, 2025

✨ Add deployment overrides to templates #525

Closed

furkatgofurov7 mentioned this pull request Apr 8, 2025

Helm chart to support to override deployment for all providers #516

Closed

rbjorklin mentioned this pull request Apr 8, 2025

✨ 🌱 Make configSecret configurable per component #780

Open

rbjorklin mentioned this pull request Apr 25, 2025

📖 fix helm install instructions #794

Merged

furkatgofurov7 mentioned this pull request May 5, 2025

How can I change provider container images when installing via Helm? #807

Open

-  {{- $addonVersion := get $addon "version" }}
+  {{- $addonVersion := get $addon "version" }}
+  {{- $addonEnabled := default (get $addon "enabled") true }}
+  {{- if not $addonEnabled }}
+  {{- continue }}
+  {{- end }}

⚠️ Make helm chart values extensible per component #638

⚠️ Make helm chart values extensible per component #638

Uh oh!

Conversation

rbjorklin commented Nov 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Nov 25, 2024

Uh oh!

k8s-ci-robot commented Nov 25, 2024

Uh oh!

netlify bot commented Nov 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for kubernetes-sigs-cluster-api-operator ready!

Uh oh!

furkatgofurov7 commented Jan 27, 2025

Uh oh!

furkatgofurov7 commented Feb 6, 2025

Uh oh!

rbjorklin commented Feb 12, 2025

Uh oh!

Uh oh!

rbjorklin commented Feb 19, 2025

Uh oh!

furkatgofurov7 commented Feb 21, 2025

Uh oh!

furkatgofurov7 commented Feb 21, 2025

Uh oh!

rbjorklin commented Mar 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

rbjorklin commented Mar 19, 2025

Uh oh!

rbjorklin commented Mar 25, 2025

Uh oh!

furkatgofurov7 Mar 25, 2025

Choose a reason for hiding this comment

Uh oh!

afarbos Mar 25, 2025

Choose a reason for hiding this comment

Uh oh!

rbjorklin Mar 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

afarbos Mar 25, 2025

Choose a reason for hiding this comment

Uh oh!

afarbos Mar 25, 2025

Choose a reason for hiding this comment

Uh oh!

rbjorklin Mar 25, 2025

Choose a reason for hiding this comment

Uh oh!

alexander-demicev left a comment

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Apr 4, 2025

Uh oh!

furkatgofurov7 left a comment

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Apr 4, 2025

Uh oh!

Uh oh!

furkatgofurov7 commented Apr 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

rbjorklin commented Nov 25, 2024 •

edited

Loading

netlify bot commented Nov 25, 2024 •

edited

Loading

rbjorklin commented Mar 7, 2025 •

edited

Loading

rbjorklin Mar 25, 2025 •

edited

Loading

furkatgofurov7 commented Apr 25, 2025 •

edited

Loading