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

feat(values) add fullnameOverride #635

Merged
merged 3 commits into from
Jul 7, 2022
Merged

feat(values) add fullnameOverride #635

merged 3 commits into from
Jul 7, 2022

Conversation

rainest
Copy link
Contributor

@rainest rainest commented Jul 7, 2022

What this PR does / why we need it:

Adds a full name override.

Which issue this PR fixes

(optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged)

Special notes for your reviewer:

Compare:

10:56:40-0700 esenin $ helm template --namespace default aaa /tmp/symkong -f ~/src/charts/charts/kong/example-values/minimal-kong-enterprise-dbless.yaml --set fullnameOverride=ccc 2>/dev/null | grep -A2 "kind: Service" | head -3 
kind: ServiceAccount
metadata:
  name: ccc
10:56:57-0700 esenin $ helm template --namespace default aaa /tmp/symkong -f ~/src/charts/charts/kong/example-values/minimal-kong-enterprise-dbless.yaml --set nameOverride=ccc 2>/dev/null | grep -A2 "kind: Service" | head -3 
kind: ServiceAccount
metadata:
  name: aaa-ccc
10:57:08-0700 esenin $ helm template --namespace default aaa /tmp/symkong -f ~/src/charts/charts/kong/example-values/minimal-kong-enterprise-dbless.yaml 2>/dev/null | grep -A2 "kind: Service" | head -3 
kind: ServiceAccount
metadata:
  name: aaa-kong

nameOverride was originally entirely undocumented; it and this are now in the readme but I've left them out of values still.

Checklist

[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]

  • PR is based off the current tip of the main branch.
  • Changes are documented under the "Unreleased" header in CHANGELOG.md
  • New or modified sections of values.yaml are documented in the README.md
  • Commits follow the Kong commit message guidelines

@rainest rainest requested a review from a team as a code owner July 7, 2022 17:59
@rainest rainest enabled auto-merge (rebase) July 7, 2022 18:03
charts/kong/README.md Outdated Show resolved Hide resolved
charts/kong/README.md Outdated Show resolved Hide resolved
@rainest rainest disabled auto-merge July 7, 2022 18:37
@rainest rainest enabled auto-merge (squash) July 7, 2022 18:37
@rainest rainest requested a review from shaneutt July 7, 2022 18:37
@rainest rainest merged commit 5e7a146 into main Jul 7, 2022
@rainest rainest deleted the feat/fullnameoverride branch July 7, 2022 18:54
@@ -14,7 +14,7 @@ We truncate at 63 chars because some Kubernetes name fields are limited to this

{{- define "kong.fullname" -}}
{{- $name := default .Chart.Name .Values.nameOverride -}}
{{- printf "%s-%s" .Release.Name $name | trunc 63 | trimSuffix "-" -}}
{{- default (printf "%s-%s" .Release.Name $name | trunc 63 | trimSuffix "-") .Values.fullnameOverride -}}
Copy link
Member

Choose a reason for hiding this comment

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

One small comment on this though: we might want to consider failing on templates rendering when .Values.fullnameOverride is longer than 63 characters since that might cause problems.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It'll fail to apply resources. I figured we shouldn't truncate the "use exactly what I provide, no additional formatting" one since the remediation there should be that you choose a new, acceptable name of your own.

Copy link
Member

Choose a reason for hiding this comment

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

Not sure if I understood but my proposal was not to truncate the full override but to fail and "return an error" (to the extent possible with helm).

sgrzemski pushed a commit to sgrzemski/charts that referenced this pull request Jul 11, 2022
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.

Add support for fullnameOverride to Chart
3 participants