Conversation
Add a reusable workflow that fetches the PR's changed file list via GitHub API and outputs it with ^/$ anchors for precise substring matching. Gate github-actions-lint, github-actions-shellcheck, and openshift-ci-lint on relevant file changes. These jobs are self-contained and low-risk for validating the approach before expanding to more jobs. AI-assisted. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Gate ui and ui-component jobs on ui/ file changes so Go-only PRs skip Cypress and Jest test runs. AI-assisted. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Skipping CI for Draft Pull Request. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The
contains(needs.detect-changes.outputs.files, '^ui/')condition treats the output as a flat string and looks for the literal^ui/; if thefilesoutput is a newline/JSON list of paths, consider either dropping the caret or using a pattern that matches actual file paths (e.g.,contains(..., 'ui/')or iterating over paths indetect-changes). - By adding
needs: detect-changesyou’ve introduced a hard dependency for theuiandui-componentjobs; ifdetect-changesfails or is skipped those jobs also skip—verify that’s acceptable, or add a fallback to still run UI tests when change detection fails.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `contains(needs.detect-changes.outputs.files, '^ui/')` condition treats the output as a flat string and looks for the literal `^ui/`; if the `files` output is a newline/JSON list of paths, consider either dropping the caret or using a pattern that matches actual file paths (e.g., `contains(..., 'ui/')` or iterating over paths in `detect-changes`).
- By adding `needs: detect-changes` you’ve introduced a hard dependency for the `ui` and `ui-component` jobs; if `detect-changes` fails or is skipped those jobs also skip—verify that’s acceptable, or add a fallback to still run UI tests when change detection fails.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
🚀 Build Images ReadyImages are ready for commit 91e951d. To use with deploy scripts: export MAIN_IMAGE_TAG=4.11.x-784-g91e951dc9d |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #20197 +/- ##
=======================================
Coverage 49.79% 49.79%
=======================================
Files 2770 2770
Lines 210212 210212
=======================================
+ Hits 104667 104672 +5
+ Misses 97823 97819 -4
+ Partials 7722 7721 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
UI jobs use make targets (make ui-test, make ui-component-tests), so Makefile changes should trigger them. AI-assisted. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis change adds a change detection system to GitHub workflows. A new Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
.github/workflows/detect-changes.yml (2)
53-56: Consider clarifying the 3000-file cap (and its relation toGITHUB_OUTPUTsize).A couple of notes on this guard:
- By the time the cap triggers,
gh api --paginatehas already fetched every page, so the cap only protectsGITHUB_OUTPUTsize / downstreamcontains()cost — not API usage. Worth a short comment to avoid future confusion.- The real hard limit is the per-job output size (~1 MiB total across all outputs). 3000 anchored paths with an average length of ~100 bytes is ~300 KiB, well under the cap, so the chosen value is safe; however, if an outlier PR has very long paths, truncation could still happen. Documenting the rationale near this check would help.
Purely a documentation/robustness nit — no behavior change required.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/detect-changes.yml around lines 53 - 56, Add an inline comment above the if [[ ${`#files`[@]} -ge 3000 ]] check explaining that this 3000-file cap is intended to limit the size of GITHUB_OUTPUT and downstream contains() cost (not API pagination, since gh api --paginate has already fetched all pages), note the ~1 MiB per-job output hard limit and that 3000 paths at ~100 bytes ≈300 KiB which is why 3000 was chosen, and mention the edge case that very long file paths could still cause truncation so callers should be aware or consider truncating/shortening entries before writing to GITHUB_OUTPUT.
39-47: Verifygh apiauth on forked PRs if repo becomes private.The
GET /repos/{owner}/{repo}/pulls/{number}/filesendpoint requirespull-requests: readpermission, which is included in the default read access for public repos. This means the API call should work on fork PRs with the read-onlyGITHUB_TOKEN. However, if this repository becomes private in the future, the fork PR token may be insufficient and the call could fail. The defensiveexit 0fallback ensures the workflow defaults to running all jobs on failure, so this is fail-safe — just be aware that fork PR optimizations depend on public repo visibility or explicit permission grant.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/detect-changes.yml around lines 39 - 47, The GH API call fetching PR files can fail for forked PRs if the repo is private because the GITHUB_TOKEN may lack pull-requests:read on forks; add a pre-check that queries the repo visibility with gh api "repos/${GH_REPO}" (use --jq '.private') before attempting the pulls files call, and if the repo is private emit a clear warning and skip the optimization (exit 0) so behavior remains safe; update the block that references PR_NUMBER and the gh api "pulls/${PR_NUMBER}/files" call to perform this visibility check first and only call pulls/files when the repo is public..github/workflows/style.yaml (1)
319-323: Optional: tighten theopenshift-ci-lintgate or justify the scope.The
^.openshift-ci/filter will not re-run this linter when the workflow itself (.github/workflows/style.yaml) or its pinned Python requirements/tooling change. In practicemake -C .openshift-ci style/lintis self-contained, so this is probably fine — but if you later change tooling assumptions, skipped PRs could miss regressions. Consider also including^.github/workflows/style.yaml$in the conditional, or add a short comment documenting why the narrow scope is sufficient.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/style.yaml around lines 319 - 323, The openshift-ci-lint job conditional currently only checks changed files under ^.openshift-ci/ which will skip rerunning the job when the workflow or its tooling change; update the condition for the openshift-ci-lint job to also check for changes to the workflow itself (e.g., include a pattern for the workflow file like ^.github/workflows/style.yaml$) or add an inline comment next to the openshift-ci-lint job explaining why the narrower scope is acceptable so future reviewers understand the rationale..github/workflows/unit-tests.yaml (1)
534-534: Stale duplicate reference inmention_author.Pre-existing nit (not introduced by this PR, but adjacent):
needs.go.outputs.new-jirasis referenced twice in the chain. Since you are already touching this workflow's gating logic, this would be a low-risk place to de-dup. Ignore if out of scope.- mention_author: ${{ needs.go.outputs.new-jiras || needs.go-postgres.outputs.new-jiras || needs.local-roxctl-tests.outputs.new-jiras || needs.ui.outputs.new-jiras || needs.go.outputs.new-jiras || needs.shell-unit-tests.outputs.new-jiras || needs.sensor-integration-tests.outputs.new-jiras }} + mention_author: ${{ needs.go.outputs.new-jiras || needs.go-postgres.outputs.new-jiras || needs.local-roxctl-tests.outputs.new-jiras || needs.ui.outputs.new-jiras || needs.shell-unit-tests.outputs.new-jiras || needs.sensor-integration-tests.outputs.new-jiras }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/unit-tests.yaml at line 534, The mention_author expression contains a duplicated token "needs.go.outputs.new-jiras"; remove the redundant occurrence so the boolean OR chain only includes each needs.<service>.outputs.new-jiras once (update the mention_author key where the expression is defined, leaving the remaining needs.* entries intact) to de-duplicate without changing semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/detect-changes.yml:
- Around line 49-51: The printf call prints a stray blank line when the files
array is empty; update the block that uses mapfile and the files array
(variables named files and the mapfile invocation) to only call printf ' %s\n'
"${files[@]}" if the array has at least one element (e.g., test "${`#files`[@]}"
-gt 0) so no format-only printf runs when output is empty.
In @.github/workflows/unit-tests.yaml:
- Around line 230-235: The gate currently only triggers the "ui" (and similarly
"ui-component") job if changes match '^ui/' or '^Makefile$', which misses edits
to workflow and custom action files; update the if condition on the "ui" and
"ui-component" jobs to also include '^.github/workflows/unit-tests.yaml$',
'^.github/actions/cache-ui-dependencies/', and '^.github/actions/job-preamble/'
(or the equivalent glob patterns for those directories) so that modifications to
the workflow file or the custom actions referenced by the jobs will rerun the UI
tests; adjust the contains(...) checks in the "ui" and "ui-component" job
definitions to include these additional paths.
---
Nitpick comments:
In @.github/workflows/detect-changes.yml:
- Around line 53-56: Add an inline comment above the if [[ ${`#files`[@]} -ge 3000
]] check explaining that this 3000-file cap is intended to limit the size of
GITHUB_OUTPUT and downstream contains() cost (not API pagination, since gh api
--paginate has already fetched all pages), note the ~1 MiB per-job output hard
limit and that 3000 paths at ~100 bytes ≈300 KiB which is why 3000 was chosen,
and mention the edge case that very long file paths could still cause truncation
so callers should be aware or consider truncating/shortening entries before
writing to GITHUB_OUTPUT.
- Around line 39-47: The GH API call fetching PR files can fail for forked PRs
if the repo is private because the GITHUB_TOKEN may lack pull-requests:read on
forks; add a pre-check that queries the repo visibility with gh api
"repos/${GH_REPO}" (use --jq '.private') before attempting the pulls files call,
and if the repo is private emit a clear warning and skip the optimization (exit
0) so behavior remains safe; update the block that references PR_NUMBER and the
gh api "pulls/${PR_NUMBER}/files" call to perform this visibility check first
and only call pulls/files when the repo is public.
In @.github/workflows/style.yaml:
- Around line 319-323: The openshift-ci-lint job conditional currently only
checks changed files under ^.openshift-ci/ which will skip rerunning the job
when the workflow or its tooling change; update the condition for the
openshift-ci-lint job to also check for changes to the workflow itself (e.g.,
include a pattern for the workflow file like ^.github/workflows/style.yaml$) or
add an inline comment next to the openshift-ci-lint job explaining why the
narrower scope is acceptable so future reviewers understand the rationale.
In @.github/workflows/unit-tests.yaml:
- Line 534: The mention_author expression contains a duplicated token
"needs.go.outputs.new-jiras"; remove the redundant occurrence so the boolean OR
chain only includes each needs.<service>.outputs.new-jiras once (update the
mention_author key where the expression is defined, leaving the remaining
needs.* entries intact) to de-duplicate without changing semantics.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 507e76c0-cfc4-4ea0-bba9-e63710aff6d8
📒 Files selected for processing (3)
.github/workflows/detect-changes.yml.github/workflows/style.yaml.github/workflows/unit-tests.yaml
davdhacs
changed the title
davdhacs
changed the title
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The
contains(needs.detect-changes.outputs.files, '^ui/')and'^Makefile$'checks won’t be treated as regex by GitHub Actions, so the^and$characters are just literals; consider either removing them or switching to a pattern/format that is actually supported (e.g., checking forui/andMakefilesubstrings based on howdetect-changesformatsfiles). - The
ifconditions foruiandui-componentare duplicated; consider extracting the shared expression into a single reusable piece (e.g., a job output fromdetect-changesor a common expression) to avoid drift if this gating logic needs to be updated later.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `contains(needs.detect-changes.outputs.files, '^ui/')` and `'^Makefile$'` checks won’t be treated as regex by GitHub Actions, so the `^` and `$` characters are just literals; consider either removing them or switching to a pattern/format that is actually supported (e.g., checking for `ui/` and `Makefile` substrings based on how `detect-changes` formats `files`).
- The `if` conditions for `ui` and `ui-component` are duplicated; consider extracting the shared expression into a single reusable piece (e.g., a job output from `detect-changes` or a common expression) to avoid drift if this gating logic needs to be updated later.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
UI jobs use shared actions (cache-ui-dependencies, job-preamble) and are defined in the unit-tests workflow file. Changes to any .github/ file should re-run them to validate. AI-assisted. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
davdhacs
force-pushed
the
davdhacs/skip-ui-for-go
branch
from
88a84f0 to
5a0f255
Compare
Shared dependencies (Makefile, make/, workflow file, job-preamble) are declared once in run_all. If any match, all jobs run. Job conditions now only list component-specific patterns: ui: '^ui/', '^.github/actions/cache-ui-dependencies/' AI-assisted. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@coderabbitai review |
🧩 Analysis chain🏁 Script executed: Length of output: 35160
Review of
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/detect-changes.yml (1)
71-80: Optional: add a one-liner explaining whygrep -Fis used.The fixed-string match is intentional because
run_allpatterns embed literal^and$anchors (same format as the emittedfilesoutput), but this isn't obvious at a glance. A brief inline comment will save a future reader from wondering whether regex metacharacters were overlooked.📝 Proposed clarifying comment
if [[ -n "${RUN_ALL}" ]]; then + # -F: patterns use literal ^/$ anchors (same format as `files` + # output), so treat them as fixed strings, not regex. anchored=$(printf '^%s$\n' "${files[@]}") while IFS= read -r pattern; do [[ -z "$pattern" ]] && continue if echo "${anchored}" | grep -qF "$pattern"; then echo "run_all: matched '${pattern}', running all jobs" >&2 exit 0 fi done <<< "${RUN_ALL}" fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/detect-changes.yml around lines 71 - 80, Add a short inline comment explaining that grep is invoked with -F (fixed-string) because RUN_ALL contains literal anchor characters like '^' and '$' that must be matched literally against the constructed anchored variable; reference the variables/commands in the block (anchored, RUN_ALL, pattern, and the grep -qF call) and place the one-line comment immediately above the if echo "${anchored}" | grep -qF "$pattern" line so future readers know the literal-match choice is intentional.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/detect-changes.yml:
- Around line 71-80: Add a short inline comment explaining that grep is invoked
with -F (fixed-string) because RUN_ALL contains literal anchor characters like
'^' and '$' that must be matched literally against the constructed anchored
variable; reference the variables/commands in the block (anchored, RUN_ALL,
pattern, and the grep -qF call) and place the one-line comment immediately above
the if echo "${anchored}" | grep -qF "$pattern" line so future readers know the
literal-match choice is intentional.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: e1dc3c15-0e1a-4210-b330-6ba4f4b2d689
📒 Files selected for processing (2)
.github/workflows/detect-changes.yml.github/workflows/unit-tests.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/unit-tests.yaml
| inputs: | ||
| run_all: | ||
| description: > | ||
| Newline-separated list of patterns (same ^/$ anchored |
There was a problem hiding this comment.
| Newline-separated list of patterns (same ^/$ anchored | |
| Newline-separated regex-like patterns (same ^/$ anchored |
2 participants
Description
Gate
uiandui-componentjobs onui/file changes so Go-only PRs skip Cypress and Jest test runs (~5-6m of compute saved per Go-only PR).Testing and quality
How I validated my change
actionlintpasses🤖 Generated with Claude Code