fix: local helm chart with version but no repo

[ardikabs]

Fixes #5163.

Problem Statement:

I have a use case working with Kustomize and the Helm chart in the ArgoCD, but apparently updating the Helm chart version didn't work as expected. Using Kustomize v5.0.2 actually solved our case, however, the related fix has been reverted because of breaking changes as detailed in #5163 (comment).

I am borrowing the codes from this PR, with adding another check from the repo field, assuming that the use of the repo is considered referring to a remote helm chart.

[k8s-ci-robot]

This PR has multiple commits, and the default merge method is: merge.
You can request commits to be squashed using the label: tide/merge-method-squash

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/test-infra repository.

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: dlish / name: Dustin Lish (a0f131c)
• ✅ login: ardikabs / name: Ardika Bagus S. (cc35d3c, 5505af4, b7b85b1, 790dbf0, 6640f87)

[k8s-ci-robot]

Welcome @ardikabs!

It looks like this is your first PR to kubernetes-sigs/kustomize 🎉. 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/kustomize 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]

Hi @ardikabs. 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/test-infra repository.

[charles-chenzz]

hi @ardikabs can you refer #5293 (comment) to sign the cla first? so that we could proceed your PR. Thanks!

[ardikabs]

hi @ardikabs can you refer #5293 (comment) to sign the cla first? so that we could proceed your PR. Thanks!

Hi @charles-chenzz , signed, thank you!

[charles-chenzz]

/ok-to-test

[ardikabs]

/cc @koba1t @yuwenma @natasha41575

[natasha41575]

@charles-chenzz Were you planning to review this PR? That would be super helpful

[charles-chenzz]

/cc
/assign

[charles-chenzz]

do an initial review, will let other reviews dives in and see what they say

[ardikabs]

HI @ardikabs Thanks for your contribution!

I think this PR looks almost good!

So, I have one comment. Could you add any tests when one helmGlobal contains a few helm charts of the same repos and different versions like here?

Hi @koba1t , since I borrow the same code from this PR #4999, the test case is already there, see https://github.com/kubernetes-sigs/kustomize/pull/5293/files#diff-a5f02292d09cefaf3346f1bed0464c9a0aac016e1c00cdfe4e3e95a83f104d79R631

[koba1t]

@ardikabs

I borrow the same code from this PR #4999, the test case is already there, see https://github.com/kubernetes-sigs/kustomize/pull/5293/files#diff-a5f02292d09cefaf3346f1bed0464c9a0aac016e1c00cdfe4e3e95a83f104d79R631

Yes, I understand that testing multiple versions of helmChart.
But I think this test case runs the helm generator multiple times, like defining multiple HelmCharts in kustomization.yaml files.
I just want to add another test that one HelmCharts contains some HelmChart configured of multiple versions.

• HelmCharts fields may contain multiple HelmChart

I believe that the test may add outside of HelmChartInflationGenerator_test.go file.

[ardikabs]

Yes, I understand that testing multiple versions of helmChart. But I think this test case runs the helm generator multiple times, like defining multiple HelmCharts in kustomization.yaml files. I just want to add another test that one HelmCharts contains some HelmChart configured of multiple versions.

• HelmCharts fields may contain multiple HelmChart

I believe that the test may add outside of HelmChartInflationGenerator_test.go file.

ah I see, got your point @koba1t, aside from adding another test for multiple helm charts as you mentioned outside of the plugin directory, are we already aligned on the test case for the HelmChartInflationGenerator?

[koba1t]

@ardikabs
Thanks for the fix!
This PR looks almost good to me!

So, This PR has a conflict file from the master branch. Could you rebase to master?
I think We can merge this PR after resolving this conflict.

[ardikabs]

@koba1t , I've addressed the conflict after the rebase. Thanks!

[koba1t]

@ardikabs
Thanks!

[koba1t]

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ardikabs, koba1t

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 [koba1t]

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

[farioas]

I believe this is not a backwards compatible change, as the directory where the chart is saved has now changed.
Because of this:

	if p.Version != "" && p.Repo != "" {
		return filepath.Join(chartHome, fmt.Sprintf("%s-%s", p.Name, p.Version))
	}

It's no longer possible to store a helm chart in the same folder.

[ardikabs]

@farioas , could you show the example of the manifest?

because this PR use case is when you are expected to use a chart from remote, but when you expect to refer chart in the same folder, you don't need to provide repo field, thus it will work as it is to refer chart in a relative or absolute path from the kustomization file.

It means this changes should be backward compatible if we refer to using a local chart (without repo).
See this test.

[farioas]

Sure. Here's the real world example.

I have base dir which is used to store charts:

apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization
helmGlobals:
  chartHome: .
helmCharts:
  - name: selenium-grid
    repo: https://www.selenium.dev/docker-selenium
    version: 0.25.3

So, when I run kustomize build --load-restrictor=LoadRestrictionsNone --enable-helm . the helm chart was downloaded in the folder selenium-grid. And it's quit useful to update it regularly, I just change the version and run the same command.

On another "overlay" layer I have this:

apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization
helmGlobals:
  chartHome: ../../../../../../base/charts/selenium-grid
helmCharts:
  - name: selenium-grid
    releaseName: selenium-grid
    namespace: selenium-grid # keep in sync with .namespace
    valuesFile: ../../../../../../base/charts/selenium-grid/values.yaml
    repo: https://www.selenium.dev/docker-selenium
    includeCRDs: true
    additionalValuesFiles:
      - values.yaml
    apiVersions:
      - networking.k8s.io/v1/Ingress

resources:
  - secret.yaml

patches:
  - path: scaledjob.yaml
  - target:
      kind: ScaledJob
      name: selenium-chrome-node
    patch: |-
      - op: replace
        path: /spec/jobTargetRef/template/spec/containers/0/lifecycle
        value:
          preStop:
            exec:
              command:
                - bash
                - -c
                - |
                  while curl 127.0.0.1:5555/status; do sleep 1; done;

namespace: selenium-grid

After this change these paths will change on each helm chart release, so whenever I update the helm chart, I have to overhaul over 100+ folders to append the version.

  chartHome: ../../../../../../base/charts/selenium-grid
...
  valuesFile: ../../../../../../base/charts/selenium-grid/values.yaml

[ardikabs]

@farioas
it seems to me that yes it was not backward compatible for your scenario, but your implementation has a drawback, which the PR is trying to solve, it was the upgrade problem, so when you need to upgrade the helm chart for example, the selenium-grid from 0.25.3 to 0.25.4, then you need to delete the base folder first, then redownload the helm chart again.

This PR, unfortunately, doesn't align to fulfill your scenario but solves the upgrading of the helm chart version intuitively (without the need to delete the chart folder).

[koba1t]

@farioas
Thanks for pointing out that problem!
You are right. We introduced chart versioning of this PR, but that means we aren't to download in the same dir when updating the chart version.
So, valuesFile is optional value, and I think that read values.yaml in charts when omit.

kustomize/plugin/builtin/helmchartinflationgenerator/HelmChartInflationGenerator.go

Lines 105 to 110 in 0c461d6

	// The ValuesFile(s) may be consulted by the plugin, so it must
	// be under the loader root (unless root restrictions are
	// disabled).
	if p.ValuesFile == "" {
	p.ValuesFile = filepath.Join(p.absChartHome(), p.Name, "values.yaml")
	}

I think looks like your use case is covered by omitting that value.
If you want to other scenarios, Could you describe your use case?

Hi @ardikabs
Could you update the below doc about the change of this PR?
https://github.com/kubernetes-sigs/cli-experimental/blob/master/site/content/en/references/kustomize/builtins/_index.md?plain=1#L782

[farioas]

then you need to delete the base folder first, then redownload the helm chart again.

That's a part of an automated pipeline:

• Lookup for a new helm chart version
• Update "base" layer to a new version
• Delete existing folder
• Run kustomize build --enable-helm .
• Commit and push
• Integration tests
• Promotion to other envs

I understand why you made this improvement, thank you for your work. I'll make changes in my workflow to rename folder after pull.