CI: Fix Skipped Required Workflows #5889
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
EZoni
force-pushed
the
ci_docs_skip_checks_amrex
branch
from
5cd181c to
811e958
Compare
EZoni
commented
May 13, 2025
Comment on lines
21
to
27
| filters: | | ||
| docs: | ||
| - 'Docs/**' | ||
| - '**.rst' | ||
| others: | ||
| - '!Docs/**' | ||
| - '!**.rst' |
Member
Author
There was a problem hiding this comment.
The **.rst filter is not present in the AMReX implementation, hopefully this is the right syntax to handle multiple filters. Not entirely intuitive why one needs to specify also the others filters...
Member
There was a problem hiding this comment.
I think the filter rules are combined by or by default. So has_non_docs_changes means no changes in either Docs/ or *.rst. The logic for has_non_docs_changes seems incorrect. Suppose I only change GOVERANCE.rst. Because there are no changes to Docs/, has_non_docs_changes will be set to true.
Member
There was a problem hiding this comment.
I think we should do this.
diff --git a/.github/workflows/check_changes.yml b/.github/workflows/check_changes.yml
index f526f162d..af2c2e62b 100644
--- a/.github/workflows/check_changes.yml
+++ b/.github/workflows/check_changes.yml
@@ -20,13 +20,10 @@ jobs:
id: changes
with:
filters: |
- docs:
- - 'Docs/**'
- - '**.rst'
- others:
+ non_docs:
- '!Docs/**'
- '!**.rst'
+ predicate-quantifier: 'every'
- id: set-output
run: |
- echo "has_docs_changes=${{ steps.changes.outputs.docs }}" >> $GITHUB_OUTPUT
- echo "has_non_docs_changes=${{ steps.changes.outputs.others }}" >> $GITHUB_OUTPUT
+ echo "has_non_docs_changes=${{ steps.changes.outputs.non_docs }}" >> $GITHUB_OUTPUT
Member
There was a problem hiding this comment.
We should also remove this because we don't need it.
has_docs_changes:
value: ${{ jobs.check.outputs.has_docs_changes }}
EZoni
force-pushed
the
ci_docs_skip_checks_amrex
branch
from
811e958 to
204dea7
Compare
EZoni
commented
May 13, 2025
EZoni
force-pushed
the
ci_docs_skip_checks_amrex
branch
from
2491730 to
82d1168
Compare
EZoni
force-pushed
the
ci_docs_skip_checks_amrex
branch
from
82d1168 to
3766c47
Compare
Member
|
I tested that amrex PR on my own fork. It works as expected. |
Member
Author
|
Thanks, @WeiqunZhang! I updated the PR, feel free to approve if everything looks good to you. |
WeiqunZhang
approved these changes
May 20, 2025
3 tasks
ax3l
pushed a commit
that referenced
this pull request
Jun 20, 2025
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...
atmyers
pushed a commit
to atmyers/WarpX
that referenced
this pull request
Jul 3, 2025
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
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
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
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 asCUDA / 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.