Make sure resource names do not exceed limit of 63 characters #491

nader-ziada · 2019-02-12T15:00:28Z

Fixes: #481

Proposed changes

Generated names for taskruns and build steps get a random suffix
of five alphanumerics. The string is guaranteed to not exceed the
length of a standard Kubernetes name (63 characters)
Update tests using fixed seed for the name generation
Remove rand string func from test folder and use GenerateName()
CRD types already have validation for names in validateObjectMetadata()

This PR was co-authored by @bobcatfish, but will not put her name in the commit to avoid CLA hassle, (sorry Christie 🤷‍♂️)

abayer · 2019-02-12T15:10:46Z

cc @dwnusbaum

nader-ziada · 2019-02-12T17:00:24Z

/test pull-knative-build-pipeline-integration-tests

bobcatfish · 2019-02-12T18:52:16Z

(sorry Christie )

haha np :)

bobcatfish

This looks great to me, but I think someone other than me should review it since I was part of creating it 😇

bobcatfish · 2019-02-12T18:53:37Z

pkg/apis/pipeline/v1alpha1/artifact_bucket.go

@@ -116,7 +117,7 @@ func (b *ArtifactBucket) GetSecretsVolumes() []corev1.Volume {
 	volumes := []corev1.Volume{}
 	for _, sec := range b.Secrets {
 		volumes = append(volumes, corev1.Volume{
-			Name: fmt.Sprintf("bucket-secret-volume-%s", sec.SecretName),
+			Name: names.SimpleNameGenerator.GenerateName(fmt.Sprintf("bucket-secret-%s", sec.SecretName)),


i had one thought, maybe overkill tho: we could make a GenerateNamef, something like:

names.SimpleNameGenerator.GenerateNamef("bucket-secret-%s", sec.SecretName)

Since we're so often passing the output of fmt.Sprintf in here anyway 🤷‍♀️

bobcatfish · 2019-02-12T18:54:51Z

pkg/reconciler/v1alpha1/taskrun/taskrun_test.go

+		wantServiceAccountName string
+		wantPodVolume          []corev1.Volume
+		wantPreSteps           []corev1.Container
+		wantSteps              []corev1.Container


oh man you got through it! thanks so much for fixing this up @pivotal-nader-ziada ❤️ ❤️ ❤️ i think this is definitely better going forward

Yeah, this is great! I have given you quite a few merge conflicts though because of #488, sorry! I would lean towards using a single wantPod field instead of using all of the individual fields so that we can match on anything in ObjectMeta or any additional fields as needed in the future as well.

haha, that one file took more time than the rest of the PR :D

I would lean towards using a single wantPod field instead of using all of the individual fields so that we can match on anything in ObjectMeta or any additional fields as needed in the future as well.

The only "trouble" here @dwnusbaum is that there ends up being a lot of repetition between the pods so the cases become pretty verbose :S (maybe it's worth it tho?)

Side note: might start looking a bit better if we add pod builders in test/builder but that's probably a PR in itself :D

might start looking a bit better if we add pod builders in test/builder

Yeah that was what I was thinking, but like you said probably best left for another PR since the current code gets the job done.

bobcatfish · 2019-02-12T18:55:53Z

test/README.md


-_See [randstring.go](./randstring.go)._
+namespace := names.SimpleNameGenerator.GenerateName("arendelle")
+```


nice catch, thanks for updating the docs!

bobcatfish · 2019-02-12T18:57:15Z

pkg/reconciler/v1alpha1/taskrun/taskrun_test.go

+				},
+			}},
+		},
+	} {


i think if we wanted to we could make some of these a bit less verbose but this is fine for now if we want to iterate on it :D

yes, agreed, just wanted to get everything working first, then we can improve on it

shashwathi

This looks great @pivotal-nader-ziada @bobcatfish 👍

shashwathi · 2019-02-13T17:31:43Z

/assign @shashwathi

nader-ziada · 2019-02-13T19:35:30Z

/test pull-knative-build-pipeline-integration-tests

nader-ziada · 2019-02-13T20:24:03Z

/test pull-knative-build-pipeline-integration-tests

test/cancel_test.go

- generated names for taskruns and build steps get a random suffix of five alphanumerics. The string is guaranteed to not exceed the length of a standard Kubernetes name (63 characters) - updated tests using fixed seed for the name generation - remove rand string func from test folder - use new rand string already used for generated names

Pipelinerun resolution now checking if taskrun already created so it doesn't generate a new random suffix to it updated e2e tests to work with variable taskrun names Signed-off-by: Nader Ziada <nziada@pivotal.io>

knative-metrics-robot · 2019-02-14T18:56:39Z

The following is the coverage report on pkg/.
Say /test pull-knative-build-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/v1alpha1/pipelinerun/pipelinerun.go	85.0%	84.9%	-0.1
pkg/reconciler/v1alpha1/pipelinerun/resources/pipelinerunresolution.go	88.5%	88.9%	0.4
pkg/reconciler/v1alpha1/taskrun/resources/pod.go	83.6%	83.7%	0.1
pkg/reconciler/v1alpha1/taskrun/taskrun.go	74.6%	74.1%	-0.5

shashwathi · 2019-02-14T20:17:59Z

/lgtm

shashwathi

Great PR. 👍

knative-prow-robot · 2019-02-14T20:18:34Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pivotal-nader-ziada, shashwathi

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:

~~OWNERS~~ [pivotal-nader-ziada,shashwathi]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

bobcatfish · 2019-02-14T22:34:55Z

nice ooooone @pivotal-nader-ziada !! 🎉

PR tektoncd#491 did the trick for generated resource containers, but not for `TaskSpec`-defined step container names. This fixes that.

PR #491 did the trick for generated resource containers, but not for `TaskSpec`-defined step container names. This fixes that.

Releasing release.yaml v0.15.2

googlebot added the cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit label Feb 12, 2019

knative-prow-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Feb 12, 2019

knative-prow-robot requested review from shashwathi and tejal29 February 12, 2019 15:00

nader-ziada mentioned this pull request Feb 12, 2019

Propagate labels from PipelineRun to TaskRun to Pods #488

Merged

nader-ziada force-pushed the long-name branch from 452ac56 to 933b5cf Compare February 12, 2019 18:40

bobcatfish reviewed Feb 12, 2019

View reviewed changes

shashwathi approved these changes Feb 12, 2019

View reviewed changes

nader-ziada force-pushed the long-name branch 2 times, most recently from 9adde1f to 244e1cb Compare February 12, 2019 21:18

bobcatfish mentioned this pull request Feb 12, 2019

Use "AppendRandomString" function from knative/pkg #490

Closed

nader-ziada force-pushed the long-name branch 2 times, most recently from e2aab67 to 8dae438 Compare February 12, 2019 23:00

knative-prow-robot assigned shashwathi Feb 13, 2019

bobcatfish mentioned this pull request Feb 13, 2019

Add Pipeline and Task-scoped environment variables #496

Closed

nader-ziada force-pushed the long-name branch from 8dae438 to 7bce4e1 Compare February 13, 2019 19:26

shashwathi reviewed Feb 13, 2019

View reviewed changes

test/cancel_test.go Outdated Show resolved Hide resolved

nader-ziada force-pushed the long-name branch 3 times, most recently from f8faa05 to 219a340 Compare February 14, 2019 17:01

nader-ziada added 2 commits February 14, 2019 13:54

pipelinerun resolution checking if taskrun exists

ae1ee4c

Pipelinerun resolution now checking if taskrun already created so it doesn't generate a new random suffix to it updated e2e tests to work with variable taskrun names Signed-off-by: Nader Ziada <nziada@pivotal.io>

nader-ziada force-pushed the long-name branch from 219a340 to ae1ee4c Compare February 14, 2019 18:54

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 14, 2019

shashwathi approved these changes Feb 14, 2019

View reviewed changes

knative-prow-robot merged commit 0502dbe into tektoncd:master Feb 14, 2019

tcnghia mentioned this pull request Feb 19, 2019

Use test name in e2e test resources, so we can tell which resources belong to which test. knative/serving#3268

Merged

abayer mentioned this pull request Feb 22, 2019

Make sure TaskSpec step container names aren't too long as well #550

Merged

3 tasks

knative-prow-robot pushed a commit that referenced this pull request Feb 26, 2019

Make sure TaskSpec step container names aren't too long as well

7499115

PR #491 did the trick for generated resource containers, but not for `TaskSpec`-defined step container names. This fixes that.

nader-ziada deleted the long-name branch March 7, 2019 16:35

mchmarny unassigned shashwathi Mar 7, 2019

piyush-garg pushed a commit to piyush-garg/pipeline that referenced this pull request Aug 17, 2020

Merge pull request tektoncd#491 from pradeepitm12/release-v0.15.2

aa7831b

Releasing release.yaml v0.15.2

pritidesai mentioned this pull request Mar 11, 2022

Add CloudEvents for Run definitions #4659

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make sure resource names do not exceed limit of 63 characters #491

Make sure resource names do not exceed limit of 63 characters #491

nader-ziada commented Feb 12, 2019

abayer commented Feb 12, 2019

nader-ziada commented Feb 12, 2019

bobcatfish commented Feb 12, 2019

bobcatfish left a comment

bobcatfish Feb 12, 2019

nader-ziada Feb 12, 2019

bobcatfish Feb 12, 2019

dwnusbaum Feb 12, 2019 •

edited

Loading

nader-ziada Feb 12, 2019

bobcatfish Feb 13, 2019

dwnusbaum Feb 13, 2019

bobcatfish Feb 12, 2019

bobcatfish Feb 12, 2019

nader-ziada Feb 12, 2019

shashwathi left a comment

shashwathi commented Feb 13, 2019

nader-ziada commented Feb 13, 2019

nader-ziada commented Feb 13, 2019

knative-metrics-robot commented Feb 14, 2019

shashwathi commented Feb 14, 2019

shashwathi left a comment

knative-prow-robot commented Feb 14, 2019

bobcatfish commented Feb 14, 2019

Make sure resource names do not exceed limit of 63 characters #491

Make sure resource names do not exceed limit of 63 characters #491

Conversation

nader-ziada commented Feb 12, 2019

Proposed changes

abayer commented Feb 12, 2019

nader-ziada commented Feb 12, 2019

bobcatfish commented Feb 12, 2019

bobcatfish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dwnusbaum Feb 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shashwathi left a comment

Choose a reason for hiding this comment

shashwathi commented Feb 13, 2019

nader-ziada commented Feb 13, 2019

nader-ziada commented Feb 13, 2019

knative-metrics-robot commented Feb 14, 2019

shashwathi commented Feb 14, 2019

shashwathi left a comment

Choose a reason for hiding this comment

knative-prow-robot commented Feb 14, 2019

bobcatfish commented Feb 14, 2019

dwnusbaum Feb 12, 2019 •

edited

Loading