🐛 (helm/v2alpha): allow manager values to be customized properly #5147

camilamacedo86 · 2025-10-27T13:20:44Z

This update makes sure the manager Helm chart uses values taken from the
Kustomize deployment files (like install.yaml), instead of using hardcoded
settings. The controller specs are now read from those inputs and passed
into Helm values so the chart renders correctly and matches the deployment.

PS:Follow up of helm-chart v2alpha introduction

Closes; #5114 #5113

camilamacedo86 · 2025-10-27T13:32:34Z

docs/book/src/multiversion-tutorial/testdata/project/dist/chart/values.yaml

    tag: latest
    pullPolicy: IfNotPresent

+  # Arguments for the manager


TODO: Use only Arguments instead to match with ENVVAR too.

camilamacedo86 · 2025-10-28T10:01:32Z

/override pull-kubebuilder-e2e-k8s-1-34-1

k8s-ci-robot · 2025-10-28T10:01:36Z

@camilamacedo86: /override requires failed status contexts, check run or a prowjob name to operate on.
The following unknown contexts/checkruns were given:

pull-kubebuilder-e2e-k8s-1-34-1

Only the following failed contexts/checkruns were expected:

EasyCLA
netlify/kubebuilder/deploy-preview
pull-kubebuilder-e2e-k8s-1-32-0
pull-kubebuilder-e2e-k8s-1-33-0
pull-kubebuilder-e2e-k8s-1-34-0
pull-kubebuilder-test
tide

If you are trying to override a checkrun that has a space in it, you must put a double quote on the context.

In response to this:

/override pull-kubebuilder-e2e-k8s-1-34-1

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.

camilamacedo86 · 2025-10-28T10:30:52Z

/override pull-kubebuilder-e2e-k8s-1-34-0

k8s-ci-robot · 2025-10-28T10:30:56Z

@camilamacedo86: Overrode contexts on behalf of camilamacedo86: pull-kubebuilder-e2e-k8s-1-34-0

In response to this:

/override pull-kubebuilder-e2e-k8s-1-34-0

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.

This update makes sure the manager Helm chart uses values taken from the Kustomize deployment files (like install.yaml), instead of using hardcoded settings. The controller specs are now read from those inputs and passed into Helm values so the chart renders correctly and matches the deployment. PS:Follow up of helm-chart v2alpha introduction Assisted-by: Cursor

camilamacedo86 · 2025-10-28T10:33:22Z

/override pull-kubebuilder-e2e-k8s-1-34-0

k8s-ci-robot · 2025-10-28T10:33:26Z

@camilamacedo86: Overrode contexts on behalf of camilamacedo86: pull-kubebuilder-e2e-k8s-1-34-0

In response to this:

/override pull-kubebuilder-e2e-k8s-1-34-0

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.

camilamacedo86 · 2025-10-28T10:36:55Z

/override pull-kubebuilder-e2e-k8s-1-34-0

k8s-ci-robot · 2025-10-28T10:36:59Z

@camilamacedo86: Overrode contexts on behalf of camilamacedo86: pull-kubebuilder-e2e-k8s-1-34-0

In response to this:

/override pull-kubebuilder-e2e-k8s-1-34-0

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.

camilamacedo86 · 2025-10-28T10:38:20Z

docs/book/src/cronjob-tutorial/testdata/project/dist/chart/templates/manager/manager.yaml

+                    {{- end }}
                    - --health-probe-bind-address=:8081
+                    {{- range .Values.controllerManager.args }}
+                    - {{ . }}


Ensure that we have the possible args exposed now

camilamacedo86 · 2025-10-28T10:38:51Z

docs/book/src/cronjob-tutorial/testdata/project/dist/chart/templates/manager/manager.yaml

-                    - --leader-elect
+                    {{- else }}
+                    # Bind to :0 to disable the controller-runtime managed metrics server
+                    - --metrics-bind-address=:0


Ensure that we properly disable the metrics server in c+r when it is not enabled
(more safe)

camilamacedo86 · 2025-10-28T10:39:09Z

docs/book/src/cronjob-tutorial/testdata/project/dist/chart/templates/manager/manager.yaml

                  command:
                    - /manager
                  image: "{{ .Values.controllerManager.image.repository }}:{{ .Values.controllerManager.image.tag }}"
+                  imagePullPolicy: {{ .Values.controllerManager.image.pullPolicy }}


getting from the helm values it was missing

camilamacedo86 · 2025-10-28T10:39:26Z

docs/book/src/cronjob-tutorial/testdata/project/dist/chart/templates/manager/manager.yaml

+                    {{- toYaml .Values.controllerManager.securityContext | nindent 20 }}
+                    {{- else }}
+                    {}
+                    {{- end }}


getting from helm values it was missing

camilamacedo86 · 2025-10-28T10:39:35Z

docs/book/src/cronjob-tutorial/testdata/project/dist/chart/templates/manager/manager.yaml

+              {{- toYaml .Values.controllerManager.podSecurityContext | nindent 14 }}
+              {{- else }}
+              {}
+              {{- end }}


getting from helm values it was missing

camilamacedo86 · 2025-10-28T10:40:08Z

pkg/plugins/optional/helm/v2alpha/scaffolds/internal/kustomize/chart_converter.go

+	extractContainerImage(container, config)
+	extractContainerArgs(container, config)
+	extractContainerResources(container, config)
+	extractContainerSecurityContext(container, config)


Now we ensure that we extract all values from the manager in the kustomize input.

k8s-ci-robot · 2025-10-28T11:14:55Z

@camilamacedo86: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-kubebuilder-e2e-k8s-1-32-0	`1966560`	link	true	`/test pull-kubebuilder-e2e-k8s-1-32-0`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

liam-mackie

LGTM! Amazing work!
Some minor nits/comments, but they can be ignored if my understanding is incorrect.
I wish there was a better option than all the string and indentation manipulation we need to do when modifying the yaml string, but I guess that's yaml 🤷

liam-mackie · 2025-10-28T10:40:35Z

docs/book/src/cronjob-tutorial/testdata/project/dist/chart/templates/manager/manager.yaml

-                    - --leader-elect
+                    {{- else }}
+                    # Bind to :0 to disable the controller-runtime managed metrics server
+                    - --metrics-bind-address=:0


Suggested change

- --metrics-bind-address=:0

- --metrics-bind-address=0

Though I assume that this is sourced from templates later on in the PR, I believe that the bind address should be 0 instead of :0 based on the metrics-server code here

Amazing cather. Yes

liam-mackie · 2025-10-28T10:49:27Z

pkg/plugins/optional/helm/v2alpha/scaffolds/internal/kustomize/chart_converter.go

+	repository := imageValue
+	tag := "latest"
+	lastColon := strings.LastIndex(imageValue, ":")
+	lastSlash := strings.LastIndex(imageValue, "/")
+	if lastColon != -1 && lastColon > lastSlash {
+		repository = imageValue[:lastColon]
+		if lastColon+1 < len(imageValue) {
+			tag = imageValue[lastColon+1:]
 		}
 	}


Nit: Consider using the parser package implementation if we're safe to include the utils package as a dependency

We try to avoid add deps as much as possible to reduce the burn to keep things maintained.

liam-mackie · 2025-10-28T11:17:44Z

pkg/plugins/optional/helm/v2alpha/scaffolds/internal/kustomize/helm_templater.go

+		builder.WriteString("# Bind to :0 to disable the controller-runtime managed metrics server\n")
+		builder.WriteString(metricsIndent)
+		builder.WriteString("- --metrics-bind-address=:0\n")
+		builder.WriteString(metricsIndent)


As commented in the docs - we should bind to 0 instead to disable I think

liam-mackie · 2025-10-28T11:22:36Z

pkg/plugins/optional/helm/v2alpha/scaffolds/internal/kustomize/helm_templater_test.go

+			Expect(result).To(ContainSubstring("- --health-probe-bind-address=:8081"))
+			Expect(result).To(ContainSubstring("{{- range .Values.controllerManager.args }}"))
+			Expect(result).NotTo(ContainSubstring("BUSYBOX_IMAGE"))
+			Expect(result).NotTo(ContainSubstring("MEMCACHED_IMAGE"))


This doesn't seem to be in the source spec, so this should always evaluate to true.

liam-mackie · 2025-10-28T11:25:25Z

pkg/plugins/optional/helm/v2alpha/scaffolds/internal/kustomize/helm_templater.go

-		}
-		return match
-	})
+	if !strings.Contains(yamlContent, "name: manager") {


Just to confirm, this means that we only allow the values.yaml to be used to specify the manager container image. If there are any sidecar/init containers, etc, we will keep the image as templated in Kustomize.
Is that expected?

. If there are any sidecar/init containers, etc, we will keep the image as templated in Kustomize.
Is that expected?

We do not scaffold any sidecar and etc.
Then, those are customisations made on top.
For it users will need to tweak things afterwards manually. |
We cannot support all possible scenarios.

However, if anyone be able to improve it to accept more scenarios, I am ok with it.

liam-mackie · 2025-10-28T11:30:14Z

pkg/plugins/optional/helm/v2alpha/scaffolds/internal/templates/values_basic_test.go

 			Expect(err).NotTo(HaveOccurred())
 		})

 		It("should include deployment configuration", func() {


Should this include assertions for the env variables we set?

k8s-ci-robot · 2025-10-28T11:33:17Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: camilamacedo86, liam-mackie

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~~ [camilamacedo86]

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

liam-mackie · 2025-10-28T11:33:45Z

/lgtm

k8s-ci-robot · 2025-10-28T11:33:49Z

@liam-mackie: changing LGTM is restricted to collaborators

In response to this:

/lgtm

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.

Copilot

Pull Request Overview

This PR enables proper customization of Helm chart values by extracting controller manager configuration from Kustomize deployment files rather than using hardcoded values. The changes ensure the Helm chart templates correctly reference values.yaml for container image, arguments, environment variables, resources, and security contexts.

Key Changes

Extract deployment configuration (image, args, env, resources, security contexts) from Kustomize output
Template manager.yaml to use .Values.controllerManager.* references instead of hardcoded values
Add args section to values.yaml with --leader-elect as default

Reviewed Changes

Copilot reviewed 6 out of 14 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
pkg/plugins/optional/helm/v2alpha/scaffolds/internal/kustomize/chart_converter.go	Refactored extraction logic to parse image info, args, env, resources, and security contexts from deployment
pkg/plugins/optional/helm/v2alpha/scaffolds/internal/kustomize/helm_templater.go	Added templating functions for args, env, resources, security contexts, and image references
pkg/plugins/optional/helm/v2alpha/scaffolds/internal/templates/values_basic.go	Enhanced values generation to include image config and args section from deployment config
testdata/project-v4-with-plugins/dist/chart/templates/manager/manager.yaml	Converted hardcoded values to Helm template references
testdata/project-v4-with-plugins/dist/chart/values.yaml	Added args section with leader-elect flag
Multiple test files	Updated tests to verify new extraction and templating behavior

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-10-28T17:35:31Z

pkg/plugins/optional/helm/v2alpha/scaffolds/internal/kustomize/helm_templater.go

+}
+
+func leadingWhitespace(line string) (string, int) {
+	trimmed := strings.TrimLeft(line, " 	")


The tab character is represented as a literal whitespace character instead of using an escape sequence. This should be \" \\t\" for better readability and consistency with the rest of the codebase.

Suggested change

trimmed := strings.TrimLeft(line, " ")

trimmed := strings.TrimLeft(line, " \t")

Copilot · 2025-10-28T17:35:31Z

pkg/plugins/optional/helm/v2alpha/scaffolds/internal/kustomize/chart_converter.go

+		}
+
+		// The following arguments should not be exposed under args
+		// manager because they are not alone customizable


The comment contains a grammatical error. 'not alone customizable' should be 'not independently customizable' or 'cannot be customized independently'.

Suggested change

// manager because they are not alone customizable

// manager because they are not independently customizable

k8s-ci-robot requested review from Kavinjsir and varshaprasad96 October 27, 2025 13:20

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 27, 2025

camilamacedo86 force-pushed the args-helm branch 2 times, most recently from 515ab8b to 4b381d1 Compare October 27, 2025 13:30

camilamacedo86 commented Oct 27, 2025

View reviewed changes

camilamacedo86 changed the title ~~✨ (helm/v2alpha): ensure that args are obtained and used in the helm charts~~ WIP: ✨ (helm/v2alpha): ensure that args are obtained and used in the helm charts Oct 27, 2025

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 27, 2025

camilamacedo86 force-pushed the args-helm branch 2 times, most recently from a328ed7 to ff241c8 Compare October 28, 2025 08:53

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 Oct 28, 2025

camilamacedo86 force-pushed the args-helm branch from 6596011 to 77cd6bb Compare October 28, 2025 09:45

camilamacedo86 changed the title ~~WIP: ✨ (helm/v2alpha): ensure that args are obtained and used in the helm charts~~ 🐛 (helm/v2alpha): ensure properly customization of the manager values Oct 28, 2025

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 28, 2025

camilamacedo86 changed the title ~~🐛 (helm/v2alpha): ensure properly customization of the manager values~~ 🐛 (helm/v2alpha): ensure properly customization of the manager values ( follow up implementation of helm/v2alpha ) Oct 28, 2025

camilamacedo86 changed the title ~~🐛 (helm/v2alpha): ensure properly customization of the manager values ( follow up implementation of helm/v2alpha )~~ 🐛 (helm/v2alpha): ensure properly customization of the manager values ( follow up implementation of helm/v2alpha ) Oct 28, 2025

camilamacedo86 force-pushed the args-helm branch 2 times, most recently from 5cff21e to 6d2155a Compare October 28, 2025 10:25

camilamacedo86 changed the title ~~🐛 (helm/v2alpha): ensure properly customization of the manager values ( follow up implementation of helm/v2alpha )~~ 🐛 (helm/v2alpha): (helm/v2alpha): allow manager values to be customized properly Oct 28, 2025

camilamacedo86 force-pushed the args-helm branch from 6d2155a to 62d91f9 Compare October 28, 2025 10:30

camilamacedo86 changed the title ~~🐛 (helm/v2alpha): (helm/v2alpha): allow manager values to be customized properly~~ 🐛 (helm/v2alpha): allow manager values to be customized properly Oct 28, 2025

camilamacedo86 added the release-blocker label Oct 28, 2025

camilamacedo86 force-pushed the args-helm branch from 62d91f9 to 1966560 Compare October 28, 2025 10:33

camilamacedo86 commented Oct 28, 2025

View reviewed changes

camilamacedo86 added this to the helm milestone Oct 28, 2025

liam-mackie approved these changes Oct 28, 2025

View reviewed changes

camilamacedo86 requested a review from Copilot October 28, 2025 17:24

Copilot AI reviewed Oct 28, 2025

View reviewed changes

	trimmed := strings.TrimLeft(line, " ")
	trimmed := strings.TrimLeft(line, " \t")

	// manager because they are not alone customizable
	// manager because they are not independently customizable

Uh oh!

🐛 (helm/v2alpha): allow manager values to be customized properly #5147

Are you sure you want to change the base?

🐛 (helm/v2alpha): allow manager values to be customized properly #5147

Conversation

camilamacedo86 commented Oct 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

camilamacedo86 commented Oct 28, 2025

Uh oh!

k8s-ci-robot commented Oct 28, 2025

Uh oh!

camilamacedo86 commented Oct 28, 2025

Uh oh!

k8s-ci-robot commented Oct 28, 2025

Uh oh!

camilamacedo86 commented Oct 28, 2025

Uh oh!

k8s-ci-robot commented Oct 28, 2025

Uh oh!

camilamacedo86 commented Oct 28, 2025

Uh oh!

k8s-ci-robot commented Oct 28, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Oct 28, 2025

Uh oh!

liam-mackie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Oct 28, 2025

Uh oh!

liam-mackie commented Oct 28, 2025

Uh oh!

k8s-ci-robot commented Oct 28, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Key Changes

Reviewed Changes

Uh oh!

Copilot AI Oct 28, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 28, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

camilamacedo86 commented Oct 27, 2025 •

edited

Loading