Support skipping single task only

[jerop]

Changes

When WhenExpressions evaluate to False, the guarded Task and
its branch (dependent Tasks) are skipped

A Task is dependent on and in the branch of another Task as specified
by ordering using runAfter or by resources using Results, Workspaces
and Resources

In some use cases, when WhenExpressions evaluate to False,
users need to skip the guarded Task only and allow ordering-dependent
Tasks to execute

When WhenExpressions evaluate to False, it is possible to allow for
execution of ordering-dependent Tasks as specified by runAfter using
the whenSkipped field by setting it to runBranch.

However, setting whenSkipped in Tasks without WhenExpressions
or Tasks with resource dependencies is invalid, and will cause
Pipeline validation errors. This ensures consistency in executing or
skipping subsequent tasks.

Further details in TEP

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 block has been filled in or deleted (only if no user facing changes)

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

When  `WhenExpressions` evaluate to `False`, it is possible to allow
execution of ordering-dependent `Tasks` as specified by `runAfter`
using the `whenSkipped` field by setting it to `runBranch`

[jerop]

/kind feature

[tekton-robot]

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

File	Old Coverage	New Coverage	Delta
internal/builder/v1beta1/pipeline.go	82.0%	81.1%	-0.9
pkg/apis/pipeline/v1beta1/pipeline_validation.go	97.4%	97.5%	0.1
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	90.3%	90.3%	-0.0

[tekton-robot]

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

File	Old Coverage	New Coverage	Delta
internal/builder/v1beta1/pipeline.go	82.0%	81.1%	-0.9
pkg/apis/pipeline/v1beta1/pipeline_validation.go	97.4%	97.5%	0.1
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	90.3%	90.3%	-0.0

[tekton-robot]

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

File	Old Coverage	New Coverage	Delta
internal/builder/v1beta1/pipeline.go	82.0%	82.2%	0.2
pkg/apis/pipeline/v1beta1/pipeline_validation.go	97.4%	97.5%	0.1
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	90.3%	90.3%	-0.0

[tekton-robot]

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

File	Old Coverage	New Coverage	Delta
internal/builder/v1beta1/pipeline.go	82.0%	82.2%	0.2
pkg/apis/pipeline/v1beta1/pipeline_validation.go	97.4%	97.5%	0.1
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	90.3%	90.3%	-0.0

[jerop]

runAfter is meant to be used to specify ordering when there are no resource dependencies - https://github.com/tektoncd/pipeline/blob/master/docs/pipelines.md#using-the-runafter-parameter

however, it seems that Workspaces don't have a way to specify resource dependencies so they use runAfter, further detail in #3109

need to look into what this means for skipping ordering-dependent tasks as specified by runAfter

a potential solution could be introducing a field - similar to runAfter - for specifying resource dependencies such as Workspaces that don't use the from or string substitution syntax

@bobcatfish @sbwsg

/hold

[bobcatfish]

@jerop i think for now it's fine to treat runAfter as the same for all circumstances - however if you wanted to tackle #3109 before this that'd be cool too. basically i dont think either needs to wait on the other

this would only be a problem for a fairly specific use case:

1. Someone has a workspace coming "from" another Task, e.g. task B needs workspace X from task A, and this is expressed using runAfter
2. That person uses a when expression on task A, and sets continueAfterSkip: true

imo (2) is actually an error - and i think if you did the same thing with a result, we'd fail it on validation, is that right? So not having #3109 before doing this would mean that there is an error case we aren't able to catch with validation. Definitely it would be better to be able to catch this, but I don't feel like that needs to block this, what do you think?

[tekton-robot]

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

File	Old Coverage	New Coverage	Delta
internal/builder/v1beta1/pipeline.go	82.0%	82.2%	0.2
pkg/apis/pipeline/v1beta1/pipeline_validation.go	97.4%	97.5%	0.1
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	90.3%	90.1%	-0.2

[jerop]

@jerop i think for now it's fine to treat runAfter as the same for all circumstances - however if you wanted to tackle #3109 before this that'd be cool too. basically i dont think either needs to wait on the other

this would only be a problem for a fairly specific use case:

1. Someone has a workspace coming "from" another Task, e.g. task B needs workspace X from task A, and this is expressed using runAfter
2. That person uses a when expression on task A, and sets continueAfterSkip: true

imo (2) is actually an error - and i think if you did the same thing with a result, we'd fail it on validation, is that right? So not having #3109 before doing this would mean that there is an error case we aren't able to catch with validation. Definitely it would be better to be able to catch this, but I don't feel like that needs to block this, what do you think?

@bobcatfish thank you for clarifying the use case for workspace + runAfter described in that issue -- agreed that's something we can tackle separately (assigned myself)

[jerop]

/hold cancel

[tekton-robot]

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

File	Old Coverage	New Coverage	Delta
internal/builder/v1beta1/pipeline.go	82.0%	82.2%	0.2
pkg/apis/pipeline/v1beta1/pipeline_validation.go	97.4%	97.5%	0.1
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	90.3%	90.1%	-0.2

[pritidesai]

so if there's a task with WhenExpressions that evaluate to false and continueAfterSkip is set to true, and it has both ordering-dependent tasks and resource-dependent tasks, the ordering-dependent tasks will be executed while resource-dependent tasks will be skipped -- shown in this test case: https://github.com/tektoncd/pipeline/pull/3176/files#diff-f7c8532cfe79319c0fff711a4979a6d6R2204-R2210

Will have to be little more careful here. It sounds like inconsistent behavior for resource dependent task vs ordering dependent task. Also a task could have both runAfter and task result and would be skipped (with these changes) due to resource dependency.

The way finally task with resource dependency is being handled is that final task is at least attempted but would fail at the param validation since the task result does not exist. But when we add param level defaults for task results, that final task would be executed with the default value.

Unfortunately I dont have any ideas how to fix this inconsistency other than allowing to specify default along with task result:

spec:
 tasks:
  - name: release-env
     taskRef:
       Name: boskos-release
     params:
       - name: proj-name
         value: $(tasks.get-env.results.project)
         default: best-default-ever

The long term solution is to support schema validation for params.

[jerop]

thank you for the review @pritidesai 😊

Will have to be little more careful here. It sounds like inconsistent behavior for resource dependent task vs ordering dependent task.

does this refer to inconsistency in to how we handle the dependencies here compared to finally tasks or inconsistency in how we handle the different kinds of dependencies here by itself?

Also a task could have both runAfter and task result and would be skipped (with these changes) due to resource dependency.

continueAfterSkipped is invalid in tasks with resource dependencies so i don't think these changes would affect the skipping of such tasks

The way finally task with resource dependency is being handled is that final task is at least attempted but would fail at the param validation since the task result does not exist. But when we add param level defaults for task results, that final task would be executed with the default value.

Unfortunately I dont have any ideas how to fix this inconsistency other than allowing to specify default along with task result:

spec:
 tasks:
  - name: release-env
     taskRef:
       Name: boskos-release
     params:
       - name: proj-name
         value: $(tasks.get-env.results.project)
         default: best-default-ever

when expressions use params from the pipeline level and use task results directly (not through params in the task level) -- does this mean we'd have to support default task results in when expressions as well (in addition to params)? (an alternative could be default results in the result-producing task)

we proposed that setting continueAfterSkip on a guarded Task with resource dependencies is invalid so it fails upfront at pipeline validation

[tekton-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbwsg

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 [sbwsg]

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

[tekton-robot]

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

File	Old Coverage	New Coverage	Delta
internal/builder/v1beta1/pipeline.go	85.6%	85.8%	0.2
pkg/apis/pipeline/v1beta1/pipeline_validation.go	99.6%	99.7%	0.0
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	92.1%	92.3%	0.1

[jerop]

/retest

[tekton-robot]

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

File	Old Coverage	New Coverage	Delta
internal/builder/v1beta1/pipeline.go	85.6%	85.8%	0.2
pkg/apis/pipeline/v1beta1/pipeline_validation.go	99.6%	99.7%	0.0
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	92.1%	92.3%	0.1

[tekton-robot]

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

File	Old Coverage	New Coverage	Delta
internal/builder/v1beta1/pipeline.go	85.6%	85.8%	0.2
pkg/apis/pipeline/v1beta1/pipeline_validation.go	99.6%	99.7%	0.0
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	92.1%	92.3%	0.1

[jerop]

/hold

[michaelsauter]

What's the status on this? Is this supposed to land in the next release?

FWIW, I ran into this PR because from reading the current docs, I assumed that when expressions would not actually skip the following tasks ... I like how the documentation is worded in this PR, much clearer now.

[tekton-robot]

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

File	Old Coverage	New Coverage	Delta
internal/builder/v1beta1/pipeline.go	85.6%	85.8%	0.2
pkg/apis/pipeline/v1beta1/pipeline_validation.go	99.6%	99.7%	0.0
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	92.1%	92.3%	0.1

[tekton-robot]

@jerop: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-tekton-pipeline-integration-tests	433219a	link	/test pull-tekton-pipeline-integration-tests
pull-tekton-pipeline-build-tests	433219a	link	/test pull-tekton-pipeline-build-tests

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

[tekton-robot]

@jerop: PR needs rebase.

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.

[jerop]

new proposal accepted: https://github.com/tektoncd/community/blob/main/teps/0059-skipping-strategies.md