fix: wire workflow_call inputs, act fallback for change detection, and correct clang-tidy/format-all failure behavior#421
Conversation
…d clang-tidy/format-all failures Co-authored-by: greenc-FNAL <2372949+greenc-FNAL@users.noreply.github.com>
c659d10
into
jules-maintenance-fix-pr-comment-reactions-18132869807280577001
There was a problem hiding this comment.
Pull request overview
This PR fixes behavioral regressions introduced by the workflow consolidation by ensuring reusable (workflow_call) workflows correctly forward ref/repo/base SHA inputs into workflow-setup, improving act behavior for change detection, and restoring “fail the job when issues are found” semantics for formatter/tidy workflows.
Changes:
- Forward
workflow_callinputs (ref,repo,pr-base-sha,checkout-path) intoworkflow-setupacross several check workflows so ref/base SHA resolution works correctly when reused. - Update
workflow-setupto skip change detection underactand provide ahas_changesoutput that allows check jobs to run locally. - Restore failure behavior in
format-allaggregation and makeclang-tidy-checkfail the job when diagnostics are detected.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| .github/workflows/python-check.yaml | Forward workflow_call inputs into workflow-setup for correct ref/repo/base SHA resolution. |
| .github/workflows/markdown-check.yaml | Same input forwarding to ensure reusable workflow resolves correct checkout/ref context. |
| .github/workflows/jsonnet-format-check.yaml | Same input forwarding for correct checkout targeting and relevance detection inputs. |
| .github/workflows/header-guards-check.yaml | Same input forwarding for reusable invocations. |
| .github/workflows/cmake-format-check.yaml | Same input forwarding for reusable invocations. |
| .github/workflows/actionlint-check.yaml | Same input forwarding for reusable invocations (with include-globs-based relevance). |
| .github/actions/workflow-setup/action.yaml | Skip change detection under act and standardize has_changes via a dedicated step. |
| .github/workflows/format-all.yaml | Fail combine-results when any formatter workflow fails/cancels/is skipped (restores prior behavior). |
| .github/workflows/clang-tidy-check.yaml | Exit non-zero when clang-tidy diagnostics are detected so the job actually fails. |
You can also share your feedback on Copilot code review. Take the survey.
| else | ||
| echo "has_changes=${{ steps.detect.outputs.has_changes }}" >> "$GITHUB_OUTPUT" |
There was a problem hiding this comment.
Set has_changes output can write an empty has_changes= when change detection is not run (e.g., file-type and include-globs are both empty so steps.detect is skipped). For consistency (and to keep the output strictly boolean-like), consider explicitly emitting has_changes=false in that case (while keeping the act override behavior).
| else | |
| echo "has_changes=${{ steps.detect.outputs.has_changes }}" >> "$GITHUB_OUTPUT" | |
| elif [ "${{ steps.detect.outputs.has_changes }}" != "" ]; then | |
| echo "has_changes=${{ steps.detect.outputs.has_changes }}" >> "$GITHUB_OUTPUT" | |
| else | |
| echo "has_changes=false" >> "$GITHUB_OUTPUT" |
| @@ -91,6 +91,7 @@ jobs: | |||
| exit 1 | |||
| elif grep -qE '^/.+\.(cpp|hpp|c|h):[0-9]+:[0-9]+: (warning|error):' clang-tidy.log; then | |||
| echo "::warning::Clang-tidy found issues in the code" | |||
There was a problem hiding this comment.
This branch emits a workflow command as ::warning::... but then exits non-zero, which fails the job. Consider switching this to ::error::... (or keeping warning but not failing) so the log annotation severity matches the job outcome.
| echo "::warning::Clang-tidy found issues in the code" | |
| echo "::error::Clang-tidy found issues in the code" |
Several behavioral regressions introduced by the workflow consolidation:
workflow_callinputs were not forwarded toworkflow-setup, causing reused workflows to resolve the wrong ref/repo/base SHA; change detection ran unconditionally inactenvironments causing unreliable results; andclang-tidy-check/format-allno longer failed their jobs when issues were found.workflow-setup—actenvironment handlingRun change detectiononis_act != 'true'Set has_changes outputstep: returnstrueunderact(so check jobs run locally), otherwise passes through detection resulthas_changesoutput to reference new stepWorkflow call input forwarding
Pass
ref,repo,pr-base-sha,checkout-pathfromworkflow_callinputs intoworkflow-setupfor:python-check.yaml,markdown-check.yaml,jsonnet-format-check.yamlcmake-format-check.yaml,header-guards-check.yaml,actionlint-check.yamlWithout this, reused workflows fell back to the caller event context instead of the explicit inputs, breaking relevance detection and checkout targeting.
format-all.yaml— restore failure behaviorAdd
Fail on formatter failuresstep (runs after comment/reaction steps) socombine-resultsexits non-zero when any sub-workflow fails/cancels/is skipped. Restores parity with the pre-consolidation inline script.clang-tidy-check.yaml— actually fail on diagnosticsAdd
exit 1when clang-tidy issues are detected. TheUpload artifactsstep already usesif: always()so artifacts are still collected on failure.💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.