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

.tekton/pull-request: run conditionally #1188

Closed
wants to merge 2 commits into from

Conversation

chmeliik
Copy link
Contributor

Don't run the pipeline if the only changed files are

  • *.md
  • .github/*
  • renovate.json

Note that a similar attempt was made in #524, which took the approach of enumerating every file/directory which should trigger the pipeline (maybe files.all wasn't available at the time). This PR takes the opposite approach.

@chmeliik
Copy link
Contributor Author

Tested the expression in https://playcel.undistro.io/ with variations of the following input

# Here is the input data in YAML or JSON format.

event: push
target_branch: gh-readonly-queue/main/

body:
  pull_request:
    draft: false

files:
  all:
  - "README.md"
  - "renovate.json"
  - ".github/workflows/checkton.yaml"

@@ -17,7 +17,7 @@ metadata:
) && (
// Don't run pipeline if only these files changed. Note that 'files.all' is an object
// coming from PaC (list of all changed files) while the second 'all' is a CEL macro.
!files.all.all(x, x.matches(r"^(renovate\.json|.*\.md|\.github/.*)$"))
!files.all.all(x, x.matches(r"^(renovate\.json|.*\.md|\.github/.*|\.tekton/.*)$"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we want to exclude .tekton?

I'd like to see if changes in tekton are valid and will pass testing before merging them

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I'm just testing if the exclusion works (and found out that Red Hat Konflux / build-definitions-pull-request somehow became a required check, so we'll need to disable that)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

\o/ thanks @arewm for removing the required check, now PRs wil no longer be blocked in cases where the .tekton/pull-request pipeline doesn't need to run

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Dropped the temp commit, ready for review

Also add comments.

Signed-off-by: Adam Cmiel <[email protected]>
Don't run the pipeline if the only changed files are

- *.md
- .github/*
- renovate.json

Signed-off-by: Adam Cmiel <[email protected]>
@chmeliik chmeliik force-pushed the selective-tekton-pipeline branch 2 times, most recently from a94b09a to 75cebe2 Compare July 25, 2024 15:56
@chmeliik
Copy link
Contributor Author

As @arewm found out, making the Konflux pipeline a not-required check means that if you press the Merge when ready button, github will not wait for the pipeline to finish.

There have been many requests for GitHub to make the "required check is skippable" behavior sane (https://github.com/orgs/community/discussions/13690), but until that happens, we can't go with this approach.

@chmeliik chmeliik closed this Jul 26, 2024
@chmeliik chmeliik deleted the selective-tekton-pipeline branch July 26, 2024 05:24
@zregvart
Copy link
Contributor

Perhaps this could be an enhancement in Pipelines as Code, mark skipped Pipelines as successful?

@zregvart
Copy link
Contributor

openshift-pipelines/pipelines-as-code#1746 if anyone wants to follow up with comments.

@chmeliik
Copy link
Contributor Author

openshift-pipelines/pipelines-as-code#1746 if anyone wants to follow up with comments.

Nice, thanks for filing that. For our use case, we can probably add something to the pipeline to skip just the e2e-tests task, as that's the main one we really care about.

But this does seem like it could be a useful generic PaC feature

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