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

[WIP] CI: handle required workflows when skipped #5469

Open
wants to merge 51 commits into
base: development
Choose a base branch
from

Conversation

EZoni
Copy link
Member

@EZoni EZoni commented Nov 18, 2024

Work-in-progress PR to try out various solutions to handle skipped but required CI workflows properly, e.g., when only documentation is changed (trivial one-word change in this PR).

Follow-up to #5387. Originally tried some of these in #5386.

@EZoni EZoni added the component: tests Tests and CI label Nov 18, 2024
@EZoni EZoni force-pushed the ci_docs_skip_checks branch 10 times, most recently from 4d32a75 to 6e4b089 Compare November 19, 2024 02:03
@EZoni EZoni changed the title [WIP] CI: handle required workflows when skipped CI: handle required workflows when skipped Nov 19, 2024
@EZoni EZoni changed the title CI: handle required workflows when skipped [WIP] CI: handle required workflows when skipped Nov 19, 2024
@EZoni
Copy link
Member Author

EZoni commented Nov 19, 2024

@WeiqunZhang

With regard to #5387 (comment) as well as what was done in #5387 and AMReX-Codes/amrex#4197, I tried several approaches including (i) adding dependencies between workflows through workflow_run, (ii) using reusable workflows through workflow_call, and others, but all failed at working around GitHub's handling of required checks that are skipped.

The approach implemented here, based on a custom bash script that is called from within each workflow before running the expected checks, seems to be the only one that I got to make work and that doesn't rely on third-party code (hence maybe easier to maintain, debug, etc.).

Let me know if this seems like a good enough solution to you.

Note that for the clang tidy checks, organized with a strategy matrix approach, we need to apply the if conditions at each step of the job, rather than at the job level, otherwise each workflow defined by the matrix remains pending.

@EZoni EZoni force-pushed the ci_docs_skip_checks branch 12 times, most recently from cf2be87 to 63b3fb2 Compare November 19, 2024 03:54
Comment on lines 16 to 17
- .github/workflows/**
- .azure-pipelines.yml
Copy link
Member Author

Choose a reason for hiding this comment

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

These two paths patterns are here for testing purposes only (since this PR modifies also files that match those patterns). They should be removed before merging the PR.

@EZoni EZoni changed the title [WIP] CI: handle required workflows when skipped CI: handle required workflows when skipped Nov 19, 2024
@EZoni
Copy link
Member Author

EZoni commented Nov 19, 2024

Please see the checks for 488add9 or 6636191 to verify how this works when all checks are skipped.

@EZoni EZoni changed the title CI: handle required workflows when skipped [WIP] CI: handle required workflows when skipped Nov 19, 2024
@EZoni EZoni force-pushed the ci_docs_skip_checks branch 2 times, most recently from 1517e8e to a088b0b Compare November 19, 2024 22:27
@EZoni EZoni force-pushed the ci_docs_skip_checks branch 2 times, most recently from 936a59c to f4869b6 Compare November 19, 2024 22:38
@EZoni EZoni force-pushed the ci_docs_skip_checks branch 2 times, most recently from e64d934 to 79629f5 Compare November 19, 2024 22:52
@EZoni EZoni force-pushed the ci_docs_skip_checks branch 2 times, most recently from 068c2e1 to bbb1df6 Compare November 19, 2024 23:23
@EZoni
Copy link
Member Author

EZoni commented Nov 19, 2024

Not clear how the new skip_checks job behaves when a given workflow is triggered by a push event (e.g., when merging a PR into the main branch), rather than a pull_request event (e.g., when pushing commits to an open PR, like here)...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: tests Tests and CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants