Skip to content

Conversation

@BadisLaffet1
Copy link

Ensures Helm v2-alpha extracts deployment configuration (image, imagePullPolicy, resources) from kustomize output instead of using hardcoded defaults.

Changes

  • chart_converter.go:
    Added parseImageString() helper (parses image strings into repository/tag/digest components with registry port detection) / Updated ExtractDeploymentConfig() (finds manager container by name, extracts image, imagePullPolicy, and resources from actual kustomize deployment).
  • values_basic.go:
    Updated generateBasicValues() (populates values.yaml with extracted kustomize data instead of hardcoded defaults, implements digest-over-tag priority logic, conditional tag/digest inclusion based on actual deployment config).
  • helm_templater.go:
    Implemented templateImageReference() (creates conditional digest/tag templating with {{- if .Values.controllerManager.image.digest }} logic) / Implemented templateResources() (generates dynamic resource templating with precise boundary detection using regex patterns), handles both - name: manager and name: manager container formats.
  • tests:
    Added test assertions validating image parsing, extraction logic, templating behavior, and generated output correctness with specific scenarios for tag/digest formats.

Before: helm install deployed hardcoded controller:latest with static resource limits regardless of kustomize content .
After: helm install deploys exact same image/resources as make build-installer.

Fixes #5113

@k8s-ci-robot k8s-ci-robot added the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label Oct 28, 2025
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: BadisLaffet1
Once this PR has been reviewed and has the lgtm label, please assign varshaprasad96 for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

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

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Oct 28, 2025

CLA Signed
The committers listed above are authorized under a signed CLA.

  • ✅ login: BadisLaffet1 / name: Badis Laffet (a825c88)

@k8s-ci-robot
Copy link
Contributor

Welcome @BadisLaffet1!

It looks like this is your first PR to kubernetes-sigs/kubebuilder 🎉. 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/kubebuilder 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
Copy link
Contributor

Hi @BadisLaffet1. Thanks for your PR.

I'm waiting for a github.com 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.

@k8s-ci-robot k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 28, 2025
@BadisLaffet1
Copy link
Author

/retitle ✨ helm/v2: Extract image and resources from kustomize

@k8s-ci-robot
Copy link
Contributor

@BadisLaffet1: Re-titling can only be requested by trusted users, like repository collaborators.

In response to this:

/retitle ✨ helm/v2: Extract image and resources from kustomize

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.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Oct 28, 2025
@camilamacedo86
Copy link
Member

camilamacedo86 commented Oct 28, 2025

Hi @BadisLaffet1,

It looks great! 🎉

Before we get this one merged, please fix the lint issues — you can run make lint-fix locally to handle that.

Also, it seems the commit message is failing for some reason — could you please take a look at that?

Lastly, once the lint issues are fixed, please squash your commits so we end up with just one commit for merging.

Moreover, please check the comments bellow.

Looking forward to seeing your updates so we can get this one merged soon! 🚀

It("should still include other basic sections", func() {
content := valuesTemplate.GetBody()

Expect(content).To(ContainSubstring("replicaCount:"))
Copy link
Member

Choose a reason for hiding this comment

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

Could we keep it?

content := valuesTemplate.GetBody()

Expect(content).NotTo(ContainSubstring("certManager:"))
Expect(content).NotTo(ContainSubstring("enable: true"))
Copy link
Member

Choose a reason for hiding this comment

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

Could we keep it ?

It("should have correct file permissions", func() {
info := valuesTemplate.GetIfExistsAction()
Expect(info).To(Equal(machinery.OverwriteFile))
Expect(info).To(Equal(machinery.SkipFile))
Copy link
Member

@camilamacedo86 camilamacedo86 Oct 28, 2025

Choose a reason for hiding this comment

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

Why are we changing it ?
We should not change it.

- name: MEMCACHED_IMAGE
value: memcached:1.6.26-alpine3.19
{{- if .Values.controllerManager.image.digest }}
image: "{{ .Values.controllerManager.image.repository }}@{{ .Values.controllerManager.image.digest }}"
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need digest?

@camilamacedo86 camilamacedo86 changed the title ✨ helm/v2: Extract image and resources from kustomize / Fixes #5113 ✨ (helm/v2alpha) Extract image and resources from kustomize Oct 28, 2025
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label Oct 28, 2025
@camilamacedo86
Copy link
Member

If you don’t mind, I’d like to close this PR in favor of #5147
.

We recently introduced helm/v2alpha, which hasn’t been released yet. The changes in that PR address this issue (and a few others) and are required before the release.

Since that work includes a significant refactor, merging this one separately would likely cause conflicts or break things.

I really appreciate you taking the time to look into this and open the PR — thank you! 🙏

My sincere apologies for any inconvenience. I was already reviewing this area but forgot to assign the issue to myself.

@BadisLaffet1
Copy link
Author

Thanks, @camilamacedo86 — totally understand!
I didn’t realize pr 5147 already covered this area.
I’d be glad to support any follow-up work or issues in Kubebuilder. 🙌

@camilamacedo86
Copy link
Member

Hi @BadisLaffet1,

Really sorry about this 🙏

I was initially thinking of keeping the image covered here as well since I didn’t want your effort to go to waste, but after reviewing it more closely, it looks like it would require some refactoring — and that would probably cause a lot of extra work later with rebases and conflict resolution.

I truly appreciate all your effort on this! If you’d like to review or improve this further in a follow-up, that would be amazing — your help is always more than welcome. 💪

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Helm/v2 - Ensure that the values for image and resources came from kustomize

3 participants