Check labels in e2e PipelineRun tests and document label propagation #500

dwnusbaum · 2019-02-13T18:46:15Z

The e2e tests in test/pipelinerun.go now assert that custom labels set on the PipelineRun are propagated through to the TaskRun and Pod, and that the static labels that are added to all TaskRuns and Pods are propagated correctly as well.

In addition, documentation has been added to explain that labels are propagated and to mention the specific labels that are added automatically to generated TaskRuns and Pods.

This is a follow-up to #488.

CC @shashwathi @bobcatfish

The e2e tests in test/pipelinerun.go now assert that custom labels set on the PipelineRun are propagated through to the TaskRun and Pod, and that the static labels that are added to all TaskRuns and Pods are propagated correctly as well. In addition, documentation has been added to explain that labels are propagated and to mention the specific labels that are added automatically to generated TaskRuns and Pods. Follow-up to tektoncd#488.

dwnusbaum · 2019-02-13T18:46:48Z

docs/pipelineruns.md

@@ -97,6 +97,26 @@ of the `TaskRun` resource object.
 For examples and more information about specifying service accounts, see the
 [`ServiceAccount`](./auth.md) reference topic.

+## Labels


This seemed like the least bad place for the docs, but I don't know if it really fits. Any suggestions are welcome!

i had trouble thinking of a good place myself! the only other alternative i can think of is to create a new doc like troubleshooting.md or faq.md or tips.md or something but none of those options seem great either 🤔 this seems fine for now tho if you don't like any of those ideas!

Yeah, it seems kind of weird to create a new file just for this information. My preference would be to leave it for now, and then if we end up with other troubleshooting-esque docs we can refactor them into a single place later. Does that seem ok?

dwnusbaum · 2019-02-13T18:48:50Z

test/pipelinerun_test.go

-		tb.PipelineRunServiceAccount(fmt.Sprintf("%s%d", saName, suffix)),
-	))
+	return tb.PipelineRun(getName(pipelineRunName, suffix), namespace,
+		tb.PipelineRunLabel("hello-world-key", "hello-world-value"),


This just adds a custom label to the service account propagation test. @shashwathi is that what you had in mind, or do you think I should create a separate test for labels specifically? (I wasn't sure how conservative we wanted to be about adding new e2e tests)

This looks good. Thank you @dwnusbaum

bobcatfish · 2019-02-13T18:51:02Z

/ok-to-test

(Still waiting to hear back on adding you to the org @dwnusbaum , sorry for the delay!!)

bobcatfish

A bit of misc. feedback from me! I won't /lgtm without a review from @shashwathi as well but for now anyway:

/approve

bobcatfish · 2019-02-13T18:52:17Z

docs/pipelineruns.md

@@ -97,6 +97,26 @@ of the `TaskRun` resource object.
 For examples and more information about specifying service accounts, see the
 [`ServiceAccount`](./auth.md) reference topic.

+## Labels


i had trouble thinking of a good place myself! the only other alternative i can think of is to create a new doc like troubleshooting.md or faq.md or tips.md or something but none of those options seem great either 🤔 this seems fine for now tho if you don't like any of those ideas!

bobcatfish · 2019-02-13T18:52:28Z

docs/pipelineruns.md

+you could use the following command:
+
+```shell
+kubectl get pods --all-namespaces -l pipeline.knative.dev/pipeline=test-pipeline


bobcatfish · 2019-02-13T18:53:27Z

test/pipelinerun_test.go

+				pod := pods.Items[0]
+				for key, taskRunVal := range labels {
+					if podVal := pod.ObjectMeta.Labels[key]; podVal != taskRunVal {
+						t.Errorf("Expected label %s=%s but got %s", key, taskRunVal, podVal)


total nitpick: could these be in a separate function called by the test? for the same of readability (test is getting long!)

Yeah good point, I'll separate it out!

Did some refactoring in 37ed488, let me know if that seems good or if you'd like to see a totally separate assertLabels function with all the label-related stuff to simplify further.

bobcatfish · 2019-02-13T18:53:45Z

test/pipelinerun_test.go

-						t.Errorf("Expected label %s=%s but got %s", lbl, expectedName, val)
+
+				// Check label propagation to Tasks
+				labels := make(map[string]string, len(pr.ObjectMeta.Labels)+3)


why +3 here? (maybe explain with a comment or a const ?)

len(pr.ObjectMeta.Labels) for custom labels, +2 for the static pipeline/pipelineRun label from the PipelineRun controller, and +1 for the taskRun label from the TaskRun controller. I'll add a comment 😄

Added a comment in 37ed488

bobcatfish · 2019-02-13T18:54:46Z

test/pipelinerun_test.go

+					labels[key] = val
+				}
+				labels[pipeline.GroupName+pipeline.PipelineLabelKey] = getName(pipelineName, i)
+				labels[pipeline.GroupName+pipeline.PipelineRunLabelKey] = getName(pipelineRunName, i)


what is the significance of pipeline.GroupName+pipeline.PipelineRunLabelKey and pipeline.GroupName+pipeline.PipelineLabelKey? (i assume these correspond to some label generation logic in the controller - maybe a comment here would help?)

Yeah those are static labels added by the controller, but definitely could use a comment.

Added a comment two lines up in 37ed488

dwnusbaum · 2019-02-13T20:02:27Z

/retest since CI failure appears to be infra related:

ERROR: (gcloud.beta.container.clusters.create) ResponseError: code=400, message=No valid versions with the prefix "1.11.7" found.

dwnusbaum · 2019-02-13T20:03:45Z

/retest

shashwathi

/lgtm

shashwathi · 2019-02-13T22:50:14Z

test/pipelinerun_test.go

-		tb.PipelineRunServiceAccount(fmt.Sprintf("%s%d", saName, suffix)),
-	))
+	return tb.PipelineRun(getName(pipelineRunName, suffix), namespace,
+		tb.PipelineRunLabel("hello-world-key", "hello-world-value"),


This looks good. Thank you @dwnusbaum

knative-prow-robot · 2019-02-13T22:52:34Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bobcatfish, dwnusbaum, 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~~ [bobcatfish,shashwathi]

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

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

knative-prow-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Feb 13, 2019

knative-prow-robot requested review from nader-ziada and tejal29 February 13, 2019 18:46

knative-prow-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Feb 13, 2019

dwnusbaum commented Feb 13, 2019

View reviewed changes

dwnusbaum mentioned this pull request Feb 13, 2019

copy the labels from Task ->TaskRun to the generated Pod #458

Closed

knative-prow-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Feb 13, 2019

bobcatfish reviewed Feb 13, 2019

View reviewed changes

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 13, 2019

bobcatfish assigned bobcatfish and shashwathi Feb 13, 2019

Refactor test code into separate functions for readability

37ed488

knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 13, 2019

shashwathi approved these changes Feb 13, 2019

View reviewed changes

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

knative-prow-robot merged commit 630d53e into tektoncd:master Feb 13, 2019

dwnusbaum deleted the 458-propagate-labels-to-pods branch February 13, 2019 23:00

mchmarny unassigned shashwathi Mar 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check labels in e2e PipelineRun tests and document label propagation #500

Check labels in e2e PipelineRun tests and document label propagation #500

dwnusbaum commented Feb 13, 2019

dwnusbaum Feb 13, 2019

bobcatfish Feb 13, 2019

dwnusbaum Feb 13, 2019

dwnusbaum Feb 13, 2019

shashwathi Feb 13, 2019

bobcatfish commented Feb 13, 2019

bobcatfish left a comment

bobcatfish Feb 13, 2019

bobcatfish Feb 13, 2019

bobcatfish Feb 13, 2019

dwnusbaum Feb 13, 2019

dwnusbaum Feb 13, 2019

bobcatfish Feb 13, 2019

dwnusbaum Feb 13, 2019

dwnusbaum Feb 13, 2019

bobcatfish Feb 13, 2019

dwnusbaum Feb 13, 2019

dwnusbaum Feb 13, 2019

dwnusbaum commented Feb 13, 2019

dwnusbaum commented Feb 13, 2019

shashwathi left a comment

shashwathi Feb 13, 2019

knative-prow-robot commented Feb 13, 2019

Check labels in e2e PipelineRun tests and document label propagation #500

Check labels in e2e PipelineRun tests and document label propagation #500

Conversation

dwnusbaum commented Feb 13, 2019

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

bobcatfish commented Feb 13, 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

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

Choose a reason for hiding this comment

dwnusbaum commented Feb 13, 2019

dwnusbaum commented Feb 13, 2019

shashwathi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knative-prow-robot commented Feb 13, 2019