-
Notifications
You must be signed in to change notification settings - Fork 88
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 pre-delete-controller Job name to 63 characters #506
Conversation
1e14a8d
to
5fabe07
Compare
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.
Only had a few minor nits/suggestions - otherwise looks great!
chart/templates/_helpers.tpl
Outdated
@@ -17,13 +17,13 @@ If release name contains chart name it will be used as a full name. | |||
*/}} | |||
{{- define "vso.chart.fullname" -}} | |||
{{- if .Values.fullnameOverride }} | |||
{{- .Values.fullnameOverride | trunc 63 | trimSuffix "-" }} | |||
{{- .Values.fullnameOverride | trunc 27 | trimSuffix "-" }} |
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.
The doc string above should probably be updated, since we are only allowing a release name of 27.
chart/templates/_helpers.tpl
Outdated
@@ -7,7 +7,7 @@ | |||
Expand the name of the chart. | |||
*/}} | |||
{{- define "vso.chart.name" -}} | |||
{{- default .Chart.Name .Values.nameOverride | trunc 63 | trimSuffix "-" }} | |||
{{- default .Chart.Name .Values.nameOverride | trunc 27 | trimSuffix "-" }} |
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.
Did we double check that 27 chars was enough, relative to the recently added viewer/editor role resources?
test/unit/deployment.bats
Outdated
yq 'select(.kind == "Job") | .metadata' | tee /dev/stderr) | ||
|
||
local actual=$(echo "$object" | yq '.name' | tee /dev/stderr) | ||
[ "${actual}" = "pdcc-abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdef" ] |
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.
cool, I wonder if we could also assert the length of $actual
as well:
Something like:
[ "${#actual}" -eq 63 ]
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.
oooo, nice! thx, have included it in d51cefc
test/unit/deployment.bats
Outdated
yq 'select(.kind == "Job") | .metadata' | tee /dev/stderr) | ||
|
||
local actual=$(echo "$object" | yq '.name' | tee /dev/stderr) | ||
[ "${actual}" = "pdcc-release-name-vault-secrets-operator" ] |
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.
nit: extra leading whitespace
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.
fixed in d51cefc
test/unit/deployment.bats
Outdated
yq 'select(.kind == "Job") | .metadata' | tee /dev/stderr) | ||
|
||
local actual=$(echo "$object" | yq '.name' | tee /dev/stderr) | ||
[ "${actual}" = "pdcc-abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdef" ] |
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.
nit: extra leading whitespace
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.
fixed in d51cefc
* truncate pre-delete controller job name to 63 chars
Truncate pre-delete-cleanup-controller to 63 characters
Fixes #260
Closes #476
Closes #499 as duplicate