Feedback on Conditions

[jlpettersson]

Expected Behavior

That "conditions" is easy to use and easy to understand.

Actual Behavior

Things actually work, the way I expect, almost. But it is not straightforward to understand.

Proposed improvements

• In the conditional pipeline example, the word from: [first-create-file] within a resource is very hard to detect. This dictate the task-ordering here. It would be easier to read if the runAfter-syntax is used.

• Terminology, "condition" and "conditions" is probably technically correct. But in programming I am used that "conditionals" means based on this expression do this or else do that. In Tekton, the "conditions" acts as "guards" for the Task. In programming this concept is usually called "Guards", e.g. Guards in Rust, Guards in Erlang but the syntax may be e.g. when: as in previous links and in e.g. Kotlin when expression

  example:

         (b) 
       / 
   (a) 
       \
         (c) 
  

  If called "conditional", I imagine that the check is located in (a) and then do (b) or (c) based on evaluation. But if called "guard" (at least conceptually), I easier understand that it can be on e.g. (b) just before the Task executed, as how it works on Tekton today.

• We do not distinguish an evaluation error and when a condition is evaluated as condition-not-met. In programming this is fine, because it is just "expression" that is evaluated to true or false. But Tekton is dealing with a distributed system and things will fail.

  E.g. it may make sense to do a retry when an evaluation fails, but not when an evaluation of a condition evaluates to condition-not-met.

  There is still missing pieces here to correctly distinguish between an evaluation fail and condition-not-met

  Can conditional-containers write true or false as termination message and exit 0 on successful evaluation?

• Be able to negate a Condition. I have a use-case were I want to do X (e.g. deploy) when the PipelineRun was triggered on master git-branch, OR do Y (e.g. create preview-environment) if the PipelineRun was not triggered on master git-branch. Here I would like to use the same Condition (eg. conditionRef: <name> in example) on both, but negate it on one. But then it is important to fail both in case of an evaluation error occur, e.g. empty-string as branch-name (that is a configuration error).

   conditions:                # alternatively "guards:"
     - when:
         conditionRef: <name>
     - whenNot:               # or notWhen
         conditionRef: <other-name>
  

• Unclear flow. E.g. what happens after a condition? It is a bit unclear, but I have tested.

         (b) — (e)
       / 
   (a) 
       \
         (c) — (d)
  

  If "condition" is set on (c) - e.g (c) is guarded by a "condition". And when evaluated to false, the steps in (c) is not evaluated, also Task (d) is not evaluated. (as I expected). But (b) and (e) is evaluated, and the PipelineRun successfully completed.

• Add option to continue after a Task-not-evaluated.

         (b) — (e)
       / 
   (a) 
       \
         (c) — (d)
  

  If the "condition" is evaluated to false on (c), I may want to execute (d) anyway. The runAfter:-syntax is very clear and it is good to use few elements to dictate task-ordering (since it must be easyli understood by authors and readers). We could introduce some twist on runAfter: ['c'] - to execute (d) even if the condition on (c) evaluates to false. (Not if condition-evaluation fails e.g. err). E.g. runAfterUnconditionally: ['c'].

  Related to: Conditionals: Configure what happens when a task is skipped due to condition failure #1023, Feature to disable a task in a Pipeline #1797 (comment), Simple if/conditional support for skipping #2127

• Waiting for a branch that may be executed (in case (c) is guarded by a condition not met), as (f) does with runAfter:['d', 'e']

         (b) — (e)
       /          \ 
   (a)              (f)
       \          /
         (c) — (d)
  

  We should handle this, e.g. with that (f) use a runAfterOneOf:['d', 'e']

• When a task run after (using runAfter:) a task that actually fail, a Pipeline author should be able to declare that the failed task wasn't so important, it was allowedToFail. This is different than condition-not-met. The task after the failed, should be able to actually check if the previous task failed using a Condition (guard).

  But we still need a finally: pipelineTask, that also can have a Condition.

  Related to: Conditionals: Configure what happens when a task is skipped due to condition failure #1023, Design: Failure Strategy for TaskRuns in a PipelineRun #1684 (comment), "Finally" tasks for Pipeline #2446

/kind design

[tekton-robot]

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

/close

Send feedback to tektoncd/plumbing.

[tekton-robot]

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.
If this issue is safe to close now please do so with /close.

/lifecycle rotten

Send feedback to tektoncd/plumbing.

[tekton-robot]

@tekton-robot: Closing this issue.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

/close

Send feedback to tektoncd/plumbing.

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.

[bobcatfish]

/reopen
/remove-lifecycle rotten

I think a lot of the points @jlpettersson brought up are being addressed in the new conditions proposal https://github.com/tektoncd/community/blob/master/teps/0007-conditions-beta.md

@jerop if you get a chance could you update this issue re. which of the issues above are being addressed and how?

[tekton-robot]

@bobcatfish: Reopened this issue.

In response to this:

/reopen
/remove-lifecycle rotten

I think a lot of the points @jlpettersson brought up are being addressed in the new conditions proposal https://github.com/tektoncd/community/blob/master/teps/0007-conditions-beta.md

@jerop if you get a chance could you update this issue re. which of the issues above are being addressed and how?

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.