Make PipelineSpec/PipelineRef in PipelineRun consistent with TaskRun #1495

vdemeester · 2019-10-30T09:15:45Z

Changes

In TaskRun, TaskRef can be nil, which was not the case for
PipelineRun (and PipelineRef). This fixes that by making the two consistent.

Note that we could do the opposite (aka change how it works in TaskRun).

Signed-off-by: Vincent Demeester vdemeest@redhat.com

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

Includes tests (if functionality changed/added)
Includes docs (if user facing)
Commit messages follow commit message best practices

See the contribution guide for more details.

Double check this list of stuff that's easy to miss:

If you are adding a new binary/image to the cmd dir, please update
the release Task to build and release this image.

Reviewer Notes

If API changes are included, additive changes must be approved by at least two OWNERS and backwards incompatible changes must be approved by more than 50% of the OWNERS, and they must first be added in a backwards compatible way.

vdemeester · 2019-10-30T10:10:22Z

/test pull-tekton-pipeline-integration-tests

dibyom · 2019-10-30T10:24:00Z

pkg/apis/pipeline/v1alpha1/pipelinerun_validation.go

-	// Check that one of PipelineRef and PipelineSpec is present
-	if ps.PipelineRef.Name == "" && ps.PipelineSpec == nil {
-		return apis.ErrMissingField("spec.pipelineRef.name", "spec.pipelineSpec")
+	// Check that one of PipelineRef and TaskSpec is present


Shouldn't this be PipelineSpec?

dibyom · 2019-10-30T10:24:57Z

pkg/apis/pipeline/v1alpha1/pipelinerun_validation.go

 	}

-	// Validate PipelineSpec if it's present
+	// Validate TaskSpec if it's present


PipelineSpec?

vdemeester · 2019-10-30T11:31:43Z

/test pull-tekton-pipeline-integration-tests

vdemeester · 2019-10-30T13:55:57Z

/test pull-tekton-pipeline-integration-tests

bobcatfish · 2019-10-30T14:01:47Z

I think this might solve #1498 ?

bobcatfish

Thanks for the cleanup @vdemeester !

/approve
/lgtm

tekton-robot · 2019-10-30T14:02:52Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bobcatfish, dibyom

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,dibyom]

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

vdemeester · 2019-10-30T14:04:59Z

/test pull-tekton-pipeline-integration-tests

vdemeester · 2019-10-30T14:36:52Z

/test pull-tekton-pipeline-integration-test

vdemeester · 2019-10-30T15:29:21Z

/test pull-tekton-pipeline-integration-test

vdemeester · 2019-10-30T16:24:47Z

/hold
Might be a legit failure… got twice the same error…

vdemeester · 2019-10-31T09:23:29Z

/test pull-tekton-pipeline-integration-tests

vdemeester · 2019-10-31T13:13:08Z

/hold cancel
/test pull-tekton-pipeline-integration-tests

dibyom · 2019-10-31T15:01:17Z

I1031 13:41:09.704] ERROR: tests timed out

Looks like YAML tests are timing out?

vdemeester · 2019-10-31T15:13:17Z

@dibyom yeah, the cloud-event yaml test is wanking since a few days… not sure why, @afrittoli is looking to update it with sidecars

vdemeester · 2019-10-31T15:39:05Z

/test pull-tekton-pipeline-integration-tests

afrittoli

Heh, I guess this is because TaskRef can be replaced by a TaskSpec.

Thanks for the cleanup!

afrittoli · 2019-11-02T22:27:55Z

third_party/VENDOR-LICENSE

@@ -5402,6 +5402,33 @@ Import: github.com/tektoncd/pipeline/vendor/github.com/modern-go/reflect2



+===========================================================


uh? why this change?

@afrittoli not sure, ran ./hack/update-codegen.sh and it added that 😓

afrittoli · 2019-11-02T22:31:05Z

This will close #1498

afrittoli · 2019-11-02T22:31:13Z

/test pull-tekton-pipeline-integration-tests

vdemeester · 2019-11-04T07:09:19Z

/test pull-tekton-pipeline-integration-tests

vdemeester · 2019-11-04T10:15:33Z

/test pull-tekton-pipeline-integration-tests

vdemeester · 2019-11-04T10:51:38Z

/test pull-tekton-pipeline-integration-tests

vdemeester · 2019-11-04T12:40:28Z

/test pull-tekton-pipeline-integration-tests

vdemeester · 2019-11-04T13:16:35Z

/test pull-tekton-pipeline-integration-tests

vdemeester · 2019-11-04T14:49:31Z

/test pull-tekton-pipeline-integration-tests

vdemeester · 2019-11-04T15:33:35Z

/test pull-tekton-pipeline-integration-tests

pritidesai · 2019-11-04T22:44:42Z

thanks for these changes
/lgtm

vdemeester · 2019-11-05T08:45:10Z

/test pull-tekton-pipeline-integration-tests

In TaskRun, TaskRef can be nil, which was not the case for PipelineRun (and PipelineRef). This fixes that by making the two consistent. Signed-off-by: Vincent Demeester <vdemeest@redhat.com>

vdemeester · 2019-11-05T10:38:32Z

pkg/reconciler/pipelinerun/metrics.go

 	ctx, err := tag.New(
 		context.Background(),
-		tag.Insert(r.pipeline, pr.Spec.PipelineRef.Name),


The failure was legit 😅 this was putting the controller in a crashloop 😝

tekton-robot · 2019-11-05T10:40:19Z

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

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/metrics.go	86.0%	86.8%	0.8

abayer · 2019-11-05T14:03:22Z

/lgtm

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

tekton-robot requested review from bobcatfish and dibyom October 30, 2019 09:15

tekton-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 30, 2019

vdemeester force-pushed the taskref-pipelineref-consistency branch from c6c149e to ee162e2 Compare October 30, 2019 09:22

vdemeester mentioned this pull request Oct 30, 2019

Make ResourceRef in PipelineResourceBinding consistent with TaskRun #1497

Merged

3 tasks

dibyom approved these changes Oct 30, 2019

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 30, 2019

vdemeester force-pushed the taskref-pipelineref-consistency branch from ee162e2 to b0cd520 Compare October 30, 2019 10:39

bobcatfish reviewed Oct 30, 2019

View reviewed changes

tekton-robot assigned bobcatfish Oct 30, 2019

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 30, 2019

vdemeester mentioned this pull request Oct 30, 2019

Adding nil Checks to TaskRun and PipelineRun Describe Commands tektoncd/cli#418

Merged

5 tasks

vdemeester mentioned this pull request Oct 30, 2019

Inconsistent Types for PipelineRef and TaskRef #1498

Closed

tekton-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 30, 2019

tekton-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 31, 2019

danielhelfand mentioned this pull request Oct 31, 2019

Check for ResourceRef and PipelineRef for TaskRun and PipelineRun Describe to be nil tektoncd/cli#429

Closed

afrittoli reviewed Nov 2, 2019

View reviewed changes

tekton-robot assigned pritidesai Nov 4, 2019

vdemeester force-pushed the taskref-pipelineref-consistency branch from b0cd520 to 013c8ab Compare November 5, 2019 10:20

tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Nov 5, 2019

Make PipelineSpec/PipelineRef in PipelineRun consistent with TaskRun

19070c7

In TaskRun, TaskRef can be nil, which was not the case for PipelineRun (and PipelineRef). This fixes that by making the two consistent. Signed-off-by: Vincent Demeester <vdemeest@redhat.com>

vdemeester force-pushed the taskref-pipelineref-consistency branch from 013c8ab to 19070c7 Compare November 5, 2019 10:37

vdemeester commented Nov 5, 2019

View reviewed changes

tekton-robot assigned abayer Nov 5, 2019

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 5, 2019

tekton-robot merged commit f15a0f3 into tektoncd:master Nov 5, 2019

vdemeester deleted the taskref-pipelineref-consistency branch November 5, 2019 14:18

		@@ -5402,6 +5402,33 @@ Import: github.com/tektoncd/pipeline/vendor/github.com/modern-go/reflect2



		===========================================================

Make PipelineSpec/PipelineRef in PipelineRun consistent with TaskRun #1495

Make PipelineSpec/PipelineRef in PipelineRun consistent with TaskRun #1495

Conversation

vdemeester commented Oct 30, 2019

Changes

Submitter Checklist

Reviewer Notes

vdemeester commented Oct 30, 2019

dibyom Oct 30, 2019

Choose a reason for hiding this comment

vdemeester Oct 30, 2019

Choose a reason for hiding this comment

dibyom Oct 30, 2019

Choose a reason for hiding this comment

vdemeester Oct 30, 2019

Choose a reason for hiding this comment

vdemeester commented Oct 30, 2019

vdemeester commented Oct 30, 2019

bobcatfish commented Oct 30, 2019

bobcatfish left a comment

Choose a reason for hiding this comment

tekton-robot commented Oct 30, 2019

vdemeester commented Oct 30, 2019

vdemeester commented Oct 30, 2019

vdemeester commented Oct 30, 2019

vdemeester commented Oct 30, 2019

vdemeester commented Oct 31, 2019

vdemeester commented Oct 31, 2019

dibyom commented Oct 31, 2019

vdemeester commented Oct 31, 2019

vdemeester commented Oct 31, 2019

afrittoli left a comment

Choose a reason for hiding this comment

afrittoli Nov 2, 2019

Choose a reason for hiding this comment

vdemeester Nov 4, 2019

Choose a reason for hiding this comment

afrittoli commented Nov 2, 2019

afrittoli commented Nov 2, 2019

vdemeester commented Nov 4, 2019

vdemeester commented Nov 4, 2019

vdemeester commented Nov 4, 2019

vdemeester commented Nov 4, 2019

vdemeester commented Nov 4, 2019

vdemeester commented Nov 4, 2019

vdemeester commented Nov 4, 2019

pritidesai commented Nov 4, 2019

vdemeester commented Nov 5, 2019

vdemeester Nov 5, 2019

Choose a reason for hiding this comment

tekton-robot commented Nov 5, 2019

abayer commented Nov 5, 2019