Skip to content

Support for setting the service name#113

Open
perangel wants to merge 1 commit intotwuni:mainfrom
airbytehq:main
Open

Support for setting the service name#113
perangel wants to merge 1 commit intotwuni:mainfrom
airbytehq:main

Conversation

@perangel
Copy link
Copy Markdown

Problem

It would be useful to be able to specify the service name and not rely on the fullname (which requires using fullnameOverride to set).

While service.name is defined in the values.yaml, it is not referenced anywhere.

Proposed Solution

  • Set the default value of service.name to "" and add an if/else conditional in the service template that will allow you to specify the service name.
  • If unset, it will fallback to the existing behavior.

Comment on lines +4 to +6
{{- if .Values.service.name }}
name: {{ .Values.service.name }}
{{- else }}
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Alternatively, I think this could be written as:

Suggested change
{{- if .Values.service.name }}
name: {{ .Values.service.name }}
{{- else }}
name: {{ .Values.service.name | default template "docker-registry.fullname" . }}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This works with include instead of template, and some extra parens:

Suggested change
{{- if .Values.service.name }}
name: {{ .Values.service.name }}
{{- else }}
name: {{ .Values.service.name | default (include "docker-registry.fullname" .) }}

I found a nice reference about template vs. include here: https://stackoverflow.com/a/71091339

Copy link
Copy Markdown
Collaborator

@joshsizer joshsizer left a comment

Choose a reason for hiding this comment

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

@perangel Nice catch, this is a much needed feature! I left a suggestion that I've tested locally and confirmed works for me.

If that suggestion looks good to you, please apply it and we can get this merged in 👍

Edit: we do also need to make that change anywhere the service name is referenced, for example from the ingress resource.

Comment on lines +4 to +6
{{- if .Values.service.name }}
name: {{ .Values.service.name }}
{{- else }}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This works with include instead of template, and some extra parens:

Suggested change
{{- if .Values.service.name }}
name: {{ .Values.service.name }}
{{- else }}
name: {{ .Values.service.name | default (include "docker-registry.fullname" .) }}

I found a nice reference about template vs. include here: https://stackoverflow.com/a/71091339

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants