Allow Resources to be Optional in Task, ClusterTask, and Condition #1601

pritidesai · 2019-11-21T21:49:20Z

Changes

Task inputs and outputs are considered required, there is no way
today to mark them optional. This change introduced a new field called
optional as part of the PipelineResourceDeclaration by default a resource
is required. To mark any resource optional, set optional to true:

apiVersion: tekton.dev/v1alpha1
kind: Task
metadata:
 name: task-check-workspace
spec:
 inputs:
   resources:
     - name: workspace
       type: git
       optional: true
 steps:
   - name: check-workspace

These changes apply to Task, ClusterTask and Condition resources.

Closes #812

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.

Release Notes

Introducing a new key called optional as part of the PipelineResourceDeclaration by default a resource is required (as is - no change in behavior). To mark any resource optional, set optional to true:

apiVersion: tekton.dev/v1alpha1
kind: Task
metadata:
 name: task-check-workspace
spec:
 inputs:
   resources:
     - name: workspace
       type: git
       optional: true
 steps:
   - name: check-workspace

pritidesai · 2019-11-21T21:49:39Z

/wip

tekton-robot · 2019-11-21T21:51:56Z

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/taskrun/resources/input_resources.go	94.1%	92.5%	-1.7
pkg/reconciler/taskrun/resources/output_resource.go	90.5%	88.6%	-1.8
pkg/reconciler/taskrun/validate_resources.go	94.7%	93.7%	-1.1
test/builder/task.go	80.2%	80.7%	0.5

imjasonh · 2019-11-21T23:07:48Z

Hey Priti, thanks for this change!

Is there an issue or design doc to describe the motivations for this change? It seems useful but I want to make sure we've thought through some of the use cases and new error scenariros before we add this to the API

There's also a bit of work going on in the PipelineResources space at the moment, so this change might be easier to land after that dust has settled. Is there a particular use case you're trying to support, and how soon do you need it?

pritidesai · 2019-11-22T00:10:54Z

hey @imjasonh, sorry forgot to link the GitHub issue #812 and Design Doc, yup the motivation is included in there ...

tekton-robot · 2019-11-22T00:52: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/taskrun/resources/input_resources.go	94.1%	92.5%	-1.7
pkg/reconciler/taskrun/resources/output_resource.go	90.5%	90.9%	0.4
pkg/reconciler/taskrun/validate_resources.go	94.7%	93.7%	-1.1
test/builder/task.go	80.2%	80.7%	0.5

tekton-robot · 2019-11-22T03:36:57Z

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/taskrun/resources/input_resources.go	94.1%	94.3%	0.2
pkg/reconciler/taskrun/resources/output_resource.go	90.5%	90.9%	0.4
pkg/reconciler/taskrun/validate_resources.go	94.7%	93.7%	-1.1
test/builder/task.go	80.2%	80.7%	0.5

tekton-robot · 2019-11-22T04:20:25Z

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/taskrun/resources/input_resources.go	94.1%	94.3%	0.2
pkg/reconciler/taskrun/resources/output_resource.go	90.5%	90.9%	0.4
pkg/reconciler/taskrun/validate_resources.go	94.7%	95.2%	0.5
test/builder/task.go	80.2%	80.7%	0.5

tekton-robot · 2019-11-22T04:32:24Z

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/taskrun/resources/input_resources.go	94.1%	94.3%	0.2
pkg/reconciler/taskrun/resources/output_resource.go	90.5%	90.9%	0.4
pkg/reconciler/taskrun/validate_resources.go	94.7%	95.2%	0.5
test/builder/task.go	80.2%	80.7%	0.5

tekton-robot · 2019-11-22T18:27:04Z

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/taskrun/resources/input_resources.go	94.1%	94.3%	0.2
pkg/reconciler/taskrun/resources/output_resource.go	90.5%	90.9%	0.4
pkg/reconciler/taskrun/validate_resources.go	94.7%	95.2%	0.5
test/builder/task.go	80.2%	80.7%	0.5

tekton-robot · 2019-11-22T18:28:09Z

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/taskrun/resources/input_resources.go	94.1%	94.3%	0.2
pkg/reconciler/taskrun/resources/output_resource.go	90.5%	90.9%	0.4
pkg/reconciler/taskrun/validate_resources.go	94.7%	95.2%	0.5
test/builder/task.go	80.2%	80.7%	0.5

tekton-robot · 2019-11-22T19:57:30Z

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/taskrun/resources/input_resources.go	94.1%	94.3%	0.2
pkg/reconciler/taskrun/resources/output_resource.go	90.5%	90.9%	0.4
pkg/reconciler/taskrun/validate_resources.go	94.7%	95.2%	0.5
test/builder/task.go	80.2%	80.7%	0.5

pritidesai · 2019-11-22T20:21:15Z

its ready for review now ...

Please let me know if there are any changes needed 😄

examples/pipelineruns/conditional-pipelinerun-with-optional-resources.yaml

pkg/reconciler/taskrun/resources/input_resources.go

ghost · 2019-12-05T15:18:29Z

It would be great to add some doc updates here as well - in particular I'm a bit unclear on the expected behaviour of variable substitutions when a resource is optional.

tekton-robot · 2019-12-11T17:01:10Z

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
test/builder/task.go	81.4%	81.8%	0.4

pritidesai · 2019-12-11T17:12:09Z

failed again 😢

I1211 16:59:30.399] ---- Checking links in the markdown files ----
I1211 16:59:30.399] ----------------------------------------------
I1211 16:59:32.799] docs/resources.md
I1211 16:59:32.800] 	ERROR	https://godoc.org/github.com/jenkins-x/go-scm/scm#State
I1211 16:59:32.800] 		Not Found (HTTP error 404)

https://godoc.org/github.com/jenkins-x/go-scm/scm#State is accessible from the browser when I try.

vdemeester · 2019-12-11T18:06:04Z

/test pull-tekton-pipeline-build-tests
/shrug

👼 🙏

pritidesai · 2019-12-11T20:39:58Z

/retest

ghost · 2019-12-11T20:44:46Z

i've made a pr against plumbing to try disabling the md link checker: tektoncd/plumbing#154

not sure at the moment what else to do beyond removing the links.

pritidesai · 2019-12-11T21:01:49Z

was hoping (PR #1737 but now realizing its for nightly builds)to see coverage build fixed but doesnt look like 😞, still produces failure https://tekton-releases.appspot.com/build/tekton-prow/pr-logs/pull/tektoncd_pipeline/1601/pull-tekton-pipeline-go-coverage/1204807757975261187/?log#log

# github.com/tektoncd/pipeline/pkg/artifacts
pkg/artifacts/artifacts_storage.go:182: Errorf format %w has unknown verb w

dibyom

Thanks for adding this @pritidesai
I was a bit concerned about potential edge cases when the resource is missing but from looking at the code, it seems like that it would be up to the task author to ensure that their tasks behave correctly when a resource is missing!

dibyom · 2019-12-11T20:55:16Z

pkg/apis/pipeline/v1alpha2/resource_types.go

@@ -94,6 +94,11 @@ type ResourceDeclaration struct {
 	// will be copied.
 	// +optional
 	TargetPath string `json:"targetPath,omitempty"`
+	// Optional declares the resource as optional.


This is a tiny nitpick but I think the last two lines comment lines are not required as the first two are descriptive enough.

would like to keep them in as those two lines show the usage and its behavior vs first two lines explains the key itself.

dibyom · 2019-12-11T21:19:52Z

test/builder/task.go

@@ -239,20 +239,30 @@ func InputsResource(name string, resourceType v1alpha1.PipelineResourceType, ops
 	}
 }

+func ResourceOptional(optional bool) TaskResourceOp {


This is totally fine but is there a easier/simpler for doing test builders for boolean values? @vdemeester

I dont think so, its more about following our convention of building a resource, for example,

Required Resource:

tb.InputsResource("resource-to-build", v1alpha1.PipelineResourceTypeGit),

Optional Resource:

tb.InputsResource("optional-resource-to-build", v1alpha1.PipelineResourceTypeGit, tb.ResourceOptional(true)),

But would be ideal to refactor the way we initialize a resource in future, as we add more keys/fields, we end up adding setters for each.

tekton-robot · 2019-12-11T21:22:27Z

[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

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

dibyom · 2019-12-11T23:00:46Z

/test pull-tekton-pipeline-build-tests

vdemeester · 2019-12-12T08:42:34Z

/test pull-tekton-pipeline-build-tests

pritidesai · 2019-12-12T21:33:46Z

/retest

pritidesai · 2019-12-12T21:48:48Z

💃 the build succeeds 😄 thanks everyone for fixing it 👍

pritidesai · 2019-12-13T19:23:05Z

Thanks for adding this @pritidesai
I was a bit concerned about potential edge cases when the resource is missing but from looking at the code, it seems like that it would be up to the task author to ensure that their tasks behave correctly when a resource is missing!

Thanks @dibyom
Scenario 1: If a required resource is not specified in TaskRun, its behavior remains same, failure with Didn''t provide required values ...
Scenario 2: If an optional resource is not specified in TaskRun but referred to in a task, the task execution still succeeds and a pod displays message such as outputs.resources.optionalimage.url: command not found which is reasonable and its up to user how to handle such use case.

ghost · 2019-12-13T20:01:55Z

/lgtm

Pipeline inputs and outputs are considered required, there is no way today to mark them optional. This change introduces a new field called optional as part of the PipelineDeclaredResource similar to previous PR tektoncd#1601, by default optional is set to false and a resource is required. To mark any resource optional, set optional to true: apiVersion: tekton.dev/v1alpha1 kind: Pipeline metadata: name: pipeline-build-image spec: resources: - name: workspace type: git optional: true tasks: - name: check-workspace Closes tektoncd#1710

Pipeline inputs and outputs are considered required, there is no way today to mark them optional. This change introduces a new field called optional as part of the PipelineDeclaredResource similar to previous PR #1601, by default optional is set to false and a resource is required. To mark any resource optional, set optional to true: apiVersion: tekton.dev/v1alpha1 kind: Pipeline metadata: name: pipeline-build-image spec: resources: - name: workspace type: git optional: true tasks: - name: check-workspace Closes #1710

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 Nov 21, 2019

tekton-robot requested review from dlorenc and imjasonh November 21, 2019 21:49

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

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

pritidesai force-pushed the optional-resources branch from 7d0df85 to 256c8f6 Compare November 22, 2019 00:49

pritidesai force-pushed the optional-resources branch from 256c8f6 to f18866e Compare November 22, 2019 03:34

pritidesai force-pushed the optional-resources branch from f18866e to 338e68e Compare November 22, 2019 04:17

pritidesai force-pushed the optional-resources branch from 338e68e to 0980ceb Compare November 22, 2019 04:28

pritidesai force-pushed the optional-resources branch from 0980ceb to 93a00f6 Compare November 22, 2019 18:24

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 Nov 22, 2019

pritidesai force-pushed the optional-resources branch from 93a00f6 to 6c6cbce Compare November 22, 2019 18:26

pritidesai force-pushed the optional-resources branch from 6c6cbce to fd53501 Compare November 22, 2019 19:54

pritidesai changed the title ~~WIP: Allow Resources to be Optional in Task~~ Allow Resources to be Optional in Task, ClusterTask, and Condition Nov 22, 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 Nov 22, 2019

ghost reviewed Dec 5, 2019

View reviewed changes

examples/pipelineruns/conditional-pipelinerun-with-optional-resources.yaml Outdated Show resolved Hide resolved

pkg/reconciler/taskrun/resources/input_resources.go Outdated Show resolved Hide resolved

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

tekton-robot added the ¯\_(ツ)_/¯ ¯\\\_(ツ)_/¯ label Dec 11, 2019

dibyom approved these changes Dec 11, 2019

View reviewed changes

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

vdemeester removed the ¯\_(ツ)_/¯ ¯\\\_(ツ)_/¯ label Dec 12, 2019

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

tekton-robot merged commit ccacb0c into tektoncd:master Dec 13, 2019

pritidesai mentioned this pull request Jan 21, 2020

Enabling Pipeline Resources to be marked as Optional #1910

Merged

3 tasks

pritidesai deleted the optional-resources branch December 5, 2020 00:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow Resources to be Optional in Task, ClusterTask, and Condition #1601

Allow Resources to be Optional in Task, ClusterTask, and Condition #1601

pritidesai commented Nov 21, 2019 •

edited

Loading

pritidesai commented Nov 21, 2019

tekton-robot commented Nov 21, 2019

imjasonh commented Nov 21, 2019

pritidesai commented Nov 22, 2019

tekton-robot commented Nov 22, 2019

tekton-robot commented Nov 22, 2019

tekton-robot commented Nov 22, 2019

tekton-robot commented Nov 22, 2019

tekton-robot commented Nov 22, 2019

tekton-robot commented Nov 22, 2019

tekton-robot commented Nov 22, 2019

pritidesai commented Nov 22, 2019

ghost commented Dec 5, 2019

tekton-robot commented Dec 11, 2019

pritidesai commented Dec 11, 2019

vdemeester commented Dec 11, 2019

pritidesai commented Dec 11, 2019

ghost commented Dec 11, 2019

pritidesai commented Dec 11, 2019

dibyom left a comment

dibyom Dec 11, 2019

pritidesai Dec 13, 2019

dibyom Dec 11, 2019

pritidesai Dec 13, 2019

tekton-robot commented Dec 11, 2019

dibyom commented Dec 11, 2019

vdemeester commented Dec 12, 2019

pritidesai commented Dec 12, 2019

pritidesai commented Dec 12, 2019

pritidesai commented Dec 13, 2019

ghost commented Dec 13, 2019

Allow Resources to be Optional in Task, ClusterTask, and Condition #1601

Allow Resources to be Optional in Task, ClusterTask, and Condition #1601

Conversation

pritidesai commented Nov 21, 2019 • edited Loading

Changes

Submitter Checklist

Reviewer Notes

Release Notes

pritidesai commented Nov 21, 2019

tekton-robot commented Nov 21, 2019

imjasonh commented Nov 21, 2019

pritidesai commented Nov 22, 2019

tekton-robot commented Nov 22, 2019

tekton-robot commented Nov 22, 2019

tekton-robot commented Nov 22, 2019

tekton-robot commented Nov 22, 2019

tekton-robot commented Nov 22, 2019

tekton-robot commented Nov 22, 2019

tekton-robot commented Nov 22, 2019

pritidesai commented Nov 22, 2019

ghost commented Dec 5, 2019

tekton-robot commented Dec 11, 2019

pritidesai commented Dec 11, 2019

vdemeester commented Dec 11, 2019

pritidesai commented Dec 11, 2019

ghost commented Dec 11, 2019

pritidesai commented Dec 11, 2019

dibyom left a comment

Choose a reason for hiding this comment

dibyom Dec 11, 2019

Choose a reason for hiding this comment

pritidesai Dec 13, 2019

Choose a reason for hiding this comment

dibyom Dec 11, 2019

Choose a reason for hiding this comment

pritidesai Dec 13, 2019

Choose a reason for hiding this comment

tekton-robot commented Dec 11, 2019

dibyom commented Dec 11, 2019

vdemeester commented Dec 12, 2019

pritidesai commented Dec 12, 2019

pritidesai commented Dec 12, 2019

pritidesai commented Dec 13, 2019

ghost commented Dec 13, 2019

pritidesai commented Nov 21, 2019 •

edited

Loading