Skip to content

[WIP] CI: Fix Skipped Required Workflows, Clean Up #5469

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 57 commits into from
Closed

[WIP] CI: Fix Skipped Required Workflows, Clean Up #5469

wants to merge 57 commits into from

Conversation

@EZoni
Copy link
Member

@EZoni EZoni commented Nov 18, 2024 •
edited
Loading

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 •
edited
Loading

@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
@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 EZoni force-pushed the ci_docs_skip_checks branch from bbb1df6 to 4dbe458 Compare November 19, 2024 23:24
@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)...

@EZoni EZoni mentioned this pull request Jan 16, 2025
Merged
@EZoni EZoni force-pushed the ci_docs_skip_checks branch 2 times, most recently from a18a56d to 3b4c6f1 Compare April 15, 2025 18:15
@EZoni EZoni force-pushed the ci_docs_skip_checks branch from 3b4c6f1 to af3b2f0 Compare April 15, 2025 18:47
@EZoni EZoni changed the title [WIP] CI: handle required workflows when skipped [WIP] CI: Fix Skipped Required Workflows, Clean Up Apr 15, 2025
EZoni
EZoni commented Apr 21, 2025
View reviewed changes
.github/workflows/scripts/check_diff.sh
# Set paths to ignore (please test grep command below when adding new patterns)
paths_ignore=()
paths_ignore+=("Docs/")
paths_ignore+=(".github/")
Copy link
Member Author

@EZoni EZoni Apr 21, 2025

Choose a reason for hiding this comment

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

This

paths_ignore+=(".github/")

was added for testing purposes and should be removed before the PR is merged, if approved.

@EZoni EZoni mentioned this pull request May 13, 2025
Merged
@EZoni
Copy link
Member Author

EZoni commented May 15, 2025

Replaced by the alternative solution implemented in #5889.

@EZoni EZoni closed this May 15, 2025
EZoni added a commit that referenced this pull request May 20, 2025
@EZoni
CI: Fix Skipped Required Workflows (#5889)
1980e99 
This implement AMReX's solution to handle skipped but required CI workflows properly, as in AMReX-Codes/amrex#4197.

This is necessary due to the limitation described in https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/collaborating-on-repositories-with-code-quality-features/troubleshooting-required-status-checks#handling-skipped-but-required-checks.

PR #5469 implements an alternative solution based on a in-house script, but I figured it may be easier to implement the same solution as in AMReX for maintenance purposes.

The new preliminary jobs are named `Analyze / Check changes`. For example, they show up as `CUDA / Analyze / Check changes`, `macOS / Analyze / Check changes`, etc.

This will allow all TC members to merge PRs where required workflows are skipped (e.g., PRs that change only documentation files), instead of relying on the WarpX admin team only.
@EZoni EZoni mentioned this pull request May 20, 2025
Merged
@EZoni EZoni mentioned this pull request May 30, 2025
Merged
3 tasks
ax3l pushed a commit that referenced this pull request Jun 20, 2025
@EZoni
CI: Fix Skipped Required Workflows (again) (#5907)
85568db 
Follow up on #5889 and fix the issue that appeared in #5883, related to
the fact that we add the `clang-tidy` checks automatically from a
matrix, rather than manually as separate jobs in the workflow file.

Note that I had managed to fix a similar issue in the alternative
in-house implementation I had proposed in #5469.

To-do:
- [x] Fix main issue by applying the `if` condition at the `step` level
(rather than the `job` level)
- [x] Remove `cron` schedule for CodeQL workflows (on Sundays at 3:27
AM...?)
- [x] Revert change in .github/workflows/check_changes.yml (implemented
only to test the PR)

Future PRs:
- Skip also CodeQL workflows (C++ and Python files analysis) - not
working yet...
@EZoni EZoni deleted the ci_docs_skip_checks branch June 21, 2025 16:06
atmyers pushed a commit to atmyers/WarpX that referenced this pull request Jul 3, 2025
@EZoni
CI: Fix Skipped Required Workflows (BLAST-WarpX#5889)
b8c8794 
This implement AMReX's solution to handle skipped but required CI workflows properly, as in AMReX-Codes/amrex#4197.

This is necessary due to the limitation described in https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/collaborating-on-repositories-with-code-quality-features/troubleshooting-required-status-checks#handling-skipped-but-required-checks.

PR BLAST-WarpX#5469 implements an alternative solution based on a in-house script, but I figured it may be easier to implement the same solution as in AMReX for maintenance purposes.

The new preliminary jobs are named `Analyze / Check changes`. For example, they show up as `CUDA / Analyze / Check changes`, `macOS / Analyze / Check changes`, etc.

This will allow all TC members to merge PRs where required workflows are skipped (e.g., PRs that change only documentation files), instead of relying on the WarpX admin team only.
atmyers pushed a commit to atmyers/WarpX that referenced this pull request Jul 3, 2025
@EZoni
CI: Fix Skipped Required Workflows (again) (BLAST-WarpX#5907)
7841f0c 
Follow up on BLAST-WarpX#5889 and fix the issue that appeared in BLAST-WarpX#5883, related to
the fact that we add the `clang-tidy` checks automatically from a
matrix, rather than manually as separate jobs in the workflow file.

Note that I had managed to fix a similar issue in the alternative
in-house implementation I had proposed in BLAST-WarpX#5469.

To-do:
- [x] Fix main issue by applying the `if` condition at the `step` level
(rather than the `job` level)
- [x] Remove `cron` schedule for CodeQL workflows (on Sundays at 3:27
AM...?)
- [x] Revert change in .github/workflows/check_changes.yml (implemented
only to test the PR)

Future PRs:
- Skip also CodeQL workflows (C++ and Python files analysis) - not
working yet...
rho-novatron pushed a commit to novatron-fusion/WarpX that referenced this pull request Oct 7, 2025
@EZoni @rho-novatron
CI: Fix Skipped Required Workflows (BLAST-WarpX#5889)
b3a44e8 
This implement AMReX's solution to handle skipped but required CI workflows properly, as in AMReX-Codes/amrex#4197.

This is necessary due to the limitation described in https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/collaborating-on-repositories-with-code-quality-features/troubleshooting-required-status-checks#handling-skipped-but-required-checks.

PR BLAST-WarpX#5469 implements an alternative solution based on a in-house script, but I figured it may be easier to implement the same solution as in AMReX for maintenance purposes.

The new preliminary jobs are named `Analyze / Check changes`. For example, they show up as `CUDA / Analyze / Check changes`, `macOS / Analyze / Check changes`, etc.

This will allow all TC members to merge PRs where required workflows are skipped (e.g., PRs that change only documentation files), instead of relying on the WarpX admin team only.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@RemiLehe RemiLehe Awaiting requested review from RemiLehe

Assignees

No one assigned

Labels

component: tests Tests and CI

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@EZoni @RemiLehe