Resolve all PipelineResources first before continuing #1353

bobcatfish · 2019-09-24T21:19:40Z

Changes

Pre #1324

As part of #1184 I need to call GetSetup on all PipelineResources
early on in PipelineRun execution. Since PipelineRuns declare all their
resource up front, I wanted to be able to resolve all of them at once,
then call GetSetup on all of them. Also, as Pipelines got more complex
(we added Conditions) it turned out we were retrieving the resources in
a few different places. Also in #1324 @pritidesai is making it so that
these Resources can be provided by spec. By resolving all of this up
front at once, we can simplify the logic later on. And you can see in
this commit that we are able to reduce the responsibilities of
ResolvePipelineRun a bit too!

Post #1324

In #1324 we updated PipelineRuns to allow for embedding ResourceSpecs in
PipelineRuns. This commit makes it so that the logic for resolving (i.e.
deciding if PipelineResources are specified by Spec or Ref) is shared by
PipelineRuns + TaskRuns. This is done by making it so that the "binding"
uses the same type underneath. The only reason they can't be the exact
same type is that TaskRuns additionally need the "path" attribute, which
is actually only used for PVC copying, which will be removed in #1284,
and then we should be able to remove paths entirely and the type can be
the same.

Also added some additional comments around the use of SelfLink, and
made sure it was well covered in the reconciler test.

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)
[n/a] 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.

Release Notes

For folks importing the client libraries, when ResourceBindings are instantiated for TaskRuns, they now need to explicitly declare the newly embedded struct PipelineResourceBinding.

bobcatfish · 2019-09-24T21:20:25Z

@pritidesai if this causes any conflicts with #1324 I'm happy to deal with the conflicts! But also maybe #1324 can get merged first and then I can deal with them in this PR :D

tekton-robot · 2019-09-24T21:25:47Z

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/pipelinerun.go	82.4%	82.6%	0.1
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	95.0%	95.2%	0.3
pkg/reconciler/taskrun/resources/input_resources.go	91.7%	94.3%	2.7
pkg/reconciler/taskrun/resources/taskresourceresolution.go	100.0%	89.5%	-10.5

tekton-robot · 2019-09-24T21:38:36Z

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/pipelinerun.go	82.4%	82.6%	0.1
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	95.0%	95.2%	0.3
pkg/reconciler/taskrun/resources/input_resources.go	92.6%	95.7%	3.2

ghost

/lgtm
/hold

Am adding the hold just to make sure that the merge doesn't happen prior to #1324

ghost · 2019-09-25T13:52:22Z

pkg/reconciler/taskrun/resources/taskresourceresolution.go

+// getResource will return an instance of a PipelineResource to use for r, either by getting it with getter or by
+// instantiating it from the embedded spec.
+func getResource(r *v1alpha1.TaskResourceBinding, getter GetResource) (*v1alpha1.PipelineResource, error) {
+	// Check both resource ref or resource Spec are not present. Taskrun webhook should catch this in validation error.


Very minor nit (and I see that this func has just been moved from another place, so it's totally not required to fix this now) but the block of code below this is pretty self-explanatory. This comment probably isn't needed?

ghost · 2019-09-25T13:52:43Z

pkg/reconciler/taskrun/resources/taskresourceresolution.go

+			Spec: *r.ResourceSpec,
+		}, nil
+	}
+	return nil, xerrors.New("Neither ResourseRef not ResourceSpec is defined")


not -> nor

vdemeester

/lgtm
/meow boxes

tekton-robot · 2019-09-25T14:19:27Z

@vdemeester:

In response to this:

/lgtm
/meow boxes

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.

tekton-robot · 2019-09-25T14:19:30Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

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

bobcatfish · 2019-09-25T21:13:24Z

I think the meow plugin broke when we updated Prow 😭 This is my theory anyway. Here's a cat (Maru?) in a box to make up for it

ghost · 2019-09-26T14:35:36Z

/lgtm

@pritidesai

As part of tektoncd#1184 I need to call `GetSetup` on all PipelineResources early on in PipelineRun execution. Since PipelineRuns declare all their resource up front, I wanted to be able to resolve all of them at once, then call `GetSetup` on all of them. Also, as Pipelines got more complex (we added Conditions) it turned out we were retrieving the resources in a few different places. Also in tektoncd#1324 @pritidesai is making it so that these Resources can be provided by spec. By resolving all of this up front at once, we can simplify the logic later on. And you can see in this commit that we are able to reduce the responsibilities of ResolvePipelineRun a bit too!

tekton-robot · 2019-09-26T19:53:02Z

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/pipelinerun.go	82.4%	82.6%	0.1
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	92.6%	95.2%	2.7
pkg/reconciler/taskrun/resources/input_resources.go	92.6%	95.7%	3.2

bobcatfish · 2019-09-26T23:52:32Z

/hold

still re-doing some things post #1324

tekton-robot · 2019-09-27T20:39:18Z

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/pipelinerun.go	82.4%	82.6%	0.1
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	92.6%	95.2%	2.7
pkg/reconciler/taskrun/resources/input_resources.go	92.6%	95.7%	3.2

tekton-robot · 2019-09-27T20:52:44Z

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/pipelinerun.go	82.4%	82.6%	0.1
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	92.6%	95.2%	2.7
pkg/reconciler/taskrun/resources/input_resources.go	92.6%	95.7%	3.2

bobcatfish · 2019-09-27T20:54:02Z

Okay! Now rebased on #1324, PTAL @pritidesai @sbwsg @vdemeester (note @vdemeester note the potential impact on the client usage for the CLI - maybe dashboard too? - I can avoid this by instead duplicating the logic to determine if a binding is a Ref or a Spec, but once we get to #1284 we'll probably remove "path" from the TaskRun binding so it's gonna change at that point anyway. Lemme know if you feel strongly and plz tag anyone else you think would be interested.

tekton-robot · 2019-09-27T20:54:08Z

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/pipelinerun.go	82.4%	82.6%	0.1
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	92.6%	95.2%	2.7
pkg/reconciler/taskrun/resources/input_resources.go	92.6%	95.7%	3.2

pritidesai · 2019-09-27T21:23:33Z

/lgtm

This is great and works like a charm, thanks @bobcatfish for the simplification, the code is much more readable now ❤️

vdemeester · 2019-09-30T08:39:34Z

note @vdemeester note the potential impact on the client usage for the CLI - maybe dashboard too?

The API doesn't change, so this is just a matter of bump the dependency. But both an old client or new client will still work against the API, so no need to duplicate things 😉

ghost · 2019-09-30T14:07:15Z

/lgtm

bobcatfish · 2019-10-01T13:59:59Z

/hold cancel

Thanks for the feedback all!!

bobcatfish · 2019-10-01T14:09:27Z

why hello stronger linting, don't mind if i do

I1001 14:05:29.226] pkg/reconciler/pipelinerun/resources/pipelinerunresolution_test.go:1062:9: elseif: can replace 'else {if cond {}}' with 'else if cond {}' (gocritic)
I1001 14:05:29.227] 	} else {
I1001 14:05:29.227] 	       ^
I1001 14:05:29.228] pkg/reconciler/pipelinerun/resources/pipelinerunresolution_test.go:1071:9: elseif: can replace 'else {if cond {}}' with 'else if cond {}' (gocritic)
I1001 14:05:29.228] 	} else {

In tektoncd#1324 we updated PipelineRuns to allow for embedding ResourceSpecs in PipelineRuns. This commit makes it so that the logic for resolving (i.e. deciding if PipelineResources are specified by Spec or Ref) is shared by PipelineRuns + TaskRuns. This is done by making it so that the "binding" uses the same type underneath. The only reason they can't be the exact same type is that TaskRuns additionally need the "path" attribute, which is actually only used for PVC copying, which will be removed in tektoncd#1284, and then we should be able to remove paths entirely and the type can be the same. Also added some additional comments around the use of `SelfLink`, and made sure it was well covered in the reconciler test.

tekton-robot · 2019-10-01T14:28: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/pipelinerun.go	82.4%	82.6%	0.1
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	92.6%	95.2%	2.7
pkg/reconciler/taskrun/resources/input_resources.go	92.6%	95.7%	3.2

ghost · 2019-10-01T14:30:04Z

/lgtm

tekton-robot requested review from abayer and dibyom September 24, 2019 21:19

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

tekton-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 24, 2019

bobcatfish mentioned this pull request Sep 24, 2019

Adding support to enable resourceSpec #1324

Merged

3 tasks

bobcatfish force-pushed the follow_the_resources branch from cae91a6 to 511d009 Compare September 24, 2019 21:23

bobcatfish force-pushed the follow_the_resources branch from 511d009 to 43b3ef1 Compare September 24, 2019 21:36

tekton-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 24, 2019

ghost reviewed Sep 25, 2019

View reviewed changes

tekton-robot assigned ghost Sep 25, 2019

tekton-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Sep 25, 2019

vdemeester approved these changes Sep 25, 2019

View reviewed changes

tekton-robot assigned vdemeester Sep 25, 2019

bobcatfish force-pushed the follow_the_resources branch from 43b3ef1 to a4ef169 Compare September 25, 2019 21:10

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

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

bobcatfish force-pushed the follow_the_resources branch from a4ef169 to 37d5327 Compare September 26, 2019 19:50

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

pritidesai mentioned this pull request Sep 26, 2019

Allow embedding specs in Pipeline and PipelineRun instead of referencing. #872

Closed

tekton-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Sep 27, 2019

bobcatfish force-pushed the follow_the_resources branch 2 times, most recently from 10b6f5b to ef5c635 Compare September 27, 2019 20:50

tekton-robot assigned pritidesai Sep 27, 2019

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 27, 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 1, 2019

bobcatfish force-pushed the follow_the_resources branch from ef5c635 to aa4fb0a Compare October 1, 2019 14:25

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

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

tekton-robot merged commit 73ba02b into tektoncd:master Oct 1, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resolve all PipelineResources first before continuing #1353

Resolve all PipelineResources first before continuing #1353

bobcatfish commented Sep 24, 2019 •

edited

Loading

bobcatfish commented Sep 24, 2019

tekton-robot commented Sep 24, 2019

tekton-robot commented Sep 24, 2019

ghost left a comment

ghost Sep 25, 2019

ghost Sep 25, 2019

vdemeester left a comment

tekton-robot commented Sep 25, 2019

tekton-robot commented Sep 25, 2019

bobcatfish commented Sep 25, 2019

ghost commented Sep 26, 2019

tekton-robot commented Sep 26, 2019

bobcatfish commented Sep 26, 2019

tekton-robot commented Sep 27, 2019

tekton-robot commented Sep 27, 2019

bobcatfish commented Sep 27, 2019

tekton-robot commented Sep 27, 2019

pritidesai commented Sep 27, 2019

vdemeester commented Sep 30, 2019

ghost commented Sep 30, 2019

bobcatfish commented Oct 1, 2019

bobcatfish commented Oct 1, 2019

tekton-robot commented Oct 1, 2019

ghost commented Oct 1, 2019

Resolve all PipelineResources first before continuing #1353

Resolve all PipelineResources first before continuing #1353

Conversation

bobcatfish commented Sep 24, 2019 • edited Loading

Changes

Pre #1324

Post #1324

Submitter Checklist

Reviewer Notes

Release Notes

bobcatfish commented Sep 24, 2019

tekton-robot commented Sep 24, 2019

tekton-robot commented Sep 24, 2019

ghost left a comment

Choose a reason for hiding this comment

ghost Sep 25, 2019

Choose a reason for hiding this comment

ghost Sep 25, 2019

Choose a reason for hiding this comment

vdemeester left a comment

Choose a reason for hiding this comment

tekton-robot commented Sep 25, 2019

tekton-robot commented Sep 25, 2019

bobcatfish commented Sep 25, 2019

ghost commented Sep 26, 2019

tekton-robot commented Sep 26, 2019

bobcatfish commented Sep 26, 2019

tekton-robot commented Sep 27, 2019

tekton-robot commented Sep 27, 2019

bobcatfish commented Sep 27, 2019

tekton-robot commented Sep 27, 2019

pritidesai commented Sep 27, 2019

vdemeester commented Sep 30, 2019

ghost commented Sep 30, 2019

bobcatfish commented Oct 1, 2019

bobcatfish commented Oct 1, 2019

tekton-robot commented Oct 1, 2019

ghost commented Oct 1, 2019

bobcatfish commented Sep 24, 2019 •

edited

Loading