Make sure TaskSpec step container names aren't too long as well #550
Conversation
PR tektoncd#491 did the trick for generated resource containers, but not for `TaskSpec`-defined step container names. This fixes that.
|
/assign @pivotal-nader-ziada |
|
@abayer: GitHub didn't allow me to assign the following users: dwnusbaum. Note that only knative members and repo collaborators can be assigned. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
There was a problem hiding this comment.
Thanks for catching and fixing this @abayer !
I have minor feedback about method naming - also trying to trick you into fixing what I think might be a related bug ;) ;) ;)
pkg/names/generate.go
Outdated
| @@ -52,3 +56,10 @@ func (simpleNameGenerator) GenerateName(base string) string { | |||
| } | |||
| return fmt.Sprintf("%s-%s", base, utilrand.String(randomLength)) | |||
| } | |||
|
|
|||
| func (simpleNameGenerator) GenerateBuildStepName(base string) string { | |||
There was a problem hiding this comment.
i think this name reflects how this function is being used, but maybe we could name it after what it's doing? It seems like the functionality we'll have is something like:
GenerateName- appends a random prefix AND truncates a string if it's too long (maybe call thisRestrictLengthWithRandomPrefix- called asnames.RestrictLengthWithRandomPrefixGenerateBuildStepName- truncates a string if it's too long (name idea:RestrictLength- since it'd be called asnames.RestrictLength)
| @@ -52,3 +56,10 @@ func (simpleNameGenerator) GenerateName(base string) string { | |||
| } | |||
| return fmt.Sprintf("%s-%s", base, utilrand.String(randomLength)) | |||
There was a problem hiding this comment.
i think there might actually be a bug here - the total length should be 63 or less, including the postfixed random string?
There was a problem hiding this comment.
Ah yeah good point, there is an off-by-one since the hyphen isn't accounted for in maxGeneratedNameLength, see https://github.com/knative/build-pipeline/pull/550/files#diff-0cd213308b34c799cb518fa9d6d1daacL46.
cb1a469 to
5a83f33
Compare
pkg/names/generate.go
Outdated
| return fmt.Sprintf("%s-%s", base, utilrand.String(randomLength)) | ||
| } | ||
|
|
||
| func (simpleNameGenerator) RestrictLength(base string) string { | ||
| if len(base) > maxGeneratedNameLength { | ||
| base = base[:maxGeneratedNameLength] |
There was a problem hiding this comment.
Why maxGeneratedNameLength instead of maxNameLength? Callers adding suffixes should be using RestrictLengthWithRandomSuffix, so the return value of this function could go all the way up to 63 chars, right?
There was a problem hiding this comment.
Honestly, oversight. Fixed it. =)
|
/test pull-knative-build-pipeline-integration-tests |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: abayer, dwnusbaum, pivotal-nader-ziada The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/test pull-knative-build-pipeline-integration-tests |
|
@dwnusbaum : I am working on a PR to fix the flakiness you ran into. |
- In PR tektoncd#550 the e2e test for taskrunTimeOut failed with reason " message: 'References a Task arendelle-2j8ft/giraffe that doesn''t exist: translating I0226 14:05:06.851] Build to Pod: serviceaccounts "default" not found'" Theory: ServiceAccount 'default' in test namespace is created with some delay but the taskrun reconciliation is triggered before that which causes this type of failure Fix: Once namespace is created call wait function to make sure SA is created.
- In PR #550 the e2e test for taskrunTimeOut failed with reason " message: 'References a Task arendelle-2j8ft/giraffe that doesn''t exist: translating I0226 14:05:06.851] Build to Pod: serviceaccounts "default" not found'" Theory: ServiceAccount 'default' in test namespace is created with some delay but the taskrun reconciliation is triggered before that which causes this type of failure Fix: Once namespace is created call wait function to make sure SA is created.
Changes
PR #491 did the trick for generated resource containers, but not for
TaskSpec-defined step container names. This fixes that.Submitter Checklist
These are the criteria that every PR should meet, please check them off as you
review them:
See the contribution guide for more details.
Release Notes
n/a