Conditions Beta: Skipping #2995

jerop · 2020-07-23T12:02:08Z

Changes

When a Condition fails, 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 of Conditions, when a Condition evaluates to
False, users need to skip the guarded Task only and allow dependent
Tasks to execute. An example use case is when there’s a particular
Task that a Pipeline wants to execute when the git branch is
dev/staging/qa, but not when it’s the main/master/prod branch.
Another use case is when a user wants to send out a notification whether
or not a parent guarded Task was executed, as described in
this issue.

In this PR, we add a continueAfterSkip field to specify whether to
terminate the whole Task branch, or to skip the guarded Task only
and continue with the rest of the Task branch.

When a Condition evaluates to False, to skip the guarded Task
only and allow dependent Tasks to execute, users can pass in the
continueAfterSkip field and set it to true or yes (case insensitive).
The field defaults to false -- the rest of the Task branch is skipped
by default as it was previously.

When continueAfterSkip is set to True, subsequent Tasks based on
ordering dependencies should execute and the subsequent Tasks based
resource dependencies will have resource validation errors.

TEP: tektoncd/community#159

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 a Condition evaluates to False, to skip the guarded Task only and allow dependent Tasks to execute, users can pass in the continueAfterSkip field and set it to true or yes (case insensitive).
- The field defaults to false -- the rest of the Task branch is skipped by default as it was previously.

tekton-robot · 2020-07-23T12:02:14Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign dibyom
You can assign the PR to them by writing /assign @dibyom in a comment when ready.

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

Needs approval from an approver in each of these files:

OWNERS

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

tekton-robot · 2020-07-23T12:04:42Z

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/v1alpha1/pipeline.go	83.6%	82.7%	-0.9
pkg/apis/pipeline/v1alpha1/pipeline_conversion.go	96.6%	96.7%	0.1
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	92.6%	92.8%	0.2

jerop · 2020-07-23T12:18:47Z

/kind feature

tekton-robot · 2020-07-23T12:24:48Z

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/v1alpha1/pipeline.go	83.6%	82.7%	-0.9
pkg/apis/pipeline/v1alpha1/pipeline_conversion.go	96.6%	96.7%	0.1
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	92.6%	92.8%	0.2

tekton-robot · 2020-07-23T12:48:11Z

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/v1alpha1/pipeline.go	83.6%	82.7%	-0.9
pkg/apis/pipeline/v1alpha1/pipeline_conversion.go	96.6%	96.7%	0.1
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	92.6%	92.8%	0.2

When a `Condition` fails, 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 of `Conditions`, when a `Condition` evaluates to `False`, users need to skip the guarded `Task` only and allow dependent `Tasks` to execute. An example use case is when there’s a particular `Task` that a Pipeline wants to execute when the git branch is dev/staging/qa, but not when it’s the main/master/prod branch. Another use case is when a user wants to send out a notification whether or not a parent guarded `Task` was executed, as described in [this issue](tektoncd#2937). In this PR, we add a `continueAfterSkip` field to specify whether to terminate the whole `Task` branch, or to skip the guarded `Task` only and continue with the rest of the `Task` branch. When a `Condition` evaluates to `False`, to skip the guarded `Task` only and allow dependent `Tasks` to execute, users can pass in the `continueAfterSkip` field and set it to `true` or `yes` (case insensitive). The field defaults to `false` -- the rest of the `Task` branch is skipped by default as it was previously. When `continueAfterSkip` is set to `True`, subsequent `Tasks` based on ordering dependencies should execute and the subsequent `Tasks` based resource dependencies will have resource validation errors.

tekton-robot · 2020-07-28T16:26:30Z

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/v1alpha1/pipeline.go	83.6%	82.7%	-0.9
pkg/apis/pipeline/v1alpha1/pipeline_conversion.go	96.6%	96.7%	0.1
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go	92.6%	92.8%	0.2

tekton-robot · 2020-07-28T16:46:48Z

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

Test name	Commit	Details	Rerun command
pull-tekton-pipeline-build-tests	`d8feda3`	link	`/test pull-tekton-pipeline-build-tests`
pull-tekton-pipeline-integration-tests	`d8feda3`	link	`/test pull-tekton-pipeline-integration-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.

bobcatfish · 2020-07-28T17:44:44Z

/hold

I think we'll want to get tektoncd/community#159 merged as implementable before we can merge this

jerop · 2020-07-28T20:17:04Z

agreed, was implementing the proof of concepts to verify the ideas, will wait for approvals

bobcatfish

I mostly just commented on the examples - also I think we'll want to add as well:

I think there's a pipeline_defualts or similar file that we can use to set the default value of the field
imo we should treat only the string "true" or "false" as valid - OR even better, can we use an actual bool? if we are using strings, we should probably add some validation at least
probably at least one e2e test, esp. if we want to cover an error case

(speaking of validation: i still think instead of having the "resolution error (failed)" case, we can validate this at pipeline creation time and treat it as invalid - i know its not consistent with finally + results, but i think it's a better experience for the user to get more info sooner)

bobcatfish · 2020-07-30T19:46:38Z

docs/pipelines.md

+      - name: file-missing
+        taskRef: 
+          name: file-missing
+    continueAfterSkip: `true`


nit: i think this should be quotation marks

bobcatfish · 2020-07-30T19:47:08Z

examples/v1alpha1/pipelineruns/conditional-pipelinerun-continue-after-skip-ordering.yaml

+  name: conditional-pipeline
+spec:
+  tasks:
+    - name: task-should-be-skipped # failed


im not sure what "failed" means here - maybe "skipped"?

bobcatfish · 2020-07-30T19:48:51Z

examples/v1alpha1/pipelineruns/conditional-pipelinerun-continue-after-skip-ordering.yaml

+  steps:
+    - name: echo-file-exists
+      image: ubuntu
+      script: 'echo UNEXPECTED!'


maybe we could make this task exit with a non-zero exit code, so that if it wasn't skipped the pipeline would fail? not important but just wondering :D

bobcatfish · 2020-07-30T19:50:16Z

examples/v1alpha1/pipelineruns/conditional-pipelinerun-continue-after-skip-ordering.yaml

+      taskRef:
+        name: echo-expected
+      runAfter: 
+        - task-should-be-skipped


my understanding is that this pipeline run will succeed even if continueAfterSkip doesn't work properly - maybe we can add a finally task that verifies that task-should-execute executed? e.g. (until we have #2557!) we could write to a workspace in this task, and the finally task could verify that the workspace was written to?

bobcatfish · 2020-07-30T19:53:04Z

examples/v1alpha1/pipelineruns/conditional-pipelinerun-continue-after-skip-resource.yaml

+        value: "$(params.a)"
+      - name: b
+        value: "$(params.b)"
+  - name: sum-and-multiply # should execute, resolution error (failed)


usually we don't have examples that fail, cuz we want the examples to show successful runs that folks can copy

tekton-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Jul 23, 2020

tekton-robot requested review from dlorenc and imjasonh July 23, 2020 12:02

tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 23, 2020

jerop changed the title ~~Skipping when Guard evaluates as False -- ContinueAfterSkip~~ [POC] Skipping when Guard evaluates as False -- ContinueAfterSkip Jul 23, 2020

jerop force-pushed the skipping branch from f9a8633 to bfe22c5 Compare July 23, 2020 12:18

tekton-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Jul 23, 2020

jerop force-pushed the skipping branch from bfe22c5 to 475b8ab Compare July 23, 2020 12:45

tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 24, 2020

jerop force-pushed the skipping branch from 475b8ab to d8feda3 Compare July 28, 2020 16:23

jerop changed the title ~~[POC] Skipping when Guard evaluates as False -- ContinueAfterSkip~~ Skipping when Guard evaluates as False -- ContinueAfterSkip Jul 28, 2020

tekton-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 28, 2020

tekton-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 28, 2020

bobcatfish reviewed Jul 30, 2020

View reviewed changes

jerop changed the title ~~Skipping when Guard evaluates as False -- ContinueAfterSkip~~ Conditions Beta: Skipping Sep 2, 2020

jerop closed this Sep 8, 2020

jerop deleted the skipping branch September 30, 2020 15:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Conditions Beta: Skipping #2995

Conditions Beta: Skipping #2995

jerop commented Jul 23, 2020 •

edited

Loading

tekton-robot commented Jul 23, 2020

tekton-robot commented Jul 23, 2020

jerop commented Jul 23, 2020

tekton-robot commented Jul 23, 2020

tekton-robot commented Jul 23, 2020

tekton-robot commented Jul 28, 2020

tekton-robot commented Jul 28, 2020

bobcatfish commented Jul 28, 2020

jerop commented Jul 28, 2020

bobcatfish left a comment

bobcatfish Jul 30, 2020

bobcatfish Jul 30, 2020

bobcatfish Jul 30, 2020

bobcatfish Jul 30, 2020

bobcatfish Jul 30, 2020

Conditions Beta: Skipping #2995

Conditions Beta: Skipping #2995

Conversation

jerop commented Jul 23, 2020 • edited Loading

Changes

Submitter Checklist

Reviewer Notes

Release Notes

tekton-robot commented Jul 23, 2020

tekton-robot commented Jul 23, 2020

jerop commented Jul 23, 2020

tekton-robot commented Jul 23, 2020

tekton-robot commented Jul 23, 2020

tekton-robot commented Jul 28, 2020

tekton-robot commented Jul 28, 2020

bobcatfish commented Jul 28, 2020

jerop commented Jul 28, 2020

bobcatfish left a comment

Choose a reason for hiding this comment

bobcatfish Jul 30, 2020

Choose a reason for hiding this comment

bobcatfish Jul 30, 2020

Choose a reason for hiding this comment

bobcatfish Jul 30, 2020

Choose a reason for hiding this comment

bobcatfish Jul 30, 2020

Choose a reason for hiding this comment

bobcatfish Jul 30, 2020

Choose a reason for hiding this comment

jerop commented Jul 23, 2020 •

edited

Loading