-
Notifications
You must be signed in to change notification settings - Fork 374
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
helm: Allow multiple installations of the Tetragon Helm chart #1400
Conversation
2659fa4
to
0f94920
Compare
Thanks! @willfindlay or @mtardy could you please have a look at this? CC: @michi-covalent |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think Helm recommended to use something like {{ template "fullname" . }}
that was combining the release name and the chart name. But it would look weird to have that here and would result in very weird naming.
It was something very complicated like:
{{/*
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 "tetragon-enterprise.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 }}
Honestly, I feel like it's simpler to do it that way, we can just end up with invalid names because of Kubernetes resources naming restriction but that would be the user shooting shooting their own foot with a nice error message so I think it's fine.
If it's annoying for some reason (because we want people to be free with their release name) we can use something like that later:
{{/*
Expand the name of the chart.
*/}}
{{- define "tetragon-enterprise.name" -}}
{{- default .Chart.Name .Values.nameOverride | trunc 63 | trimSuffix "-" }}
{{- end }}
With a dedicated .Values.nameOverride
that can be used for those advanced use cases.
Hey @willfindlay do you want to take a look at this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep this makes good sense to me
I added the
release-note/minor
Sorry @ashishkurmi we delayed your PR because of unavailability and unfortunately, you now need to rebase on main. Could you do that? |
Signed-off-by: Ashish Kurmi <[email protected]>
1c6acb7
to
3010f6e
Compare
@mtardy no worries, I have rebased my pull request, could you please review and merge it if everything looks good? I have also requested code review from @willfindlay as the earlier sign off had become stale. |
so did you adapt in the rebase? because a new operator thingy was pushed in the helm chart right, or modified? If yes we can merge this :) |
fixes #1399