Reorganize and refactor workflows and actions#129
Reorganize and refactor workflows and actions#129greenc-FNAL merged 6 commits intoFramework-R-D:mainfrom
Conversation
greenc-FNAL
commented
Nov 21, 2025
- feat(workflows): Reorganize and standardize GitHub Actions
- refactor(workflows): align cmake-build with other workflows
This commit introduces a major reorganization of the GitHub Actions workflows to improve modularity, maintainability, and the local development experience with `act`. Key changes include: - **New Reusable Actions:** - `detect-act-env`: A composite action that detects if the workflow is running in a local `act` environment. This allows for more robust conditional execution of steps, such as skipping artifact uploads. - `get-pr-info`: A composite action that fetches Pull Request details (ref, sha, repo) for workflows triggered by issue comments. - **Standardized Workflow Structure:** - Workflows are now consistently structured with a preliminary "detect changes" job that determines if the main job should run. - The `if` conditions for jobs have been updated to use the output of the `detect-act-env` action, ensuring a more reliable local testing experience. - **Improved Logging and Error Handling:** - Workflows now include more descriptive "announce" steps and use `::group::` to make logs easier to read. - Error handling has been improved with more explicit checks and clearer failure messages. - **Refined `configure-cmake` Action:** - The `configure-cmake` action has been updated to handle CMake arguments more flexibly and robustly. This refactoring streamlines the CI/CD pipeline, reduces code duplication, and makes the workflows easier to understand and maintain.
Refactors the cmake-build workflow to align its structure and job naming with other workflows in the repository. This change incorporates a "detection" step that uses the existing detect-relevant-changes reusable action to determine if the build should run. It also adds a corresponding "skipped" job to provide clearer feedback when the build is not necessary. The original trigger conditions (workflow_dispatch and issue_comment with @phlexbot build) and author authorization checks are preserved.
|
Note: This has been verified on my fork, but 2adf3a3 causes tests to fail here because it refers safely to reusable actions that are not on this repository's |
This commit updates all external GitHub Actions to their latest versions, pinning them to a commit hash and adding a version comment.
marcpaterno
left a comment
There was a problem hiding this comment.
Here are comments from GitHub Copilot that seem work considering:
Things I found questionable, fragile or worth double-checking
Word-splitting / globbing risk when appending EXTRA_OPTIONS to CMAKE_ARGS:
The configure-cmake action does: CMAKE_ARGS+=($EXTRA_OPTIONS). If EXTRA_OPTIONS contains quoted values, spaces, or shell metacharacters, this will lead to word-splitting and globbing. Prefer to parse safely (e.g. read into an array with proper splitting rules, or pass EXTRA_OPTIONS as a single string and use eval-safe handling).
Mixed behavior for clang-tidy run: build step forced to succeed
The clang-tidy-run step was changed to wrap the cmake build & then append a trailing "true", which guarantees that step always returns success. This hides the build result from Actions and defers failure detection to later steps. That is acceptable if intentional, but it changes failure semantics and can hide pipeline status in logs (the job will still fail later if checks detect issues). Consider adding clear log lines and an explicit check step that fails with a descriptive error to make root cause easier to find.
Replacing env.ACT checks with outputs.is_act requires detect-act step be present and run before referencing it
The new pattern uses needs.detect-xxx.outputs.is_act; make sure the detect-act action is always executed in jobs that rely on its output. I saw detect-act additions in many jobs, but double-check every job that now references .outputs.is_act to ensure the detect-act step actually runs in that job (no cross-job output confusion).
Potential missing quoting in YAML expression references
Some expressions like base-ref: ${{ github.event_name == 'issue_comment' && fromJSON(steps.pr.outputs.result).base_sha || github.event.pull_request.base.sha || github.event.before }} are compact and rely on JSON parsing and conditional evaluation inside YAML. These can be fragile; test comment-triggered runs to ensure the expressions resolve correctly for all event types.
get-pr-info action uses actions/github-script and core.setOutput in a script block
The action uses actions/github-script to call github.rest.pulls.get and then core.setOutput in a script literal — ensure the version used supports this script shape. The file name is action.yml and this action is used via ./actions/get-pr-info which should work, but be cautious with version compatibility.
Using continue-on-error on lint steps but then failing in an evaluate step
This is a fine pattern (collects output then fails in a dedicated evaluation step), but if you ever remove the evaluator or its if: always() condition, the lint failure would become a soft failure. Keep the evaluator in place and make it clear in logs why failure is declared.
In several places actions/checkout and other actions are pinned to commit ids rather than tags; the commit ids differ across files. Pins are OK, but keep a plan for updating all pins consistently (some places still used old pins before this patch).
Removal of some if: always() wrappers
I noticed a few if: always() conditions were removed (e.g. Unknown Compiler Error step). Removing always() can alter whether diagnostic steps run on success vs failure; double-check intended behavior so diagnostics still run when you expect them to.
b8f644b to
3b3f739
Compare
This commit refactors the GitHub Actions workflows to improve consistency, efficiency, and clarity. The changes ensure that all workflows follow a standard structure and provide better feedback to developers. Key improvements include: - **Structural Consistency:** - A `pre-check` / `detect-changes` job pattern has been implemented across all relevant workflows to standardize the workflow structure. - Job names have been renamed for consistency (e.g., `tidy-pre-check` is now `pre-check`). - **Efficiency:** - Workflows now skip the `detect-changes` step when triggered by `workflow_dispatch` or run locally with `act`, avoiding unnecessary work. - The `if` conditions on all downstream jobs have been updated to correctly handle cases where the detection job is skipped or has no changes. - **Improved Feedback and Logging:** - The output messages in `*-skipped` jobs and other informational steps are updated to use the `::notice::` and `::error::` annotations for better visibility and consistency. - The bot command for `clang-tidy-fix` has been changed to `@phlexbot tidy-fix` to align with the `<subject>-<verb>` convention. - **CodeQL Analysis Enhancements:** - The `codeql-analysis` workflow has been updated to provide clear, actionable feedback for pull requests from forks. - If new alerts are found, the workflow now logs the full details using `::error::` and fails the run. - If only fixed alerts are found, the details are logged with `::notice::` and the run succeeds.
3b3f739 to
21fd9b7
Compare
|
Based on the AI comments @marcpaterno posted above, I have checked over the workflows with respect to the issues flagged, and I have made some changes with 21fd9b7 to further improve the structure, clarity, efficiency and consistency of the workflows. From the commit message:
|