Simple if/conditional support for skipping

[skaegi]

A common pattern used in some of the pipelines we're migrating to Tekton, is to check which branch is being built, and if the branch name matches a particular string like "dev", perform some extra logic.

I'd like to propose adding support to Pipeline with either a simple if or augmenting the conditions to allow in-lined boolean logic to decide whether to skip a Task. What's important for this use-case is that this check is not an "assertion" and just a skip guard so that the Task succeeds with a reason of "skipped". Also see #1023

For the syntax of the expression, I'd suggest we re-use JSONPath filter expressions, where the boolean logic is based on whether or not the JSONPath result list is empty. For example, given a parameter named "branch" a JSONPath filter expression for "dev" would look something like this...

$.params.branch == 'dev'

In the context of a Pipeline this might look something like...

apiVersion: tekton.dev/v1alpha1
kind: Pipeline
metadata:
  name: two-tasks-one-if
spec:
  params:
    - name: branch
  tasks:
  - name: skippable-task
    if: $.params.branch == 'dev'
    taskRef:
      name: special-dev-only-task
  - name: run-regardless-of-first-task-skipped
    taskRef:
      name: always-run-this
    runAfter:
      - skippable-task

[vdemeester]

/kind feature
/cc @dibyom

[skaegi]

Also see https://json-schema.org/draft/2019-09/json-schema-core.html#rfc.section.9.2.1 -- for some additional boolean logic operators that we could support above and beyond a single expression string

[skaegi]

I uses a JSONPath Expression in the above example but an alternative worth considering is to use CEL.

[pritidesai]

Yup, love both ❤️ more or less both serves similar purpose, can be concise and descriptive at the same time, sample here.

- cel:
            filter: "header.canonical('X-GitHub-Event') == 'push'"

This example can be addressed using conditions and runOn 😄 but condition would be heavy weight and complex just to validate params 😞 Also runOn is designed to have always equivalent to ["success", "failure", "skip"] (not implemented in the existing PR yet).

apiVersion: tekton.dev/v1alpha1
kind: Condition
metadata:
  name: compare-branch
spec:
  params:
    - name: "branch"
  check:
    image: alpine
    script: 'test $(params.branch) = dev'
---

apiVersion: tekton.dev/v1alpha1
kind: Pipeline
metadata:
  name: two-tasks-one-if
spec:
  params:
    - name: "branch"
      default: "dev"
  tasks:
    - name: skippable-task
      taskRef:
        Name: special-dev-only-task
      conditions:
        - conditionRef: "compare-branch"
          params:
            - name: "branch"
              value: "$(params.branch)"
    - name: run-regardless-of-first-task-skipped
      taskRef:
        Name: always-run-this
      runAfter:
        - skippable-task
      runOn:
        - task: skippable-task
          states: ["always"]

It would be ideal to have if implemented with either jsonpath or cel where this example would be simplified to:

apiVersion: tekton.dev/v1alpha1
kind: Pipeline
metadata:
  name: two-tasks-one-if
spec:
  params:
    - name: "branch"
      default: "dev"
  tasks:
    - name: skippable-task
      taskRef:
        Name: special-dev-only-task
      if: {.spec.params.branch} == 'dev'
    - name: run-regardless-of-first-task-skipped
      taskRef:
        Name: always-run-this
      runAfter:
        - skippable-task
      if: |
        {.spec.tasks.skippable-task.status.conditions[*].status} == true ||
       {.spec.tasks.skippable-task.status.conditions[*].status} == false
OR:
      runOn:
        - task: skippable-task
          states: ["always"]

[ssvinoth22]

It would be great if we have the above If instead of creating Conditions for each validation or logical check.

[dibyom]

Overall, I like this proposal though there are a couple of things we should discuss!

• I think it'd be nice to have some sort of simpler conditionals for the use cases you described. The current implementation is useful for access to the file system/pipeline resources though its heavy for a lot of simpler use cases.

• We can debate JSONPath vs CEL vs some other lang but I think we should definitely discuss the implications of adding/supporting an entire expression language as part of the Tekton API spec. I think @bobcatfish has some thoughts in this area!

• The skipping task behavior. The proposal is to only skip the task but not any of the following tasks. Which is different from how conditionals does it today (i.e. it skips the entire branch of dependent tasks). I think both of these behaviors have their use cases though I think we should be consistent and not do different things depending on if it is a Condition or If. Another way of defining this behavior could be:

  - name: skippable-task
    if: $.params.branch == 'dev'
    taskRef:
      name: special-dev-only-task
  - name: run-regardless-of-first-task-skipped
    taskRef:
      name: always-run-this
    runAfter:
      - skippable-task
    runOn: ["skip", "success"] # Run if skippable-task is successful or skipped but not if it failed.

• Also, we should consider the implications for runOn itself given that runOn is essentially syntactic sugar over the if expressions. As @pritidesai 's example shows, those if expressions for getting the state of a previous task can get complex and verbose, so its probably worth keeping the specialized syntax

[k]

I would like to use Conditionals also as a skip. For example I only want to build an image if the image doesn’t already exist, but I always want to deploy the image for a git sha even if it is already built. However, I still want the ability to wait for the build if I have to. The only way I can think of to do this now is to have two separate branches each with its own conditional which is rather wasteful since the conditional really only needs to check whether it needs to build or not, the deploy only should wait if the build needed to be run.

This doesn't even work properly because you can't have two tasks with the same name, even if the conditionals are setup so only one of each name will run, so because of these two completely separate conditional branches the deploys cannot depend on each other if one service's image needs to be built and the other service's image needs to be deployed.

The motivation to do something like this is because its much quicker to make github API calls to see if a particular git sha has be built and pushed to the docker registry, then while using kaniko have it pull down all the cached layers to figure out nothing changed.

[bobcatfish]

Hi all!

If possible I'd like to take a step back and try to understand what problems we are trying to solve here.

It seems like there are 2 pieces of functionality being proposed:

1. An inline syntax for conditions
2. The ability to skip a task without skipping the rest of the branch

I'd really like to understand (2) in a lot more detail - what are some use cases for this? @k mentioned one (though @k the syntax that @skaegi is proposing I think will explicitly not work for what you want - if you want to make a call to an external API as part of your condition, JSON path afaik won't allow that and even to use CEL we'd have to compile in support for that), sounds like he wants to:

1. Check if an image exists, build the image if it doesn't exist
2. Deploy the image

@k can you talk a bit more about this scenario where the image has already been built? do you only ever build the image once?

Regarding (1): Am I right in assuming that the main motivation is to avoid spinning up a pod to evaluate a conditional expression? I think we have a lot of options to improve this, some ideas:

• Add a new syntax like Simon is describing - but then there's the question of whether it should be jsonpath or CEL or something else - my strong strong preference is to avoid coupling Tekton to any particular expression language as much as possible. jsonpath I can kinda get my head around in general, though I think starting to embed support for if statements is kinda crossing the line for me: whatever expression language we choose, there will be someone who wants to use something else and some use cases (like @k's) we don't support, so I want to avoid locking us into anything for as long as possible
• Adding kinds of Condition CRDs, kinda like how we have different kinds of interceptors in triggers (including support for CEL) - we could add a few known types, and for some we could optimize by evaluating in the controller or having pooled pods to handle the evaluations or even by using something like interceptors
• other ideas?

[k]

@bobcatfish thanks for better organizing this discussion. Here's my use case in more detail:

We are using git ops within a monorepo (a repo that has code to build multiple images). In addition, we can have many different environments and building at any one time because we support creating ephemeral environments via a github label. The overall flow for this is:

1. Argo-events github trigger creates ArgoCD Application
2. Argocd Application describes environment including Tekton resources that describe how to build all the images in the environment
3. Tekton can be run manually to build and deploy images to argocd application (eventually I would like this to be an ArgoCD sync hook so all images are automatically updated on sync, but I need this optimization first because repulling layers with kaniko is too expensive and slow)

So in this scenario with a microservice Application with 10+ services rebuilding these images every time is very expensive, but if we tag every image with build with the sha of the last change to that microservice then we can use the docker registry as a way to determine whether we need to build that image or not.

The Tekton Pipeline does the following:

• files-changed Condition that uses the github API to determine the last commit for a microservice and checks to see if it exists in the docker registry
• build-and-push Task that builds and pushes the docker image with kaniko if files-changed is true
• deploy Task that patches the deployment to the newly build sha (dependency on build-and-push)

This works the first time an image is built, however, if another PR pulls in a commit from master where images were already built for some of the services, the deploy should still run even though build-and-push should not run. That's where it would be great to have optional dependencies or the ability to just "skip" the build-and-push in this case.

Having two separate "branches" of the pipeline doesn't work when there are dependencies between the multiple services that being built (for example service A should be deployed before B and C)

Update: Also happy to demo it at a Working Group meeting sometime. I really like how I can iterate on the Tekton build system in my own environment since all the resources are namespaced there

[bobcatfish]

Note from our working group today @skaegi pointed out that his main motivation is:

2. The ability to skip a task without skipping the rest of the branch

[GregDritschler]

What happens to a result reference if the referenced task was skipped through this feature? Empty string?

[tekton-robot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
If this issue is safe to close now please do so with /close.

/lifecycle stale

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.

[bobcatfish]

/remove-lifecycle rotten
/assign jerop
/reopen

This is another one that @jerop is handling via https://github.com/tektoncd/community/blob/master/teps/0007-conditions-beta.md#efficiency-1 !

[skaegi]

I do believe we can close this now that we have awesome "when expressions" support -- ty ty @jerop

[pritidesai]

Hey @skaegi I had left this open 😜 and was planning on closing after we have continueAfterSkip.

The use case in the description above wouldn't work with only when expressions the way they are implemented. skippable-task is skipped based on the when expression but run-regardless-of-first-task-skipped would be skipped too. We need continueAfterSkip to continue executing run-regardless-of-first-task-skipped.

apiVersion: tekton.dev/v1alpha1
kind: Pipeline
metadata:
  name: two-tasks-one-if
spec:
  params:
    - name: branch
  tasks:
  - name: skippable-task
    when:
      - input: "$(params.branch)"
         operator: in
         values: [ "dev" ]
    taskRef:
      name: special-dev-only-task
  - name: run-regardless-of-first-task-skipped
    taskRef:
      name: always-run-this
    runAfter:
      - skippable-task
    continueAfterSkip: true

[skaegi]

Oh no that's really too bad and you're right to re-open this. Unless I'm mistaken what you're saying is that a skipped Task is an assert "failure" that does not end the PipelineRun but prevents all dependent Tasks to run. That is definitely not what any of my users have ever asked for. In addition that introduces a logical tri-state and really complicates the meaning of run-after. continueAfterSkip might fix the problem but is novel and a bandaid to try and accommodate the tri-state.

What people have and continue to ask for is a Task with a "when" or "if" semantic that will conditionally run the Task but otherwise resolve to Success. This is something they've done in Jenkins and Travis and are trying to move to Tekton.
e.g.
https://www.jenkins.io/doc/book/pipeline/syntax/#when
https://docs.travis-ci.com/user/conditional-builds-stages-jobs

They are not generally introducing long chains of Tasks dependent on an earlier "Conditional" ever. If there is any logic deeper than what a Task provides teams encapsulate the logic are currently making remote calls to start secondary pipelines. Ideally in the future this could be handled by a Custom Task gated by a when expression.

[manuelwallrapp]

We also have this need of continue after a skipped task. What is the status of this feature request? Is this covered with the CEL merge of @jerop?

[thomascube]

@pritidesai / @jerop is there any progress to be expected regarding a continueAfterSkipor runOn feature? I can't see how CEL would solve this particular problem.

[jerop]

We also have this need of continue after a skipped step

@manuelwallrapp do you mean a skipped task?

@manuelwallrapp @thomascube we have a recently accepted design in TEP-0059: Skipping Strategies that should be released next month

/remove-lifecycle stale

[manuelwallrapp]

@jerop that https://github.com/tektoncd/community/blob/main/teps/0059-skipping-strategies.md seems to do what we need. Thank you for your fast feedback.