Adding support to enable resourceSpec #1324

pritidesai · 2019-09-17T18:42:44Z

Its now possible to embed resourceSpec into PipelineRun, for example:

apiVersion: tekton.dev/v1alpha1
kind: PipelineRun
metadata:
  name: pipelinerun-echo-greetings
spec:
  resources:
    - name: git-repo
        resourceRef:
          name: my-git-repo

Can be specified as:

apiVersion: tekton.dev/v1alpha1
kind: PipelineRun
  name: pipelinerun-echo-greetings
spec:
  resources:
    - name: git-repo
      resourceSpec:
        type: git
        params:
          - name: url
            value: https://github.com/myrepo/myrepo.git

Partially Fixes #872

Changes

This PR has lot of changes :) and WIP, still adding integration test.
First, added new fields in PipelineRunSpec (pipelinerun_types.go) and PipelineResourceBinding (resource_types.go). Followed by reading and resolving Inputs/Outputs to read specifications if specified. Also updated unit tests and added documentation.

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 and TaskRun 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

Describe any user facing changes here, or delete this block.

Examples of user facing changes:
- API changes
- Bug fixes
- Any changes in behavior

tekton-robot · 2019-09-17T18:42:59Z

Hi @pritidesai. Thanks for your PR.

I'm waiting for a tektoncd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

pritidesai · 2019-09-17T21:10:09Z

/wip

bobcatfish · 2019-09-17T23:30:09Z

@pritidesai this is very exciting!!! Thanks so much for working on this :D (And for including docs right off the bad!!)

Adding support to enable pipelineSpec and resourceSpec

Apologies for this request because it might be a bit frustrating to do, but would it be possible to separate embedding of the pipelineSpec and the resourceSpec into 2 separate PRs? I think we all agree that embedding resourceSpecs is something we want to do, but afaik we don't have total agreement about PipelineSpecs 🙏

pritidesai · 2019-09-18T18:18:42Z

hey @bobcatfish sure, I will create a separate PR 😢 what happened to adding pipelineSpec ?

Its a lot of code movement from one branch to another, would hope for a conclusion in near future on pipelineSpec.

dibyom · 2019-09-19T14:55:11Z

/ok-to-test

tekton-robot · 2019-09-19T14:57:13Z

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/resources/input_output_steps.go	95.2%	89.3%	-6.0
test/builder/pipeline.go	87.1%	87.3%	0.2

bobcatfish · 2019-09-19T15:00:30Z

I0919 14:55:47.116] ---- Checking links in the markdown files ----
I0919 14:55:47.116] ----------------------------------------------
I0919 14:56:13.854] docs/pipelineruns.md
I0919 14:56:13.854] 	ERROR	https://www.apache.org/licenses/LICENSE-2.0
I0919 14:56:13.854] 		Dialing to the given TCP address timed out
I0919 14:56:14.629] --------------------------------------

hmmm weird

/test pull-tekton-pipeline-build-tests

tekton-robot · 2019-09-19T18:51: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/resources/input_output_steps.go	95.2%	89.3%	-6.0
test/builder/pipeline.go	87.1%	87.3%	0.2

@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!

bobcatfish

It looks like some of the coverage went down a bit - i think the functions resolveConditionResources and ToTaskResourceBindings are each just missing a new test case. But again since #1353 is refactoring that anyway, I'm super happy to merge this as is! (Esp since it already includes great docs + examples + end to end tests!! ❤️ )

/lgtm

Gotta leave this on hold for at least one more OWNER to approve tho :)

bobcatfish · 2019-09-24T21:12:46Z

examples/pipelineruns/pipelinerun-with-resourcespec.yaml

+          - name: revision
+            value: master
+          - name: url
+            value: https://github.com/tektoncd/pipeline


great example!

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

test/builder/task.go

@@ -597,9 +597,6 @@ func TaskRunOutputsResource(name string, ops ...TaskResourceBindingOp) TaskRunOu
 	return func(i *v1alpha1.TaskRunOutputs) {
 		binding := &v1alpha1.TaskResourceBinding{
 			Name: name,
-			ResourceRef: v1alpha1.PipelineResourceRef{
-				Name: name,
-			},


thanks for fixing this!!! 🙏 pretty weird that we got away with it all this time 😅

bobcatfish · 2019-09-24T21:21:01Z

pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go

+					Name: resourceBinding.Name,
+				},
+				Spec: *resourceBinding.ResourceSpec,
+			}


Note that in #1353 I'm refactoring the PipelineRun resource resolution logic so it will all happen in one place - then we should only need to do this check in function instead of in a few different places :D

@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!

dibyom · 2019-09-24T21:27:07Z

pkg/reconciler/pipelinerun/resources/conditionresolution_test.go

@@ -290,9 +290,6 @@ func TestResolvedConditionCheck_ToTaskResourceBindings(t *testing.T) {

 	expected := []v1alpha1.TaskResourceBinding{{
 		Name: "git-resource",
-		ResourceRef: v1alpha1.PipelineResourceRef{


Why was this deleted?

Sometimes you want to have a ResourceBinding without a Ref (e.g. if you're using a spec!). So in the tests for embedded TaskRun Resource specs, this was being set even tho it shouldn't be

(The other function actually doesn't set this)

(Might be worth adding this to the commit message if it isn't there already @pritidesai !)

tekton-robot · 2019-09-24T21:27:16Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

@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!

@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!

bobcatfish · 2019-09-25T13:14:14Z

Looks like you hit the approve button @dibyom (vs. /approve) so imma merge!

/hold cancel

bobcatfish · 2019-09-25T13:14:28Z

I guess we'll just leave the commit message as is XD

bobcatfish · 2019-09-25T15:38:35Z

Looks like this might be a real failure @pritidesai !

sidecar_test.go:84: Creating Task "sidecar-test-task-1"
sidecar_test.go:89: Creating TaskRun "sidecar-test-task-run-1"
sidecar_test.go:99: Error waiting for TaskRun "sidecar-test-task-run-1" to finish: task run sidecar-test-task-run-1 failed!
sidecar_test.go:111: Error waiting for Pod "" to terminate both the primary and sidecar containers: resource name may not be empty
sidecar_test.go:116: Error getting TaskRun pod: resource name may not be empty
sidecar_test.go:140: Either the primary or sidecar containers did not terminate

But just in case:

/test pull-tekton-pipeline-integration-tests

pritidesai · 2019-09-25T15:50:24Z

Looks like this might be a real failure @pritidesai !

sidecar_test.go:84: Creating Task "sidecar-test-task-1"
sidecar_test.go:89: Creating TaskRun "sidecar-test-task-run-1"
sidecar_test.go:99: Error waiting for TaskRun "sidecar-test-task-run-1" to finish: task run sidecar-test-task-run-1 failed!
sidecar_test.go:111: Error waiting for Pod "" to terminate both the primary and sidecar containers: resource name may not be empty
sidecar_test.go:116: Error getting TaskRun pod: resource name may not be empty
sidecar_test.go:140: Either the primary or sidecar containers did not terminate

But just in case:

/test pull-tekton-pipeline-integration-tests

will check just in few, it has happened before where integration tests failed but then with retest, they succeeded, but will confirm.

@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!

@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!

bobcatfish · 2019-09-26T23:22:39Z

pkg/reconciler/pipelinerun/resources/conditionresolution.go

 			Name: name,
-			ResourceRef: v1alpha1.PipelineResourceRef{
+		}
+		if r.SelfLink != "" {


hey @pritidesai, somehow I didn't notice this when I looked before: what's the significance of selfLink here? it seems like its being used to decide if a resource should be embedded or not? 🤔

hey @pritidesai, somehow I didn't notice this when I looked before: what's the significance of selfLink here? it seems like its being used to decide if a resource should be embedded or not? 🤔

Hey @bobcatfish yes selfLink is the deciding factor here and most reliable, if resource is specified through reference (resourceRef), selfLink will never be empty and set to something like /apis/tekton.dev/pipelineresources/my-git-resource since my-git-resource is already created and can be referred to through this link. This was the best choice, as in inputs being type of map[string]*v1alpha1.PipelineResource in input_output_steps.go line 56 always had name set to the resource name and PipelineResource.name set to the same name as resource name even when a resource specification were given (using resourceSpec). Hope this helps.

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.

@pritidesai

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!

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.

Fixes tektoncd#240 PR tektoncd/pipeline#1324 makes it possible to embed PipelineResourceSpecs into a PipelineRun. This PR updates our PipelineRuns and TaskRuns to use an embedded PipelineResourceSpec. So, we will no longer encounter race conditions between the PipelineResources and PipelineRun/TaskRun. The Gopkg.toml file was updated to use revision 8871979dfc08fb65ae544c6ad2de83f8bab617b0 of tektoncd/pipeline, so we can ensure PR 1324 will be available.

tekton-robot requested review from abayer and dlorenc September 17, 2019 18:42

tekton-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Sep 17, 2019

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

tekton-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Sep 17, 2019

pritidesai force-pushed the pipeline-run-resource-spec branch from 25aa5b2 to 472784c Compare September 17, 2019 21:07

pritidesai force-pushed the pipeline-run-resource-spec branch 2 times, most recently from 929a705 to 36c8df9 Compare September 18, 2019 18:12

pritidesai force-pushed the pipeline-run-resource-spec branch from 36c8df9 to 97513fc Compare September 18, 2019 20:00

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

pritidesai force-pushed the pipeline-run-resource-spec branch 5 times, most recently from 6df069d to 7690da0 Compare September 18, 2019 21:53

pritidesai changed the title ~~Adding support to enable pipelineSpec and resourceSpec~~ Adding support to enable resourceSpec Sep 18, 2019

pritidesai force-pushed the pipeline-run-resource-spec branch from 7690da0 to cd9c522 Compare September 19, 2019 01:49

tekton-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 19, 2019

pritidesai force-pushed the pipeline-run-resource-spec branch from cd9c522 to 66d6625 Compare September 19, 2019 18:48

pritidesai force-pushed the pipeline-run-resource-spec branch from 66d6625 to a3f0f92 Compare September 19, 2019 18:53

bobcatfish mentioned this pull request Sep 24, 2019

Resolve all PipelineResources first before continuing #1353

Merged

2 tasks

bobcatfish reviewed Sep 24, 2019

View reviewed changes

tekton-robot assigned bobcatfish Sep 24, 2019

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

dibyom approved these changes Sep 24, 2019

View reviewed changes

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

tekton-robot merged commit bcbba97 into tektoncd:master Sep 25, 2019

bobcatfish reviewed Sep 26, 2019

View reviewed changes

This was referenced Sep 27, 2019

Webhooks extension PipelineRun sometimes doesn't have PipelineResources ready in time tektoncd/experimental#240

Closed

Import resource errors under race condition tektoncd/dashboard#469

Closed

ncskier mentioned this pull request Oct 17, 2019

Embed PipelineResources in PipelineRun and TaskRun tektoncd/experimental#296

Closed

2 tasks

vdemeester mentioned this pull request Oct 23, 2019

Describe task does not work for inline task spec tektoncd/cli#380

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding support to enable resourceSpec #1324

Adding support to enable resourceSpec #1324

pritidesai commented Sep 17, 2019 •

edited

Loading

tekton-robot commented Sep 17, 2019

pritidesai commented Sep 17, 2019

bobcatfish commented Sep 17, 2019

pritidesai commented Sep 18, 2019 •

edited

Loading

dibyom commented Sep 19, 2019

tekton-robot commented Sep 19, 2019

bobcatfish commented Sep 19, 2019

tekton-robot commented Sep 19, 2019

bobcatfish left a comment

bobcatfish Sep 24, 2019

bobcatfish Sep 24, 2019

bobcatfish Sep 24, 2019

dibyom Sep 24, 2019

bobcatfish Sep 25, 2019

tekton-robot commented Sep 24, 2019

bobcatfish commented Sep 25, 2019

bobcatfish commented Sep 25, 2019

bobcatfish commented Sep 25, 2019

pritidesai commented Sep 25, 2019

bobcatfish Sep 26, 2019

pritidesai Sep 26, 2019 •

edited

Loading

Adding support to enable resourceSpec #1324

Adding support to enable resourceSpec #1324

Conversation

pritidesai commented Sep 17, 2019 • edited Loading

Changes

Submitter Checklist

Reviewer Notes

Release Notes

tekton-robot commented Sep 17, 2019

pritidesai commented Sep 17, 2019

bobcatfish commented Sep 17, 2019

pritidesai commented Sep 18, 2019 • edited Loading

dibyom commented Sep 19, 2019

tekton-robot commented Sep 19, 2019

bobcatfish commented Sep 19, 2019

tekton-robot commented Sep 19, 2019

bobcatfish left a comment

Choose a reason for hiding this comment

bobcatfish Sep 24, 2019

Choose a reason for hiding this comment

bobcatfish Sep 24, 2019

Choose a reason for hiding this comment

bobcatfish Sep 24, 2019

Choose a reason for hiding this comment

dibyom Sep 24, 2019

Choose a reason for hiding this comment

bobcatfish Sep 25, 2019

Choose a reason for hiding this comment

tekton-robot commented Sep 24, 2019

bobcatfish commented Sep 25, 2019

bobcatfish commented Sep 25, 2019

bobcatfish commented Sep 25, 2019

pritidesai commented Sep 25, 2019

bobcatfish Sep 26, 2019

Choose a reason for hiding this comment

pritidesai Sep 26, 2019 • edited Loading

Choose a reason for hiding this comment

pritidesai commented Sep 17, 2019 •

edited

Loading

pritidesai commented Sep 18, 2019 •

edited

Loading

pritidesai Sep 26, 2019 •

edited

Loading