Skip to content
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

Fix secret name conflicts #873

Merged
merged 10 commits into from
Aug 16, 2023
Merged

Conversation

VihasMakwana
Copy link
Contributor

Fixes #439.

@VihasMakwana VihasMakwana requested review from a team as code owners August 3, 2023 12:22
@VihasMakwana VihasMakwana changed the title Fix secret name Fix secret name conflicts Aug 3, 2023
Comment on lines 152 to 157
{{ $name := (include "splunk-otel-collector.name" .) -}}
{{- if contains $name .Release.Name -}}
{{- .Release.Name | trunc 63 | trimSuffix "-" -}}
{{- else -}}
{{- printf "%s-%s" .Release.Name $name | trunc 63 | trimSuffix "-" -}}
{{- end -}}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
{{ $name := (include "splunk-otel-collector.name" .) -}}
{{- if contains $name .Release.Name -}}
{{- .Release.Name | trunc 63 | trimSuffix "-" -}}
{{- else -}}
{{- printf "%s-%s" .Release.Name $name | trunc 63 | trimSuffix "-" -}}
{{- end -}}
{{ default (include "splunk-otel-collector.fullname" .) .Values.secret.name }}

I think you can just replace directly with the helper function, and can also remove the if/else before this. splunk-otel-collector.fullname already evaluates the same conditions with the addition of also respecting the fullNameOverride variable which is missing here.

@VihasMakwana
Copy link
Contributor Author

@jinja2 I took a look and it turns out I over-complicated the changes. Please have a look at my latest changes, they're quite simple than before and easy to understand.

@dmitryax
Copy link
Contributor

@VihasMakwana please run make render

@VihasMakwana
Copy link
Contributor Author

@dmitryax I think we are good to merge.

@dmitryax dmitryax merged commit 1710161 into signalfx:main Aug 16, 2023
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix the secret name to be able to install more than one chart in the same cluster
3 participants