Allow declaring and passing resources to conditions by dibyom · Pull Request #1151 · tektoncd/pipeline

dibyom · 2019-08-02T21:36:34Z

Changes

Allow condition to declare pipeline resources in their definitions.
The resource can be accessed by the condition container during a check.
Only input resources are supported. The declared resources can be specified
in a PipelineTaskCondition and variable substitution for the resource is
supported just like with a TaskResource.

Fixes #1021

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

Release Notes

Adds resource support for conditionals. Conditions have to declare 
the resources they can use and these can be passed in via the
Pipeline and PipelineRun spec similar to TaskResources.

tekton-robot · 2019-08-02T21:38:51Z

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/v1alpha1/pipelinerun/resources/conditionresolution.go	80.0%	71.4%	-8.6
pkg/reconciler/v1alpha1/pipelinerun/resources/pipelinerunresolution.go	90.5%	85.9%	-4.6

tekton-robot · 2019-08-05T20:39:35Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dibyom

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [dibyom]

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

tekton-robot · 2019-08-05T20:42:07Z

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/v1alpha1/pipelinerun/pipelinerun.go	Do not exist	82.3%
pkg/reconciler/v1alpha1/pipelinerun/resources/conditionresolution.go	80.0%	76.9%	-3.1
pkg/reconciler/v1alpha1/pipelinerun/resources/pipelinerunresolution.go	90.5%	85.9%	-4.6

tekton-robot · 2019-08-06T15:16:32Z

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/v1alpha1/pipelinerun/pipelinerun.go	Do not exist	82.1%
pkg/reconciler/v1alpha1/pipelinerun/resources/conditionresolution.go	80.0%	83.6%	3.6
pkg/reconciler/v1alpha1/pipelinerun/resources/pipelinerunresolution.go	90.5%	91.1%	0.6

tekton-robot · 2019-08-06T18:53:33Z

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/v1alpha1/pipelinerun/pipelinerun.go	Do not exist	82.2%
pkg/reconciler/v1alpha1/pipelinerun/resources/conditionresolution.go	80.0%	83.6%	3.6
pkg/reconciler/v1alpha1/pipelinerun/resources/pipelinerunresolution.go	90.5%	91.1%	0.6

tekton-robot · 2019-08-06T19:05:03Z

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/v1alpha1/pipelinerun/pipelinerun.go	Do not exist	82.2%
pkg/reconciler/v1alpha1/pipelinerun/resources/conditionresolution.go	80.0%	83.6%	3.6
pkg/reconciler/v1alpha1/pipelinerun/resources/pipelinerunresolution.go	90.5%	91.1%	0.6

bobcatfish

Pretty cool! I agree with adding this functionality for sure :D I left a few comments, mostly just some refactoring ideas.

Couple other thoughts:

Looks like there are a couple lines in conditionresolution.go not covered that would probably be easy to cover with a couple extra test cases
I merged #1172 so any comments, examples using ${} should use $() instead (and it should support both but as long as you're calling the same logic it should be fine! might want to add a test case or two just in case) 😭 🙏

bobcatfish · 2019-08-12T16:07:21Z

docs/conditions.md

+provide the Condition container step with data or context that is needed to perform the check.
+
+Input resources, like source code (git), are dumped at path
+`/workspace/resource_name` within a mounted


Hm I'm thinking that we should create a separate section in the resources.md docs for how the paths work - the docs are a bit spread out, e.g.

https://github.com/tektoncd/pipeline/blob/master/docs/tasks.md#controlling-where-resources-are-mounted
and
https://github.com/tektoncd/pipeline/blob/master/docs/taskruns.md#overriding-where-resources-are-copied-from
And even the path templating (like you said!)
https://github.com/tektoncd/pipeline/blob/master/docs/tasks.md#templating

But I think/hope that both of those docs would apply to this use of resources as well? In which case it'd be good if we can put all those docs in one place and link to them from here and from the other places

🙏

Yeah, the thing is since things like targetPath are part of TaskResource, they are in the tasks doc. Now with conditions, there is some duplication. I'll add a section in the resource doc on how to use resources in conditions and tasks. That section will link to both the Task and Condition resource docs and contain the common bits e.g. targetPaths, the path variable substitution.

I'm thinking of doing this in a follow-up PR.

examples/pipelineruns/conditional-pipelinerun.yaml

bobcatfish · 2019-08-12T17:37:29Z

pkg/apis/pipeline/v1alpha1/condition_types.go

+	// TargetPath is the path in workspace directory where the resource
+	// will be copied.
+	// +optional
+	TargetPath string `json:"targetPath"`


when i first saw this i wondered, why not use the existing type?

pipeline/pkg/apis/pipeline/v1alpha1/task_types.go

Lines 109 to 122 in bb85571

type TaskResource struct {

// Name declares the name by which a resource is referenced in the Task's

// definition. Resources may be referenced by name in the definition of a

// Task's steps.

Name string `json:"name"`

// Type is the type of this resource;

Type PipelineResourceType `json:"type"`

// TargetPath is the path in workspace directory where the task resource

// will be copied.

// +optional

TargetPath string `json:"targetPath"`

// Path to the index.json file for output container images.

// +optional

OutputImageDir string `json:"outputImageDir"`

I'm guessing it's b/c of the OutputImageDir field on the condition resource, is that correct? if that's the only reason then im thinking maybe we could do something like:

Define one type (maybe called something like ResourceInput? or something better XD)

Use this new type directly for conditions

Make it so that TaskResource embeds the type and adds OutputImageDir

Maybe open an issue to explore changing OutputImageDir (I'm not a big fan of the fact that the generic struct TypeResource has an image specific field :S)

Maybe open an issue to explore changing OutputImageDir (I'm not a big fan of the fact that the generic struct TypeResource has an image specific field :S)

We can probably leave that for now - between the resource extensibility design and a proposal im making for treating output images differently i think we're gonna get it fixed :D

but in the meantime embedding might be an okay solution!

Nice! I like this a lot better!!!
(And yeah the original reason was to avoid the OutputImageDir field

Done! ResourceInput might be confusing with Input Resources so I chose ResourceDeclaration though maybe DecalaredResource is better 🤔

Just don't tell @dlorenc that i recommended embedding.... 😇

🤣 🤣 🤣

pkg/reconciler/v1alpha1/pipelinerun/pipelinerun.go

pkg/reconciler/v1alpha1/pipelinerun/resources/conditionresolution.go

bobcatfish · 2019-08-12T17:47:10Z

pkg/reconciler/v1alpha1/pipelinerun/resources/conditionresolution.go

+}
+
+// Prepends inputs. to all resource template strings so that they can be replaced by
+// taskrun variable substitution e.g. ${resources.name.key} to ${inputs.resources.name.key}


hm this seems like a bit of a hack 😇 im thinking in the long run we should refactor this so we can call the templating (now called substitution 😅 ) functions without needing this hack first (np if we want to do this in a separate PR or even open an issue about it)

Indeed pretty hacky! I think it should be simple enough to do the resource substitution completely here. I'll give it a shot in this PR

pkg/reconciler/v1alpha1/pipelinerun/resources/conditionresolution.go

bobcatfish · 2019-08-12T17:51:07Z

pkg/reconciler/v1alpha1/pipelinerun/resources/pipelinerunresolution.go

+				// Finally add it to the resolved resources map
+				rr[r.Name] = gotResource
+			}
+			rcc.ResolvedResources = rr


this kinda feels like it could be a separate function, what do you think?

resolveConditionChecks could even become ResolveConditionChecks and be called by the reconciler after calling ResolvePipelineRun - that would make it so ResolvePipelineRun and its unit tests could be simpler, and you'd no longer need to distinguish between different kinds of errors

ok, I split the resource resolution into a separate function. There is definitely a lot of scope to refactor the pipelineState resolution -- besides resolving the conditionChecks separately, another thing that might help (particularly with #1023) would be changing the PipelineState from just being a slice of the ResolvedTasks to having a reference to the Dag nodes.

There is definitely a lot of scope to refactor the pipelineState resolution

Yeah probably too much to do all at ounce - but thanks for the incremental improvement!

tekton-robot · 2019-08-16T13:45:53Z

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/v1alpha1/pipelinerun/pipelinerun.go	82.4%	82.2%	-0.2
pkg/reconciler/v1alpha1/pipelinerun/resources/conditionresolution.go	80.0%	83.9%	3.9
pkg/reconciler/v1alpha1/pipelinerun/resources/pipelinerunresolution.go	94.5%	94.9%	0.3

bobcatfish · 2019-08-16T15:31:09Z

I0816 14:00:38.929] ERROR: test demo-condtional-pr-list-files-1-gknrc-file-exists-68bvx=SucceededFalse but should be succeededtrue
I0816 14:00:38.930] ERROR: one or more YAML tests failed

ooo that might be legit

bobcatfish · 2019-08-16T15:31:16Z

/test pull-tekton-pipeline-integration-tests

tekton-robot · 2019-08-26T21:03:38Z

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	Do not exist	82.2%
pkg/reconciler/pipelinerun/resources/conditionresolution.go	80.0%	83.9%	3.9
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	94.5%	94.9%	0.3

tekton-robot · 2019-08-26T23:01:33Z

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	Do not exist	82.2%
pkg/reconciler/pipelinerun/resources/conditionresolution.go	80.0%	83.3%	3.3
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	94.5%	94.9%	0.3

dibyom · 2019-08-26T23:05:31Z

/hold

pkg/apis/pipeline/v1alpha1/pipeline_types.go

docs/conditions.md

pkg/apis/pipeline/v1alpha1/task_validation_test.go

pkg/reconciler/pipelinerun/resources/conditionresolution.go

pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go

pkg/reconciler/pipelinerun/resources/pipelinerunresolution_test.go

test/builder/condition.go

afrittoli · 2019-08-29T10:58:54Z

Thanks for this! I will review it now. A quick note, there are three commits in this PR, do you plan to squash or if not could you amend the commit messages of the last two commits?

dibyom · 2019-08-29T17:21:20Z

@afrittoli Thanks...Yes, will squash and also going to make a couple of changes later today based on @bobcatfish 's review comment.

tekton-robot · 2019-08-29T22:38:14Z

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/apis/pipeline/v1alpha1/resource_paths.go	Do not exist	100.0%
pkg/reconciler/pipelinerun/pipelinerun.go	Do not exist	82.2%
pkg/reconciler/pipelinerun/resources/conditionresolution.go	80.0%	83.6%	3.6
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	94.5%	94.9%	0.3
pkg/reconciler/taskrun/resources/apply.go	97.6%	100.0%	2.4

Allow condition to declare pipeline resources in their definitions. The resource can be accessed by the condition container during a check. Only input resources are supported. The declared resources can be specified in a PipelineTaskCondition and variable substitution for the resource is supported just like with a `TaskResource`. Signed-off-by: Dibyo Mukherjee <dibyo@google.com>

Creates a new internal struct called `ResourceDeclaration`. TaskResources now embed this and add additional fields such as `OutputImageDir` while `ConditionResource` has simply been replaced with this new type. Signed-off-by: Dibyo Mukherjee <dibyo@google.com>

Previously, we'd change the variable names to what the taskrun format and then rely on the taskrun's variable substitution logic. This approach is hacky and now we explicitly replace the resource variable strings with the right values without relying on the TaskRun logic. In the future, we should consider doing the same for parameter variables as well. Signed-off-by: Dibyo Mukherjee <dibyo@google.com>

tekton-robot · 2019-08-30T15:46:05Z

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/apis/pipeline/v1alpha1/resource_paths.go	Do not exist	100.0%
pkg/reconciler/pipelinerun/pipelinerun.go	94.4%	82.2%	-12.2
pkg/reconciler/pipelinerun/resources/conditionresolution.go	80.0%	83.6%	3.6
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	94.5%	95.0%	0.4
pkg/reconciler/taskrun/resources/apply.go	97.6%	100.0%	2.4

dibyom · 2019-08-30T15:47:07Z

/hold cancel

dibyom · 2019-08-30T16:04:28Z

pkg/reconciler/pipelinerun/pipelinerun.go	94.4%	82.2%	-12.2

🤔

dibyom · 2019-08-30T16:04:59Z

/test pull-tekton-pipeline-build-tests

bobcatfish

I left a couple super minor comments! I was gonna lgtm but I just saw the -12.2% coverage thing so maybe I'll wait for that XD

bobcatfish · 2019-08-30T15:59:09Z

docs/pipelines.md

+            value: my-value
+        resources:
+          - name: workspace
+            resource: source-repo


Hmm something just occurred to me - can conditions use the from clause for resources? If there is more work to do here I totally would expect that in a follow up PR, but basically what I'm thinking is that if a Task is getting a resource from another Task, but has some condition to execute on that resource, the user would probably want the condition to be executed on that same task

e.g. a Task has a git output (assuming a world where a git output creates a new commit), another Task gets the git resource from that task, but the condition says only if the most recent commit changed XYZ files (kind of contrived but maybe??)

bobcatfish · 2019-08-30T16:00:19Z

pkg/apis/pipeline/v1alpha1/pipeline_types.go

+type PipelineConditionResource struct {
+	// Name is the name of the PipelineResource as declared by the Condition.
+	Name string `json:"name"`
+	// Resource is the name of the DeclaredPipelineResource to use.


hm this is kinda odd but also really minor - these comments refer to DeclaredPipelineResource but I don't think that DeclaredPipelineResource is a type that's actually defined anywhere?

(Maybe this is ResourceDeclaration)?

bobcatfish · 2019-08-30T16:00:47Z

pkg/apis/pipeline/v1alpha1/resource_paths_test.go

+			}
+		})
+	}
+}


haha yeessssss i love the super focused test coverage XD

bobcatfish · 2019-08-30T16:01:11Z

pkg/apis/pipeline/v1alpha1/resource_paths_test.go

+	for _, tc := range tcs {
+		t.Run(tc.name, func(t *testing.T) {
+			if actual := v1alpha1.OutputResourcePath(tc.resource); actual != tc.expected {
+				t.Errorf("Unexpected output resource path: %s", actual)


can be helpful to included the expected path in the error message as well

bobcatfish · 2019-08-30T16:01:45Z

pkg/apis/pipeline/v1alpha1/task_types.go

-	// +optional
-	TargetPath string `json:"targetPath,omitempty"`
-	// Path to the index.json file for output container images.
+	ResourceDeclaration


dibyom · 2019-08-30T17:11:38Z

/test pull-tekton-pipeline-go-coverage

tekton-robot · 2019-08-30T17:13:39Z

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/apis/pipeline/v1alpha1/resource_paths.go	Do not exist	100.0%
pkg/reconciler/pipelinerun/pipelinerun.go	94.4%	82.2%	-12.2
pkg/reconciler/pipelinerun/resources/conditionresolution.go	80.0%	83.6%	3.6
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	94.5%	95.0%	0.4
pkg/reconciler/taskrun/resources/apply.go	97.6%	100.0%	2.4

dibyom · 2019-08-30T18:01:11Z

I just saw the -12.2% coverage thing so maybe I'll wait for that XD

Not sure this is accurate. The coverage for that file is around 82.2% according to another recently updated PR: #1204 (comment)

Trying to run this locally fails with a data race :(
(I think this is the same issue as #1124)

bobcatfish · 2019-08-30T18:10:53Z

Something might be up with that coverage bot anyway #1248

bobcatfish · 2019-08-30T18:11:00Z

/lgtm

tekton-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 2, 2019

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

tekton-robot requested review from bobcatfish and dlorenc August 2, 2019 21:36

tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 2, 2019

dibyom changed the title ~~WIP: Allow declaring and passing resources to conditions~~ Allow declaring and passing resources to conditions Aug 2, 2019

tekton-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 2, 2019

dibyom changed the title ~~Allow declaring and passing resources to conditions~~ WIP: Allow declaring and passing resources to conditions Aug 2, 2019

tekton-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 2, 2019

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

dibyom force-pushed the cond-resources branch from ffe4c2e to 0587fdb Compare August 5, 2019 20:39

dibyom force-pushed the cond-resources branch from 574207a to 1ee4780 Compare August 6, 2019 19:01

dibyom changed the title ~~WIP: Allow declaring and passing resources to conditions~~ Allow declaring and passing resources to conditions Aug 6, 2019

tekton-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 6, 2019

bobcatfish reviewed Aug 12, 2019

View reviewed changes

dibyom force-pushed the cond-resources branch from 1ee4780 to 1a13076 Compare August 16, 2019 13:40

dibyom force-pushed the cond-resources branch from 1a13076 to 6ffa047 Compare August 26, 2019 21:01

dibyom force-pushed the cond-resources branch from 6ffa047 to 6fe5b0a Compare August 26, 2019 22:59

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 Aug 26, 2019

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

ghost reviewed Aug 27, 2019

View reviewed changes

dibyom force-pushed the cond-resources branch from 6fe5b0a to 2492702 Compare August 29, 2019 22:36

dibyom added 3 commits August 30, 2019 11:40

dibyom force-pushed the cond-resources branch from 2492702 to f06760b Compare August 30, 2019 15:43

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

bobcatfish reviewed Aug 30, 2019

View reviewed changes

tekton-robot assigned bobcatfish Aug 30, 2019

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

tekton-robot merged commit ecddd2f into tektoncd:master Aug 30, 2019

dibyom deleted the cond-resources branch August 30, 2019 18:18

This was referenced Aug 30, 2019

Support from clause for resources in Conditionals #1256

Closed

Add a using resources section in resources.md #1257

Merged

	type TaskResource struct {
	// Name declares the name by which a resource is referenced in the Task's
	// definition. Resources may be referenced by name in the definition of a
	// Task's steps.
	Name string `json:"name"`
	// Type is the type of this resource;
	Type PipelineResourceType `json:"type"`
	// TargetPath is the path in workspace directory where the task resource
	// will be copied.
	// +optional
	TargetPath string `json:"targetPath"`
	// Path to the index.json file for output container images.
	// +optional
	OutputImageDir string `json:"outputImageDir"`

Conversation

dibyom commented Aug 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Submitter Checklist

Release Notes

Uh oh!

tekton-robot commented Aug 2, 2019

Uh oh!

tekton-robot commented Aug 5, 2019

Uh oh!

tekton-robot commented Aug 5, 2019

Uh oh!

tekton-robot commented Aug 6, 2019

Uh oh!

tekton-robot commented Aug 6, 2019

Uh oh!

tekton-robot commented Aug 6, 2019

Uh oh!

bobcatfish left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tekton-robot commented Aug 16, 2019

Uh oh!

bobcatfish commented Aug 16, 2019

Uh oh!

bobcatfish commented Aug 16, 2019

Uh oh!

tekton-robot commented Aug 26, 2019

Uh oh!

tekton-robot commented Aug 26, 2019

Uh oh!

dibyom commented Aug 26, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

afrittoli commented Aug 29, 2019

Uh oh!

dibyom commented Aug 29, 2019

Uh oh!

tekton-robot commented Aug 29, 2019

dibyom commented Aug 2, 2019 •

edited

Loading