fix(argo-cd): installation failure due to dot(.) in release name.

[jagpreetstamber]

CLOSES #3685

Checklist:

• I have bumped the chart version according to versioning
• I have updated the documentation according to documentation
• I have updated the chart changelog with all the changes that come with this pull request according to changelog.
• Any new values are backwards compatible and/or have sensible default.
• I have signed off all my commits as required by DCO.
• I have created a separate pull request for each chart according to pull requests
• My build is green (troubleshooting builds).

[jmeridth]

I'm torn on this one. I was always taught don't include periods in the names when I'm installing the helm chart.

In the issue you are fixing, you said helm install -n argocd test.argocd to reproduce. The fix is to not use a period 😄

I personally prefer an error instead of a tool renaming without my explicit saying so. @mkilchhofer @yu-croco @tico24 I'm willing to be outvoted on this. I've seen both perspectives online.

[mkilchhofer]

I'm torn on this one.
(..)
@mkilchhofer @yu-croco @tico24 I'm willing to be outvoted on this. I've seen both perspectives online.

I have the same mixed feelings about this PR.

When I create a chart using Helm's latest skeleton helm create mychart, it has the same drawback as we discuss here:

$ cat mychart/templates/deployment.yaml mychart/templates/_helpers.tpl
apiVersion: apps/v1
kind: Deployment
metadata:
  name: {{ include "mychart.fullname" . }}
  labels:
    {{- include "mychart.labels" . | nindent 4 }}
spec:
  # ..

{{/*
Expand the name of the chart.
*/}}
{{- define "mychart.name" -}}
{{- default .Chart.Name .Values.nameOverride | trunc 63 | trimSuffix "-" }}
{{- end }}

{{/*
Create a default fully qualified app name.
We truncate at 63 chars because some Kubernetes name fields are limited to this (by the DNS naming spec).
If release name contains chart name it will be used as a full name.
*/}}
{{- define "mychart.fullname" -}}
{{- if .Values.fullnameOverride }}
{{- .Values.fullnameOverride | trunc 63 | trimSuffix "-" }}
{{- else }}
{{- $name := default .Chart.Name .Values.nameOverride }}
{{- if contains $name .Release.Name }}
{{- .Release.Name | trunc 63 | trimSuffix "-" }}
{{- else }}
{{- printf "%s-%s" .Release.Name $name | trunc 63 | trimSuffix "-" }}
{{- end }}
{{- end }}
{{- end }}

# ..

The problem is that charts normally has dozens of edge cases like this and we cannot cover all of them with a simple fix. And if we can, the code would be unnecessarily complex.

Also I think the community normally understands this behavior of the charts as the edge cases are well-known.

I am open to accept this PR but I think it doesn't help much. And for sure I don't want to try fixing all of the edge cases charts normally have.

[jagpreetstamber]

I'm torn on this one.
(..)
@mkilchhofer @yu-croco @tico24 I'm willing to be outvoted on this. I've seen both perspectives online.

I have the same mixed feelings about this PR.

When I create a chart using Helm's latest skeleton helm create mychart, it has the same drawback as we discuss here:

$ cat mychart/templates/deployment.yaml mychart/templates/_helpers.tpl
apiVersion: apps/v1
kind: Deployment
metadata:
  name: {{ include "mychart.fullname" . }}
  labels:
    {{- include "mychart.labels" . | nindent 4 }}
spec:
  # ..

{{/*
Expand the name of the chart.
*/}}
{{- define "mychart.name" -}}
{{- default .Chart.Name .Values.nameOverride | trunc 63 | trimSuffix "-" }}
{{- end }}

{{/*
Create a default fully qualified app name.
We truncate at 63 chars because some Kubernetes name fields are limited to this (by the DNS naming spec).
If release name contains chart name it will be used as a full name.
*/}}
{{- define "mychart.fullname" -}}
{{- if .Values.fullnameOverride }}
{{- .Values.fullnameOverride | trunc 63 | trimSuffix "-" }}
{{- else }}
{{- $name := default .Chart.Name .Values.nameOverride }}
{{- if contains $name .Release.Name }}
{{- .Release.Name | trunc 63 | trimSuffix "-" }}
{{- else }}
{{- printf "%s-%s" .Release.Name $name | trunc 63 | trimSuffix "-" }}
{{- end }}
{{- end }}
{{- end }}

# ..

The problem is that charts normally has dozens of edge cases like this and we cannot cover all of them with a simple fix. And if we can, the code would be unnecessarily complex.

Also I think the community normally understands this behavior of the charts as the edge cases are well-known.

I am open to accept this PR but I think it doesn't help much. And for sure I don't want to try fixing all of the edge cases charts normally have.

his PR but I think it doesn't help much. And for sure I don't want to try fixing all of the edge cases charts normally have.

@mkilchhofer @jmeridth The charts are many time installed by different mechanisms, Although there will be failure in the first attempt if user gives wrong input they will have to fix and retry.

I added this PR as we are trying to integrate this as part of Extension for Azure Kubernetes Service to allow easier installation for Azure customers and we ran into the problem where extension names can have dot(.), We can handle this at our end but we still wanted to contribute upstream as it is simple fix.

[jmeridth]

I'm willing to approve after the effort you've put in @jagpreetstamber. If another maintainer approves, you're good. They may not 🤷. Thank you for your understanding and your contribution efforts. We always appreciate upstream contributions. 🙇