Conversation
Pure bash replacement for 3rd party file change detection actions. Uses git diff with glob matching — no JS runtime overhead. Extracted from PR #15987 with minor improvements: - set -eo pipefail for proper error handling - proper variable quoting throughout 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
find-changes.shscript depends on Bash associative arrays (declare -A), which aren’t supported by the default Bash 3.x on macOS runners; consider either enforcing a Bash 4+ shell (e.g., viabashfrom Homebrew orubuntu-only usage) or adding a guard/error message so this fails clearly on unsupported environments. - The current
filtersparsing viadeclare -A filters="$1"is quite brittle and tightly couples the caller to Bash literal syntax; consider either dropping the quotes (declare -A filters=$1) and documenting that the input must be a valid associative array initializer, or switching to a more structured format (e.g., newline-delimitedkey=globpairs) to reduce parsing surprises.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `find-changes.sh` script depends on Bash associative arrays (`declare -A`), which aren’t supported by the default Bash 3.x on macOS runners; consider either enforcing a Bash 4+ shell (e.g., via `bash` from Homebrew or `ubuntu`-only usage) or adding a guard/error message so this fails clearly on unsupported environments.
- The current `filters` parsing via `declare -A filters="$1"` is quite brittle and tightly couples the caller to Bash literal syntax; consider either dropping the quotes (`declare -A filters=$1`) and documenting that the input must be a valid associative array initializer, or switching to a more structured format (e.g., newline-delimited `key=glob` pairs) to reduce parsing surprises.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a reusable Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(220,230,241,0.5)
participant PRClient as PR Client
PRClient->>GitHub: Open/Update PR
end
rect rgba(200,245,205,0.5)
participant GitHub as GitHub Actions
GitHub->>DetectWF: Trigger reusable detect-changes (workflow_call)
end
rect rgba(255,243,205,0.5)
participant DetectWF as DetectWorkflow
participant API as GitHub API
DetectWF->>API: List PR files (paginate)
API-->>DetectWF: Return file list or error/truncated
DetectWF->>DetectWF: Format filenames (anchor with ^...$), write `files` output
DetectWF-->>GitHub: Set job output `files`
end
rect rgba(241,222,255,0.5)
participant CI as CI Workflows
participant Runner as Runner
CI->>DetectWF: Wait for output (needs)
alt matching paths present
CI->>Runner: Execute gated jobs (linters, tests, integration)
else no matching paths (on PR)
CI->>GitHub: Skip gated jobs
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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: 3
🤖 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/actions/files-changed/action.yaml:
- Around line 25-28: Replace inline shell interpolation of inputs in the run
step by exporting inputs as environment variables and invoke the script with
bash; specifically, set environment variables (e.g., FILTERS, BASE, SHA) from
the GitHub Actions inputs (inputs.filters, inputs.base, inputs.sha) and call
bash "${{ github.action_path }}/find-changes.sh" using those env vars (and write
output to GITHUB_OUTPUT as before). Update the run block so it uses explicit
bash to execute find-changes.sh and references the env vars inside the script
invocation instead of embedding '${{ inputs.* }}' directly.
In @.github/actions/files-changed/find-changes.sh:
- Around line 19-30: The current IFS=$'\n' causes the echo of matches via echo
"matches=${!matches[*]}" to expand keys with newlines, which breaks
GITHUB_OUTPUT parsing; modify the write to GITHUB_OUTPUT so the matched keys are
joined on a single space (or otherwise single line) before emitting—e.g.
temporarily join "${!matches[@]}" into a single-space string or use printf to
format the list and then write that single-line value instead of the current
echo that can output multiple lines; target the matches associative array and
the echo "matches=${!matches[*]}" invocation to implement this change.
- Line 24: The current equality check if [[ $line == $exp ]]; then uses shell
globs that allow * to cross '/' boundaries; replace it with a path-segment-aware
matcher: add a function matches_path() that takes the file path ($line) and the
pattern ($exp), splits both on '/' and compares each segment pair-by-pair so '*'
only matches within a single segment (use [[ "$seg" == "$pat_seg" ]] for segment
matching), allow a special '**' pattern to mean "match remaining segments", and
ensure segment counts must match unless '**' is used; then replace the if [[
$line == $exp ]]; then with if matches_path "$line" "$exp"; then to enforce
correct path-segment glob 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: Pro Plus
Run ID: 1d3a8535-b602-4a57-8cde-8c13424156c6
📒 Files selected for processing (2)
.github/actions/files-changed/action.yaml.github/actions/files-changed/find-changes.sh
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #20171 +/- ##
=======================================
Coverage 49.74% 49.74%
=======================================
Files 2768 2768
Lines 209706 209706
=======================================
Hits 104318 104318
Misses 97690 97690
Partials 7698 7698
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:
|
🚀 Build Images ReadyImages are ready for commit 1927c31. To use with deploy scripts: export MAIN_IMAGE_TAG=4.11.x-793-g1927c318c8 |
Skip jobs on PRs when their component's files haven't changed: - go jobs: *.go, go.mod, go.sum, Makefile, make/*, operator/*, scanner/* - ui jobs: ui/* - shell jobs: scripts/*, deploy/*, *.sh - gha lint: .github/* - openshift-ci: .openshift-ci/* - version checks: COLLECTOR_VERSION, FACT_VERSION, SCANNER_VERSION On push to master/release branches, all jobs still run unconditionally. Enhanced find-changes.sh: multi-glob per filter (space-separated), extglob support, short-circuit on first match, proper IFS handling. Flagged sensor-integration-tests as misplaced (uses Kind cluster). AI-assisted. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…hanges - scanner-db-integration-tests: only run when scanner/* or go.mod/go.sum change - check-crd-compatibility: only run when operator/* or config-controller/* change Both still run unconditionally on push to master/release branches. AI-assisted. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
declare -A silently produces an empty array when the value has trailing whitespace after the closing parenthesis. The YAML > folded scalar adds a trailing newline which becomes a trailing space. Collapse and trim whitespace before declare. AI-assisted. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Avoids shell injection risk from inline ${{ inputs.* }} expansion.
Uses GITHUB_ACTION_PATH env var and bash invocation instead of
direct script execution.
AI-assisted.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace bash glob matching with git's native :(glob) pathspec matching which uses .gitignore-style semantics: * matches within a path segment, ** matches across segments. This is the standard behavior developers expect from tools like .gitignore, GitHub paths: filters, etc. Inline the script into action.yaml — with git doing the matching, there's not enough logic to justify a separate file. Update all filter patterns: *.go → **/*.go, ui/* → ui/**, etc. AI-assisted. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
JSON is native to GHA, parsed with jq (pre-installed on runners), and eliminates the declare -A brittleness (trailing whitespace bug, multi-line quoting). Also more readable at call sites. AI-assisted. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Fetch changed files via gh api pulls/{number}/files instead of
git diff. This eliminates the need for repo history — checkout
now uses sparse-checkout for just the action file.
Glob-to-regex conversion handles .gitignore semantics:
** at start → (.+/)? (zero or more segments)
** at end → .+ (everything inside)
* → [^/]* (single segment)
Removes base/sha inputs (uses PR number from github context).
AI-assisted.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The slack jobs only care if test/style jobs failed. Those already depend on detect-changes transitively — listing it directly in the slack needs was unnecessary. AI-assisted. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…onditions Instead of hardcoding all category names in the detect-changes output for push events, each job now checks: if: github.event_name == 'push' || contains(...matches, 'category') This eliminates the fragile duplicated string from detect-changes outputs and makes it clear at each job why it runs on push. AI-assisted. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Switch detect-changes from a composite action to a reusable workflow. This eliminates the checkout step entirely — GitHub reads reusable workflow files directly from the repo, and the job uses only the GitHub API (no git needed). Callers go from ~12 lines of boilerplate (outputs, checkout, step) to 3 lines (uses + with). AI-assisted. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Proto changes should trigger Go tests since they regenerate Go code. AI-assisted. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
misc-checks had needs: detect-changes but no if: guard. Since detect-changes is skipped on push events, misc-checks was also silently skipped. Remove the dependency — misc-checks should always run regardless of which files changed. AI-assisted. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
GitHub API caps at 3000 files per PR. If we hit that limit, we can't trust the file list is complete, so match all filter keys to ensure no jobs are incorrectly skipped. AI-assisted. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@coderabbitai review |
|
@sourcery-ai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The
filtersJSON is duplicated across multiple workflows (style,unit-tests,scanner-db-integration-tests, etc.); consider centralizing the filter definitions in a single reusable location (e.g., a workflow input default or a checked-in JSON file) to avoid drift and make future updates less error-prone. - Using
contains(needs.detect-changes.outputs.matches, 'go')on a space-separated string can lead to accidental substring matches if new labels are added (e.g.,go-ui); you may want to output a delimiter-wrapped list (like,go,ui,), or a JSON array, and adjust the condition checks accordingly for more robust matching.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `filters` JSON is duplicated across multiple workflows (`style`, `unit-tests`, `scanner-db-integration-tests`, etc.); consider centralizing the filter definitions in a single reusable location (e.g., a workflow input default or a checked-in JSON file) to avoid drift and make future updates less error-prone.
- Using `contains(needs.detect-changes.outputs.matches, 'go')` on a space-separated string can lead to accidental substring matches if new labels are added (e.g., `go-ui`); you may want to output a delimiter-wrapped list (like `,go,ui,`), or a JSON array, and adjust the condition checks accordingly for more robust matching.
## Individual Comments
### Comment 1
<location path=".github/workflows/detect-changes.yml" line_range="43-45" />
<code_context>
+ echo "^${regex}$"
+ }
+
+ mapfile -t files < <(gh api "repos/${GH_REPO}/pulls/${PR_NUMBER}/files" --paginate --jq '.[].filename')
+ echo "Changed files (${#files[@]}):" >&2
+ printf ' %s\n' "${files[@]}" >&2
+
+ # GitHub API caps at 3000 files per PR. If we hit that,
+ # assume everything changed so no jobs are skipped.
+ if [[ ${#files[@]} -ge 3000 ]]; then
+ echo "::warning::PR has 3000+ changed files, matching all filters" >&2
+ all_keys=$(echo "${FILTERS}" | jq -r 'keys[]' | tr '\n' ' ')
</code_context>
<issue_to_address>
**suggestion:** Failure modes when GH CLI or API calls misbehave could cause unexpected hard failures.
This step assumes `gh` exists and that the API call always succeeds. With `set -eo pipefail`, any `gh api` failure (rate limiting, network issues, missing `PR_NUMBER`, or `${{ github.token }}` permission problems) will hard-fail the workflow and block dependent jobs. Consider handling a failed `gh api` by either treating it like the `>= 3000 files` case (warn and mark all filters as matched so nothing is skipped) or failing fast with a clearer, actionable error message about the environment/permissions issue.
```suggestion
if ! gh_files_json="$(gh api "repos/${GH_REPO}/pulls/${PR_NUMBER}/files" --paginate --jq '.[].filename')"; then
echo "::warning::Failed to list changed files via GitHub API (check PR_NUMBER, gh auth, and permissions); matching all filters" >&2
all_keys=$(echo "${FILTERS}" | jq -r 'keys[]' | tr '\n' ' ')
echo "matches=${all_keys}" | tee -a "${GITHUB_OUTPUT}"
exit 0
fi
mapfile -t files < <(printf '%s\n' "${gh_files_json}")
echo "Changed files (${#files[@]}):" >&2
printf ' %s\n' "${files[@]}" >&2
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
If gh api fails (rate limiting, auth, network), match all filter keys so no jobs are skipped. Also document the label substring constraint for contains()-based matching. Addresses sourcery-ai review feedback. AI-assisted. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/unit-tests.yaml (1)
25-35: Consider including^config-controller/in thegojob filter.The
gojob runsmake -C operator/ test-integrationandtest-helm(lines 92, 95). The operator Helm/integration tests pull inconfig-controllerCRDs/content in this repo, and the companion workflowcheck-crd-compatibility.yamlgates on bothoperator/andconfig-controller/. A PR that touches onlyconfig-controller/would skip this job here while CRD-compat still runs. Low-impact, but worth aligning for consistency.contains(needs.detect-changes.outputs.files, '^proto/') || - contains(needs.detect-changes.outputs.files, '^operator/') + contains(needs.detect-changes.outputs.files, '^operator/') || + contains(needs.detect-changes.outputs.files, '^config-controller/')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/unit-tests.yaml around lines 25 - 35, The `go` job filter is missing the config-controller directory pattern so PRs touching only config-controller/ will skip the go job; update the job condition for the `go` job (the contains(...) list in the if: expression) to include the '^config-controller/' pattern so the `go` job runs when files under config-controller/ change, keeping it consistent with the `check-crd-compatibility.yaml` gating and the operator tests (which run make -C operator/ test-integration and test-helm).
🤖 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 58-62: The step currently always runs printf '^%s$\n'
"${files[@]}" which when files is empty emits a single "^$" and makes
outputs.files non-empty; fix by guarding that printf with a length check on the
files array so you only append file patterns to GITHUB_OUTPUT when files
contains elements (e.g. test ${`#files`[@]} -gt 0 before running printf '^%s$\n'
"${files[@]}" and otherwise write an empty files output), referencing the
existing variables/commands: files, printf '^%s$\n' "${files[@]}", and
GITHUB_OUTPUT (so outputs.files is truly empty when no files changed).
---
Nitpick comments:
In @.github/workflows/unit-tests.yaml:
- Around line 25-35: The `go` job filter is missing the config-controller
directory pattern so PRs touching only config-controller/ will skip the go job;
update the job condition for the `go` job (the contains(...) list in the if:
expression) to include the '^config-controller/' pattern so the `go` job runs
when files under config-controller/ change, keeping it consistent with the
`check-crd-compatibility.yaml` gating and the operator tests (which run make -C
operator/ test-integration and test-helm).
🪄 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: Pro Plus
Run ID: e696554a-5f2b-439a-9fb7-c9877308a15a
📒 Files selected for processing (5)
.github/workflows/check-crd-compatibility.yaml.github/workflows/detect-changes.yml.github/workflows/scanner-db-integration-tests.yaml.github/workflows/style.yaml.github/workflows/unit-tests.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/check-crd-compatibility.yaml
- .github/workflows/style.yaml
…N check
- Fix: empty files array emitted ^$ making output truthy; now skip
output entirely when no files changed
- Add ^config-controller/ to go job (operator tests use it)
- Simplify VERSION check to contains('VERSION$')
- Remove redundant comments on detect-changes jobs
AI-assisted.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
davdhacs
changed the title
This reverts commit 2802a79.
This reverts commit 1af1a43.
| if: >- | ||
| !needs.detect-changes.outputs.files || | ||
| contains(needs.detect-changes.outputs.files, '^operator/') || | ||
| contains(needs.detect-changes.outputs.files, '^config-controller/') |
There was a problem hiding this comment.
I went too far with adding file filters to so many jobs. But this gives good examples of what would be possible. And we could just start with a conditional for a single job like skipping just check-crd-compatibility or check-generated-files.
There was a problem hiding this comment.
How to handle required checks? If we skip the job the merge will be blocked
There was a problem hiding this comment.
Skipped jobs satisfy required checks — GitHub treats job-level skips (via if:) as "Success":
"Required status checks must have a successful, skipped, or neutral status before collaborators can make changes to a protected branch." https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/managing-protected-branches/about-protected-branches#require-status-checks-before-merging
and
"If a job within a workflow is skipped due to a conditional, it will report its status as 'Success'."
— Troubleshooting required status checks
This only works because we skip at the job level (if: on each job), not at the workflow level (on.push.paths). Workflow-level skips would leave checks as "Pending" and block merge.
Also see: About protected branches — "Required status checks must have a successful, skipped, or neutral status."
There was a problem hiding this comment.
smaller PR to add the action and apply it only to .github/ and .openshift-ci/ paths+linters: #20195
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The explanatory comment for the
'^scripts/'pattern indetect-changes.ymlis misleading — the pattern will match nested paths likescripts/nested/foo.shas well, so either update the comment or tighten the pattern if you truly only want the root-level directory. - The
ifconditions for several jobs repeat similar path/extension checks (e.g.,.go$,^go.mod$,^go.sum$); consider extracting common patterns into reusable expressions or grouping them via an additional boolean output fromdetect-changesto reduce duplication and future maintenance overhead.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The explanatory comment for the `'^scripts/'` pattern in `detect-changes.yml` is misleading — the pattern will match nested paths like `scripts/nested/foo.sh` as well, so either update the comment or tighten the pattern if you truly only want the root-level directory.
- The `if` conditions for several jobs repeat similar path/extension checks (e.g., `.go$`, `^go.mod$`, `^go.sum$`); consider extracting common patterns into reusable expressions or grouping them via an additional boolean output from `detect-changes` to reduce duplication and future maintenance overhead.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| needs: detect-changes | ||
| if: >- | ||
| !needs.detect-changes.outputs.files || | ||
| contains(needs.detect-changes.outputs.files, 'VERSION$') |
There was a problem hiding this comment.
Nitpick, because this would be an optimization and not a functional issue -
VERSION$ here matches any file ending in VERSION, including things like BUILD_IMAGE_VERSION and scanner/VULNERABILITY_VERSION, which I'm not sure if are relevant here. Maybe more explicit patterns could optimize some of this, but also feel free to resolve this and ignore if you don't think the juice is worth the squeeze.
There was a problem hiding this comment.
I want it to be matching any possible filename ending with "VERSION" so that if we add a new one, like FACT_VERSION was added this year, then we don't need to add it here. And the edge case of a *VERSION file changing which we don't check in this job triggering this is rare and acceptible (not a long/slow job I think).
So, yes (not enough juice to be worth it) I think for now it is better to match all and accept some extra runs (we run on every commit as it is right now).
These jobs run make targets, so Makefile changes should trigger them. Addresses review feedback from AlexVulaj. AI-assisted. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
I'll hold this change until I have more input. I intend to reduce it to only add the condition on one job as a guinea-pig tester/example. Then we can increase the use to this extent when/if more teams+engineers want or approve it. |
|
@davdhacs: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
replacing with #20195 |
3 participants
Description
Skip CI jobs on PRs when their files haven't changed.
A reusable workflow (
detect-changes.yml) fetches the PR's changed file list via GitHub API and outputs it with^/$anchors. Each job matches withcontains():!...filesdefaults to run^anchors root directories,$anchors extensions — no false positivesApplied to: unit-tests, style, scanner-db-integration-tests, check-crd-compatibility.
Testing and quality
How I validated my change
actionlintpasses on all modified workflowsstrict_required_status_checks_policyisfalse— skipped jobs don't block mergeTest 1 — Go file change (
pkg/version/version.go):Unit Tests |
Style
Test 2 — UI file change (
ui/package.json):Unit Tests |
Style
Both runs were cancelled by concurrency (revert commits), but job statuses (pending vs skipping) were captured before cancellation, confirming correct filter behavior.
🤖 Generated with Claude Code