Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Guard Task execution via changed files #1558

Merged

Conversation

zregvart
Copy link
Member

@zregvart zregvart commented Oct 31, 2024

The new Task in .tekton/tasks/changed-dirs.yaml produces a list of changed directories for a particular pull request as an array result. Given this result guards can be done, e.g. using CEL expressions to limit when a Task on the Pipeline needs to run.
This way we can skip expensive Tasks that are unrelated to the change done in the pull request.

Similar to work in #1188 and #524, with the distinction that the PipelineRun is executed, only potentially not in full.

@zregvart
Copy link
Member Author

zregvart commented Oct 31, 2024

Marked as draft, not sure if this approach will work, need to un-draft to have the Pipeline run.

Any further ideas welcome!

@zregvart zregvart force-pushed the pr/guard-task-by-changed-directories branch from 99c8c2e to 94435f2 Compare October 31, 2024 11:07
@zregvart zregvart marked this pull request as ready for review October 31, 2024 11:10
@zregvart zregvart force-pushed the pr/guard-task-by-changed-directories branch from 94435f2 to 7b269b1 Compare October 31, 2024 11:11
@zregvart
Copy link
Member Author

We don't seem to allow CEL expressions in when:

feature flag enable-cel-in-whenexpression should be set to true to use CEL

@zregvart zregvart force-pushed the pr/guard-task-by-changed-directories branch 8 times, most recently from 3fa0fe7 to e646f15 Compare October 31, 2024 13:32
@zregvart zregvart force-pushed the pr/guard-task-by-changed-directories branch from e646f15 to 4ee07c0 Compare October 31, 2024 14:19
@chmeliik
Copy link
Contributor

It does seem too limiting without enable-cel-in-whenexpression For many of the steps, guarding them on one specific directory is not enough :(

@chmeliik
Copy link
Contributor

I was going to create a script that gets the list of changed files and evaluates some gitignore-style conditions in one step to enable skipping e2e-tests. It would avoid the result size limit problems and be more flexible than the default when (but less flexible than when with CEL)

I can put together a PR for it eventually

@zregvart
Copy link
Member Author

It does seem too limiting without enable-cel-in-whenexpression For many of the steps, guarding them on one specific directory is not enough :(

I agree, the alternative, as you pointed out is to not build a generic solution but a specific one, which would mean moving the logic into the changed-dirs Task (and in the process probably renaming it to something like task-switchboard). Tedious to maintain and not nearly as nice.

I do think the first iteration could be to look at the top level directories, that might be good enough.

There could be a middle ground, e.g. provide a set of expressions (in CEL or something else) and when those evaluate as true emit one item in the result array of changed-dirs, then use when based on that value. If that makes sense. I can try to prototype this idea on this PR.

I was going to create a script that gets the list of changed files and evaluates some gitignore-style conditions in one step to enable skipping e2e-tests. It would avoid the result size limit problems and be more flexible than the default when (but less flexible than when with CEL)

I think that kind of logic should go to the e2e harness, e.g. using Skip based on the Tasks/Pipelines changed. Might be easier to maintain this in golang, with a bit of effort this could payoff greatly.

@zregvart zregvart force-pushed the pr/guard-task-by-changed-directories branch from 4ee07c0 to 66a6ffc Compare November 11, 2024 12:12
@zregvart zregvart changed the title Guard Task execution via changed directories Guard Task execution via changed files Nov 11, 2024
@zregvart zregvart force-pushed the pr/guard-task-by-changed-directories branch 8 times, most recently from badf619 to 658bf74 Compare November 11, 2024 13:23
@zregvart zregvart force-pushed the pr/guard-task-by-changed-directories branch from 658bf74 to fba20b0 Compare November 11, 2024 13:27
@zregvart
Copy link
Member Author

This version evaluates expressions over all files in a PR using OPA/Rego. The expressions parameter determine by evaluating to true what result is omitted, the function strings.any_prefix_match comes in quite handy.

@chmeliik
Copy link
Contributor

This version evaluates expressions over all files in a PR using OPA/Rego. The expressions parameter determine by evaluating to true what result is omitted, the function strings.any_prefix_match comes in quite handy.

I like this 👍

Comment on lines 49 to 52
- tasks := strings.any_prefix_match(input, "task/")
- tasks_pipelines := strings.any_prefix_match(input, ["task/", "pipelines/"])
- e2e_tests := strings.any_prefix_match(input, ["task/", "pipelines/"])
- check_partner_tasks := strings.any_prefix_match(input, "partners/")
Copy link
Contributor

@chmeliik chmeliik Nov 11, 2024

Choose a reason for hiding this comment

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

IMO all of these should also trigger on .tekton/ and hack/ - a step should run if the step definition changes (or if the script that implements the step changes).

Copy link
Contributor

Choose a reason for hiding this comment

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

(could also get more specific with it, but .tekton/ and hack/ would be good enough for me)

Copy link
Member Author

Choose a reason for hiding this comment

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

Very true, added...

@zregvart zregvart force-pushed the pr/guard-task-by-changed-directories branch 2 times, most recently from 33a3841 to d00accf Compare November 12, 2024 10:58
The new Task in `.tekton/tasks/task-switchboard.yaml` produces a list of
bindings that evaluate to true for a particular pull request as an array
result. Given this result guards can be done, e.g. using when
expressions to limit when a Task on the Pipeline needs to run.
This way we can skip expensive Tasks that are unrelated to the change
done in the pull request.

Similar to work in konflux-ci#1188 and konflux-ci#524, with the distinction that the
PipelineRun is executed, only potentially not in full.
@zregvart zregvart force-pushed the pr/guard-task-by-changed-directories branch from d00accf to 76e4986 Compare November 14, 2024 11:05
Copy link
Contributor

@chmeliik chmeliik left a comment

Choose a reason for hiding this comment

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

LGTM 🌮

@zregvart zregvart added this pull request to the merge queue Nov 15, 2024
Merged via the queue into konflux-ci:main with commit 89fdc4c Nov 15, 2024
14 checks passed
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.

3 participants