-
Notifications
You must be signed in to change notification settings - Fork 177
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
Template the password secret name #109
Template the password secret name #109
Conversation
@huw0 can you rebase? |
68e96b4
to
393e329
Compare
393e329
to
249e329
Compare
@nineinchnick - now rebased. With the changes I made in #108 it made sense for the helper to also be renamed in this PR. I've tested with the following options which I think correctly preserve backwards compatibility. However it'd be worth confirming my assumptions :). The default name of the secret is now - chartName-file-authentication. |
charts/trino/templates/_helpers.tpl
Outdated
{{- .Values.auth.passwordAuthSecret | trunc 63 | trimSuffix "-" }} | ||
{{- else }} | ||
{{- $name := default .Chart.Name .Values.nameOverride }} | ||
{{- if contains $name .Release.Name }} |
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.
Should we use hasPrefix .Release.Name $name
instead of contains
?
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.
Yes, I think this would be better.
See new commit. There were a number of functions that previously used contains that now use hasPrefix. This also means the if logic has changed.
charts/trino/templates/_helpers.tpl
Outdated
{{- if contains $name .Release.Name }} | ||
{{- .Release.Name | trunc 63 | trimSuffix "-" }}-file-authentication | ||
{{- else }} | ||
{{- printf "%s-%s" .Release.Name $name | trunc 63 | trimSuffix "-" }}-file-authentication |
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.
If we append an extra 20 characters, shouldn't we use trunc 43
?
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.
There were a number of places where the string is truncated to 63 but then is appended to. I've amended all of them in the new commit, ensuring that truncation is always done after the string is built.
Hopefully this will prevent the issue reoccurring if there are any future renames.
b3d2b43
to
51c03b4
Compare
As title