Skip to content

Conversation

@mrnugget
Copy link
Contributor

@mrnugget mrnugget commented Apr 26, 2021

This extends the schema of the batch spec steps to add a new field:

  • if: <string that can use templating to evaluate to "true" or "false">

(the corresponding schema changes sourcegraph/sourcegraph are here: https://github.com/sourcegraph/sourcegraph/pull/20399)

if: is evaluated in a best-effort partial-evaluation manner before the execution of the steps. If we can statically evaluate it, we use the result of that evaluation to build a per-repository/per-workspace list of tasks, which increases cache utilisation.

See the comments in partial_eval.go for more details.

It also extends the StepContext in the templates to have the steps.{added,modified,deleted}_files and steps.path fields.

Example batch spec

name: conditional-exec
description: Testing conditional execution of steps

on:
  - repository: github.com/sourcegraph-testing/mkcert
  - repository: github.com/sourcegraph-testing/zap
  - repository: github.com/sourcegraph/automation-testing

steps:
  #
  # Step 1.
  # No `if:`, runs in every repository/workspace
  #
  - run: echo "This is ${{ repository.name }}" >> message.txt
    container: alpine:3
  #
  # Step 2.
  # `if:` checks for repository name. Only runs in github.com/sourcegraph/automation-testing
  #
  - run: echo "hello viewers! is the background noise from the landscapers too loud? leave a message after the beep" >> message.txt
    if: "${{ eq repository.name \"github.com/sourcegraph/automation-testing\" }}"
    container: alpine:3
  #
  # Step 3.
  # `if:` checks repository name. Only runs in repos
  #
  - run: echo "repo name contains sourcegrap-testing" >> message.txt
    # The new builtin function `matches` makes it easy to check for repository names:
    if: "${{ matches repository.name \"*sourcegraph-testing*\" }}"
    container: alpine:3
  #
  # Step 4.
  # Checks for go.mod existance and saves to outputs
  #
  - run:  if [[ -f "go.mod" ]]; then echo "true"; else echo "false"; fi
    container: alpine:3
    outputs:
      goModExists:
        value: ${{ step.stdout }}
  #
  # Step 5.
  # `if:` checks whether the value just saved to outputs is true
  #
  - run: go fmt ./...
    container: golang
    # `if:` supports templating and has access to the full step context, including `previous_step.stdout`, `repository.name`, etc.
    if: ${{ outputs.goModExists }}

  #
  # Step 6.
  # `if:` checks whether the steps are executed in a workspace.
  #
  - run: echo "hello workspace" >> workspace.txt
    container: golang
    if: ${{ eq steps.path "sub/directory/in/repo" }}

changesetTemplate:
  title: Testing conditional execution of steps
  body:  Testing conditional execution of steps
  branch: thorsten/conditional-exec
  published: false
  commit:
    message: Conditional exec of steps

Demo video

Click here

Copy link

@chrispine chrispine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one question about the TODO comment. Thanks!

@mrnugget mrnugget force-pushed the mrn/conditional-exec branch from 410631c to 2bf1637 Compare April 27, 2021 08:42
@mrnugget mrnugget changed the title Prototype: conditional exec Conditional execution of steps with in: and if: attributes Apr 27, 2021
@mrnugget mrnugget force-pushed the mrn/conditional-exec branch from eb233ff to 6605471 Compare April 27, 2021 13:44
@mrnugget mrnugget force-pushed the mrn/conditional-exec branch from 9608082 to ef33f27 Compare April 28, 2021 10:50
@mrnugget mrnugget changed the title Conditional execution of steps with in: and if: attributes Conditional execution of steps with if: May 3, 2021
@mrnugget mrnugget marked this pull request as ready for review May 4, 2021 11:38
@mrnugget mrnugget requested a review from a team May 4, 2021 11:39
@mrnugget
Copy link
Contributor Author

mrnugget commented May 5, 2021

@sourcegraph/batchers this has been updated to also allow bool|string for the if: attribute.

And I extended how far we can statically evaluate the condition by implementing support for Go's not and ne template functions. That means common things like if: ${{ ne repository.name "github.com/foo" }} or if: ${{ not (matches repository.name "github.com/horst*") }} can now be used to work with the cache.

Copy link
Member

@eseliger eseliger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very neat 🌟 Thanks a lot for picking up the suggestion of making it a single field and sorting out the caching with a partial evaluator :)

Copy link
Contributor

@LawnGnome LawnGnome left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really cool!

We might want to tighten up what's accepted here slightly: playing around with if conditions, I can see that we report errors in templates pretty nicely, but there's some weirdness with non-template strings. If I define a step with, say, if: dsjfldl, then that seems to silently gets treated as true right now. Maybe we should error in that case?

@mrnugget
Copy link
Contributor Author

mrnugget commented May 6, 2021

We might want to tighten up what's accepted here slightly: playing around with if conditions, I can see that we report errors in templates pretty nicely, but there's some weirdness with non-template strings. If I define a step with, say, if: dsjfldl, then that seems to silently gets treated as true right now. Maybe we should error in that case?

It's not treated as true. If you put in if: fadsfasdf then it gets detected as "static" and the boolean value of that condition is false. But it turns out that it looks like we execute steps if len(steps) == 0. I'm going to build a check in for that, but I'm probably going to do that in another PR, so thanks for uncovering :)

@mrnugget
Copy link
Contributor Author

mrnugget commented May 6, 2021

I'm going to build a check in for that, but I'm probably going to do that in another PR, so thanks for uncovering :)

This was far easier than I assumed it would be. Pushed a commit for this.

@mrnugget mrnugget merged commit 6baa132 into main May 6, 2021
@mrnugget mrnugget deleted the mrn/conditional-exec branch May 6, 2021 09:32
scjohns pushed a commit that referenced this pull request Apr 24, 2023
This extends the schema of the batch spec `steps` to add a new field:

- `if: <string that can use templating to evaluate to "true" or "false">`

(the corresponding schema changes `sourcegraph/sourcegraph` are here: https://github.com/sourcegraph/sourcegraph/pull/20399)

`if:` is evaluated in a best-effort partial-evaluation manner before the execution of the `steps`. If we can statically evaluate it, we use the result of that evaluation to build a per-repository/per-workspace list of tasks, which increases cache utilisation.

See the comments in `partial_eval.go` for more details.

It also extends the `StepContext` in the templates to have the `steps.{added,modified,deleted}_files` and `steps.path` fields.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants