Added support for legacy Helm chart repositories

[frizzr]

This change adds support for legacy (non-OCI) Helm chart repositories in the least-disruptive way to the existing API.

I will add unit tests if the maintainers are initially okay with this. If not, I could instead refactor the code to add a separate method with a different name and the different signature.

If not interested in adding legacy Helm repo support at all, just say so and I'll close my PR.

Thanks for your consideration!

Russ

[RetGal]

The PR looks fine, as it extends functionality and maintains API compatibility.
Of course it would be extra nice if you could also extend the test and examples.
But in any case, thank you very much for your contribution, which is greatly appreciated!

[RetGal]

@frizzr Could you perhaps give us an example usage of helm push to a non oci registry?

[frizzr]

@frizzr Could you perhaps give us an example usage of helm push to a non oci registry?

Yes. I'll get something together in the next day or two.

[frizzr]

I'll need to create additional code to send a Helm chart package to a legacy Helm repository. There is no built-in helm command to upload a chart to a non-OCI repository. Helm only added the push command for OCI.

A legacy Helm repository is just a file server that handles HTTP(S) file uploads. In a CI/CD process we normally use curl and use basic authentication, so given your base image has curl on it, I can go ahead and use that if you have no objections.

The way I will write it is that the user will need to prefix any vendor-specific pathing as part of the repository string. For example, JFrog Artifactory usually requires artifactory after the registry (hostname) and then the repository name and the folder path.

So, for those who want to send a chart to Artifactory hosted at artifactory.mycompany.com and the name of the Helm chart repository is myteam-helm, then the user would need to issue the following:

dagger call package-push \
  --registry artifactory.mycompany.com \
  --repository artifactory/myteam-helm \
  --username $REGISTRY_HELM_USER \
  --password env:REGISTRY_HELM_PASSWORD \
  --directory ./examples/testdata/mychart/ \
  --use-non-oci-helm-repo=true

Another difference with non-OCI repositories is that you can put the packaged chart in any subpath in the destination repository, so similarly, we would allow the user to suffix that pathing to the repository parameter.

Continuing the example, if the user wanted to put the chart in a products/my-team-product subpath in the repository, then the command issued would be

dagger call package-push \
  --registry artifactory.mycompany.com \
  --repository artifactory/myteam-helm/products/my-team-product \
  --username $REGISTRY_HELM_USER \
...

Finally, I'm open to even adding a new Dagger function to your API if you feel this is overloading things too much with the existing API, or creating additional optional parameters with the existing function. In short, I'm flexible and willing to abide by any API guidance you provide.

Still, maybe all of this is a deal-breaker for you if you only want to include commands that Helm directly supports or assume that the proposed support will only be for HTTPS Basic Authentication.

I'll have to write the code either way (since I am making a wrapper around your module anyway given our in-company API is different) but I fully understand whatever you decide.

[RetGal]

OK, thank you for the clarifications!

I think it's fine to use curl rather than adding another binary to the image.

Basicly the existing helm push command
[]string{"helm", "push", fmt.Sprintf("%s-%s.tgz", name, version), opts.getRepoFqdn()}
would be replaced by something like this for non oci then:
[]string{"curl", "-X", "PUT", "-u", fmt.Sprintf("%s:%s", opts.Username, opts.Password), "--data-binary", fmt.Sprintf("%s-%s.tgz", name, version), opts.getChartFqdn()}

This logic could be put in a separate function, which then could be called inside the WithExec statement:

func (p PushOpts) getPushCommand(name string, version string) []string {
	if p.Oci {
		return []string{"helm", "push", fmt.Sprintf("%s-%s.tgz", name, version), p.getRepoFqdn()}
	}
	return []string{"curl", "-X", "PUT", "-u", fmt.Sprintf("%s:%s", p.Username, p.Password), "--data-binary", fmt.Sprintf("%s-%s.tgz", name, version), p.getChartFqdn(name)}
}

WithExec(opts.getPushCommand(name, version)).

[frizzr]

The only concern I have with your suggestion of putting the push command function in PushOpts would be that the password secret would be exposed since we are grabbing it from the struct and not running it through WithSecretVariable. Maybe I am misunderstanding how secret variables work with Dagger. I'm still new to it all.

For now I will just inline the logic in PackagePush. Feel free to suggest other approaches or to educate me. Thanks!

[RetGal]

That's actually a good point! I'll leave the further implementation up to you =)

[frizzr]

Okay. Initial code added. I'll now test it.

[frizzr]

I was able to test that non-OCI charts do upload successfully and the logic for detecting if the chart already exists on the server is also working for non-OCI charts.

Would it be possible for you to test OCI charts with this code?

I did have to change the new boolean flag from --oci to --use-non-oci-helm-repo and reverse its polarity due to this current issue with default=true when using a module from Go:

dagger/dagger#8810

[RetGal]

Thank you, looks great! Just one last thing: I added a test for the existing chart check.
Could you please rebase and check if the new functionality also passes the test?

[chrira]

Thank you @frizzr for your contributions!
We added tests for the PackagePush return logic.
Please rebase your PR onto main.
Then you'll see, that a test fails.
I commented the problems of your solution.

[chrira]

This is the behavior of the method for existing or not existing versions. Errors are only thrown for technical reasons.

[frizzr]

I believe the commit I made resolves this. Let me know for sure. Thanks!