Support to override helm release name #1682

lcostea · 2019-06-04T06:50:43Z

Description of the change

Field releaseName added to the application crd under helm
On the cli the flag is release-name
Unit tests to check release name is overriden

Benefits

When working with a centralised ArgoCD, the application names will have names based on cluster names and this will make the resources deployed not to be identical in different clusters. For example a redis service will have different names on different clusters because of different release names which will make it hard to be identified. This feature allows the release name to be overridden, so it will not be connected to the application name.

Closes #1066

alexec

This looks good, great unit tests.

I'd like to understand what happens when you change the name of the release. I think that @jessesuen mentions this in the original issue.

Can I ask for an e2e test (test/e2e/helm_test.go)?

Alex

alexec · 2019-06-04T16:17:14Z

pkg/apis/application/v1alpha1/types.go

@@ -113,6 +113,8 @@ type ApplicationSourceHelm struct {
 	ValueFiles []string `json:"valueFiles,omitempty" protobuf:"bytes,1,opt,name=valueFiles"`
 	// Parameters are parameters to the helm template
 	Parameters []HelmParameter `json:"parameters,omitempty" protobuf:"bytes,2,opt,name=parameters"`
+	// The Helm release name. If omitted it will use the application name


alexec · 2019-06-04T16:17:25Z

pkg/apis/application/v1alpha1/types.go

@@ -124,7 +126,7 @@ type HelmParameter struct {
 }

 func (h *ApplicationSourceHelm) IsZero() bool {
-	return len(h.ValueFiles) == 0 && len(h.Parameters) == 0
+	return (h.ReleaseName == "") && len(h.ValueFiles) == 0 && len(h.Parameters) == 0


good work - easy to overlook

lcostea · 2019-06-04T18:04:35Z

@alexec Regarding the change of the release name, because argo-cd now injects the label: app.kubernetes.io/instance=<<application_name>> this would cause a problem when application name will be different than release name (if we override it) and the chart uses this label for a selector.

An example would be the sealed-secret chart from helm/stable. Their deployment uses the app.kubernetes.io/instance: {{ .Release.Name }} as a selector for pods, while if argo-cd will overwrite it with the application name, when the application name will be different than the release name then the selector will stop working. And of course this can lead to big problems, like when upgrading.

A possible fix would be to use a different label for argo-cd, like it was used before the PR 857.
Another one would be not to allow to override release name when the chart uses the app.kubernetes.io/instance and to write some warning - that's what @jessesuen was mentioning.

So I can add the e2e test, but I guess we need to decide for the additional fix first.

alexec · 2019-06-05T18:24:20Z

I've approved, but you need to resolve the merge conflict.

I think users would appreciate a paragraph or tow /docs/user-guide-helm.md to document this behavior and how to use it with the flag @jessesuen mention.

alexec

Needs git merge master. Also docs.

jessesuen · 2019-06-06T01:10:32Z

Please read my comments about the error detection here:
#1066 (comment)

Basically, I think we either need to have chart dependent erroring, or we don't error at all.

Field releaseName added to the application crd On the cli the flag is release-name Unit tests to check release name is overriden

lcostea · 2019-06-06T06:32:45Z

@alexec
I solved the conflicts and added docs to the /docs/user-guide/helm.md including how to change the tracking label.
No additional error handling was added.

lcostea · 2019-06-06T08:32:55Z

Is the fact that codegen is failing a problem? I did add the releaseName under helm, so there are changes to the crds and protobuf. But I also saw some additional changes after I ran make codegen to the yaml, that weren't affected by the releaseName addition.

I checked the contributing guide, but that didn't mention if we should run the codegen part on PRs or not.

alexec · 2019-06-06T14:57:42Z

docs/user-guide/helm.md

+```
+
+```diff
+- Important notice on overriding the release name


minor - you can use a !!! warning admonition block here, see https://squidfunk.github.io/mkdocs-material/extensions/admonition/

I am not sure admonition extension works on Github, I just tried on a separate repo to display an warning and it didn't work. Maybe I am missing something?

alexec · 2019-06-06T14:58:06Z

docs/user-guide/helm.md

+```diff
+- Important notice on overriding the release name
+```
+Please not that overriding the Helm release name might cause problems when the chart you are deploying is using the `app.kubernetes.io/instance` label. 


great stuff

alexec · 2019-06-06T14:58:14Z

util/helm/helm_test.go

@@ -123,3 +123,39 @@ func TestHelmDependencyBuild(t *testing.T) {
 	_, err = h.Template("wordpress", "", nil)
 	assert.NoError(t, err)
 }
+
+func TestHelmTemplateReleaseNameOverwrite(t *testing.T) {


alexec · 2019-06-06T15:03:22Z

I should update the PR template.

You should run make pre-commit before push (ok it should probably be called pre-push :) ) . That runs code-gen, tests, and linting. One problem can be that you need the exact correct versions of the various tools. These are printed in the CircleCI job.

This has been a pain for other people, so let me know that' the, and I'll take 15m to sort it out for you.

lcostea · 2019-06-06T18:39:59Z

Yes, it could be the versions of those tools.

On the other hand I was looking at that codegen step that fails and it is somehow normal because I did add the releaseName parameter that went into assets/swagger.json

What is not normal and I don't understand yet is why both yaml crd have so many changes and this might be because of tool versions. Here I am still checking the versions in the CI and compare with what I have.

On the pkg/apis/api-rules/violation_exceptions.list seems that it should not be modified, so I will revert that.

alexec · 2019-06-06T18:52:02Z

I've merged and I'll fix quickly locally.

alexec self-requested a review June 4, 2019 14:51

alexec mentioned this pull request Jun 4, 2019

Make it possible to set release name different from argcd app name. #1066

Closed

alexec requested changes Jun 4, 2019

View reviewed changes

alexec approved these changes Jun 5, 2019

View reviewed changes

alexec requested changes Jun 5, 2019

View reviewed changes

Support to override helm release name

a76fb4c

Field releaseName added to the application crd On the cli the flag is release-name Unit tests to check release name is overriden

alexec reviewed Jun 6, 2019

View reviewed changes

alexec mentioned this pull request Jun 6, 2019

Updates contribution guide to state versions of tools and add PR template #1706

Closed

alexec approved these changes Jun 6, 2019

View reviewed changes

alexec merged commit 8275200 into argoproj:master Jun 6, 2019

vania-pooh mentioned this pull request Aug 17, 2023

Add chart customization options like fullnameOverride to the moon2 chart aerokube/charts#208

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support to override helm release name #1682

Support to override helm release name #1682

lcostea commented Jun 4, 2019

alexec left a comment

alexec Jun 4, 2019

alexec Jun 4, 2019

lcostea commented Jun 4, 2019

alexec commented Jun 5, 2019

alexec left a comment

jessesuen commented Jun 6, 2019

lcostea commented Jun 6, 2019

lcostea commented Jun 6, 2019

alexec Jun 6, 2019

lcostea Jun 6, 2019

alexec Jun 6, 2019

alexec Jun 6, 2019

alexec commented Jun 6, 2019

lcostea commented Jun 6, 2019

alexec commented Jun 6, 2019

Support to override helm release name #1682

Support to override helm release name #1682

Conversation

lcostea commented Jun 4, 2019

alexec left a comment

Choose a reason for hiding this comment

alexec Jun 4, 2019

Choose a reason for hiding this comment

alexec Jun 4, 2019

Choose a reason for hiding this comment

lcostea commented Jun 4, 2019

alexec commented Jun 5, 2019

alexec left a comment

Choose a reason for hiding this comment

jessesuen commented Jun 6, 2019

lcostea commented Jun 6, 2019

lcostea commented Jun 6, 2019

alexec Jun 6, 2019

Choose a reason for hiding this comment

lcostea Jun 6, 2019

Choose a reason for hiding this comment

alexec Jun 6, 2019

Choose a reason for hiding this comment

alexec Jun 6, 2019

Choose a reason for hiding this comment

alexec commented Jun 6, 2019

lcostea commented Jun 6, 2019

alexec commented Jun 6, 2019