Skip to content

Conversation

@greenc-FNAL
Copy link
Contributor

@greenc-FNAL greenc-FNAL commented Oct 10, 2025

  • New action detect-relevant-changes
  • Restrict extensions to those we use/expect to use in Phlex
  • Security and robustness improvements to workflows and actions
  • Integrate use of detect-relevant-changes into existing workflows.
  • Remove unnecessary invocation of default target from clang-tidy-check

@greenc-FNAL greenc-FNAL reopened this Oct 10, 2025
@greenc-FNAL greenc-FNAL force-pushed the maintenance/workflow-relevance-checks branch from 502521d to e0f518e Compare October 10, 2025 22:54
@greenc-FNAL greenc-FNAL marked this pull request as ready for review October 10, 2025 22:54
@greenc-FNAL greenc-FNAL requested a review from knoepfel October 13, 2025 14:19
@knoepfel
Copy link
Member

@greenc-FNAL, can you explain the need for this PR? It's adding quite a bit a code, and I'd like to understand its benefits before adopting ~500 lines of code that we need to maintain.

@knoepfel
Copy link
Member

I'm particularly curious how much faster using rg instead of ack speeds up the checks. The number of files in the repository is not large (and not expected to become large), so I wouldn't expect a large difference.

@greenc-FNAL
Copy link
Contributor Author

It was more a case of, "If I have to make a container anyway, may as well use the better thing."

@greenc-FNAL
Copy link
Contributor Author

... also the thing that doesn't depend on PERL.

@knoepfel
Copy link
Member

It was more a case of, "If I have to make a container anyway, may as well use the better thing."

So now there are two images that we need to maintain? And one that only provides ripgrep on top of the default image the GitHub action runs?

As I mentioned earlier, there are not many files that we need to deal with, and the ones that we do have follow a convention: .cpp, .hpp, CMakeLists.txt, etc. (not .cc, .cxx, .hxx, .hh, .h, etc.). I'd prefer to keep our CI/CD infrastructure as minimal as possible until we know that we need something more. My feeling is what we need now is not greatly benefited by adding an extra image (as small as it may be). I think our code structure is simple enough that find should be sufficient.

greenc-FNAL added a commit that referenced this pull request Oct 14, 2025
@greenc-FNAL
Copy link
Contributor Author

The Dockerfile was only half a dozen non-comment lines, but I take your point. Originally, AI said that the basic runner had rg installed by default: it was wrong.

- Apply to `cmake-format-check`.

Use ripgrep (`rg`) instead of `ack`

Fix arguments to rg

Remove unnecessary default target build

Reduce per-workflow load and correct use of `rg`

- Reorganize Dockerfile collection for plurality
- Add a minimal container definition for `rg`
- Require ripgrep, fail if not found
- Use `rg --sort path` and `comm` to compare file lists

Handle .in files

Fix comment

Update more workflows to be more efficient and handle generated files

Update permissions
- Initial checks use cached version of workflows, which don't send the
required arguments to the action, apparently. Once this PR is merged,
this commit can be reverted.
Actions and workflows:

- Are now protected against input-based injection attacks or unsafe
characters (such as apostrophes in comments).
  - Use environment variables instead of direct injection (`${{ ... }}`)
  for `inputs\..*`.
  - Use quoting of variables when appropriate.
  - Use `printf` with parameter substitution rather then simply `echo`.
- Use `grep -E` with word boundary checking instead of `contains()` to
avoid false positives.
@greenc-FNAL greenc-FNAL force-pushed the maintenance/workflow-relevance-checks branch from 5e203fa to 24a8460 Compare October 15, 2025 13:52
@greenc-FNAL
Copy link
Contributor Author

To address your other point:

  • 9e2f390 is a necessary fix for comment injection. If you need I can pull it out into a separate PR or commit it directly to main, but either way it should go in ASAP.
  • I have removed almost 50 lines of redundant (and, in a couple of cases, wrong) code.
  • The main value-added of the remainder is to save CI time and resources, and PR development time: a coverage check takes about 8 minutes now: why trigger it if you're only updating utility scripts or documentation?
  • It's also extensible (relatively) trivially, for e.g. Python, YAML, or JSON checks.

@knoepfel
Copy link
Member

Thanks, @greenc-FNAL, for the explanation. I agree with the goal of this PR. I will add some review comments soon.

@greenc-FNAL greenc-FNAL requested a review from Copilot October 15, 2025 15:03
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR optimizes CI workflows by introducing a change detection mechanism that skips workflows when no relevant files have changed, reducing unnecessary container loads and build operations. The changes introduce a new detect-relevant-changes action that uses file type filtering to determine if workflows should run.

Key changes:

  • Created a reusable detect-relevant-changes action that filters changed files by type (cpp, cmake)
  • Added detection jobs to multiple workflows (coverage, clang-tidy-check, clang-format-check, cmake-format-check) with corresponding skip jobs
  • Improved shell script safety by quoting variables and using environment variables instead of inline GitHub Actions expressions
  • Extended file type coverage to include .in template files for both C++ and CMake formatting workflows

Reviewed Changes

Copilot reviewed 14 out of 17 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
ci/entrypoint.sh Converted to symbolic link pointing to docker/phlex-ci-default/entrypoint.sh
ci/docker/phlex-ci-default/entrypoint.sh New location for entrypoint script content (relocated from ci/entrypoint.sh)
.github/workflows/coverage.yaml Added change detection job to skip coverage workflow when no C++/CMake changes exist
.github/workflows/cmake-format-fix.yaml Improved shell safety with quoted variables and added .in file support
.github/workflows/cmake-format-check.yaml Added change detection job and extended file pattern matching to include .in files
.github/workflows/cmake-build.yaml Enhanced trigger condition checking with proper quoting and environment variables
.github/workflows/clang-tidy-fix.yaml Improved shell script safety by using environment variables
.github/workflows/clang-tidy-check.yaml Added change detection, removed unnecessary default build step, added container credentials
.github/workflows/clang-format-fix.yaml Added .in file extensions and improved shell safety
.github/workflows/clang-format-check.yaml Added change detection job and extended file type support to .in files
.github/actions/setup-build-env/action.yaml Improved shell safety with quoted variables and printf for path construction
.github/actions/detect-relevant-changes/action.yaml New action implementing file change detection with type-based filtering
.github/actions/configure-cmake/action.yaml Improved shell safety by using environment variables instead of inline expressions
.github/actions/build-cmake/action.yaml Improved shell safety with environment variables

Copy link
Member

@knoepfel knoepfel left a comment

Choose a reason for hiding this comment

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

@greenc-FNAL, thanks for the PR. Now that I understand its purpose, I generally agree with these changes. I do have some questions/suggestions below for your consideration.

@greenc-FNAL greenc-FNAL merged commit e33a01a into main Oct 15, 2025
12 checks passed
@greenc-FNAL greenc-FNAL deleted the maintenance/workflow-relevance-checks branch October 16, 2025 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants