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

Truncate the delete apiservice job name #471

Merged
merged 2 commits into from
Nov 30, 2023

Conversation

abalaven
Copy link
Contributor

Observed an error where a long konk-service release name could potentially prevent a delete-apiservice job from triggering as it would surpass the 63 character limit allowed for job names. This PR resolves this by truncating the length of the job name to less than 63 characters.

@abalaven abalaven requested a review from a team as a code owner October 26, 2023 21:18
@@ -1,7 +1,7 @@
apiVersion: batch/v1
kind: Job
metadata:
name: {{ include "konk-service.fullname" . }}-delete-apiservice
name: {{ include "konk-service.fullname" . | trunc 43 }}-delete-apiservice
Copy link
Contributor

@kd7lxl kd7lxl Oct 26, 2023

Choose a reason for hiding this comment

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

This is fine, but it might be a little more obvious to the reader to truncate the whole string to the actual limit like

Suggested change
name: {{ include "konk-service.fullname" . | trunc 43 }}-delete-apiservice
name: {{ printf "%s-delete-apiservice" (include "konk-service.fullname" .) | trunc 63 }}

Copy link
Contributor

Choose a reason for hiding this comment

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

Never mind, I think I prefer the way you did it originally. A really long name that truncates the entire -delete-apiservice part off the end could result in an ambiguous name.

Copy link
Contributor

Choose a reason for hiding this comment

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

One thing I would do either way is trim double - like the helpers do:

Suggested change
name: {{ include "konk-service.fullname" . | trunc 43 }}-delete-apiservice
name: {{ include "konk-service.fullname" . | trunc 43 | trimSuffix "-" }}-delete-apiservice

@abalaven abalaven force-pushed the truncate-del-api-service-job-name branch from 962c6b2 to 1af2cad Compare November 29, 2023 21:09
@abalaven abalaven force-pushed the truncate-del-api-service-job-name branch from 1af2cad to e1de851 Compare November 30, 2023 00:20
@abalaven abalaven merged commit 0573663 into main Nov 30, 2023
9 checks passed
@abalaven abalaven deleted the truncate-del-api-service-job-name branch November 30, 2023 00:39
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.

2 participants