fix(ci): make docs-only PRs mergeable (gate required e2e/smoke per-job)#334
fix(ci): make docs-only PRs mergeable (gate required e2e/smoke per-job)#334Timofei Larkin (lllamnyp) wants to merge 1 commit into
Conversation
The `kamaji-datastore`, `smoke (helm)` and `smoke (manifest)` contexts are required by branch protection, but their workflows filtered themselves out at the `on:` trigger via path filters. A workflow skipped at the trigger never reports its check, so the required contexts stayed in "Expected" and blocked any PR that touched none of the filtered paths (docs-only PRs in particular). enforce_admins is on, so there was no admin bypass either. Move the path filtering off the trigger and into a cheap `changes` job (dorny/paths-filter, no checkout): the workflows now always start, and the expensive jobs are skipped via a job-level `if:` when nothing relevant changed. A job skipped via `if:` still reports its check as "skipped", which branch protection treats as passing — so docs-only PRs are mergeable while the ~30-45 min e2e and the kind smokes still skip (zero compute) on them. The smoke matrix is kept intact so the required context names are unchanged. Signed-off-by: Timofei Larkin <lllamnyp@gmail.com>
|
Note Gemini is unable to generate a review for this pull request due to the file types involved not being currently supported. |
📝 WalkthroughWalkthroughBoth e2e and release-smoke workflows are refactored to move path filtering from trigger level to job-level conditionals. A new ChangesWorkflow Conditional Gating for Docs-Only PR Unblock
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 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: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/e2e.yml:
- Line 35: The workflow uses an unpinned action reference
`dorny/paths-filter@v3`; replace that tag with a specific commit SHA for the
intended v3 release to mitigate supply-chain risk. Locate the `uses:
dorny/paths-filter@v3` entry in the e2e.yml workflow, find the corresponding v3
commit SHA from the dorny/paths-filter GitHub releases or tags, and update the
line to `uses: dorny/paths-filter@<commit-sha>` ensuring the SHA matches the
v3.x release you want to pin to.
- Around line 48-52: The dependent jobs 'kamaji-datastore' (in e2e.yml) and
'smoke' (in release-smoke.yml) currently declare needs: changes but their
existing if: condition is skipped when the 'changes' job itself is skipped;
update each job's if to guard with always(), e.g. if: ${{ always() &&
(github.event_name != 'pull_request' || needs.changes.outputs.code == 'true')
}}, so the job evaluates the condition even when 'changes' is skipped and will
run correctly for tag pushes and workflow_dispatch; keep the needs: changes
declaration and only change the if expression to include always().
- Around line 25-45: Add a minimal permissions block to the "changes" job so it
doesn't run with default repo permissions; under the job named changes (the job
that defines outputs from the dorny/paths-filter step with id filter) add e.g.
permissions: pull-requests: read (and any other specific read-only scopes you
need) to limit access to read-only PR metadata via the GitHub API.
In @.github/workflows/release-smoke.yml:
- Around line 27-51: In the "changes" job (the job block labeled changes that
runs the dorny/paths-filter step with id "filter"), replace the default
repository permissions by adding a minimal permissions block that grants only
what is needed to read PR metadata (e.g., set pull-requests: read and contents:
none) so the job has explicit read-only access and no write permissions; ensure
the permissions block is placed at the same indentation level as runs-on and
steps within the "changes" job.
- Line 35: The workflow uses an unpinned action reference
"dorny/paths-filter@v3" which must be replaced with a commit SHA to mitigate
supply-chain risks; update the "uses: dorny/paths-filter@v3" line to "uses:
dorny/paths-filter@<commit-sha>" where <commit-sha> is the full SHA of the v3
release commit (verify the SHA matches the intended v3.x tag on the
dorny/paths-filter repository before committing).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: aea80e42-6775-4d26-9aba-4a54053ee804
📒 Files selected for processing (2)
.github/workflows/e2e.yml.github/workflows/release-smoke.yml
| changes: | ||
| # Cheap (~seconds, no checkout — dorny uses the PR API) gate that decides | ||
| # whether the expensive job below needs to run. PR-only: tag pushes and | ||
| # manual dispatch always run the suite unconditionally (see the `if` on | ||
| # kamaji-datastore). | ||
| if: github.event_name == 'pull_request' | ||
| runs-on: ubuntu-latest | ||
| outputs: | ||
| code: ${{ steps.filter.outputs.code }} | ||
| steps: | ||
| - uses: dorny/paths-filter@v3 | ||
| id: filter | ||
| with: | ||
| # `code` is true when the PR touches anything the e2e suite | ||
| # exercises. Only-negated patterns get an implicit `**`, so this | ||
| # reads as "all files except Markdown and docs/**" — i.e. docs-only | ||
| # PRs leave `code` false and the ~30-45 min run below is skipped. | ||
| filters: | | ||
| code: | ||
| - '!**/*.md' | ||
| - '!docs/**' |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Restrict permissions for the changes job.
The changes job uses default repository permissions. Since it only reads PR metadata via the GitHub API (no checkout, no writes), it should declare minimal read-only permissions.
🔐 Proposed permissions block
changes:
if: github.event_name == 'pull_request'
runs-on: ubuntu-latest
+ permissions:
+ pull-requests: read
outputs:
code: ${{ steps.filter.outputs.code }}🧰 Tools
🪛 zizmor (1.25.2)
[warning] 25-45: overly broad permissions (excessive-permissions): default permissions used due to no permissions: block
(excessive-permissions)
[error] 35-35: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/e2e.yml around lines 25 - 45, Add a minimal permissions
block to the "changes" job so it doesn't run with default repo permissions;
under the job named changes (the job that defines outputs from the
dorny/paths-filter step with id filter) add e.g. permissions: pull-requests:
read (and any other specific read-only scopes you need) to limit access to
read-only PR metadata via the GitHub API.
Source: Linters/SAST tools
| outputs: | ||
| code: ${{ steps.filter.outputs.code }} | ||
| steps: | ||
| - uses: dorny/paths-filter@v3 |
There was a problem hiding this comment.
Pin dorny/paths-filter to a commit SHA.
The action reference dorny/paths-filter@v3 is unpinned. Per static analysis policy, action references should be pinned to a commit SHA to prevent supply-chain attacks.
🔒 Proposed fix
- - uses: dorny/paths-filter@v3
+ - uses: dorny/paths-filter@de90cc6fb38fc0963ad72b210f1f284cd68cea36 # v3.0.2
id: filterVerify the SHA corresponds to the intended v3.x release in the dorny/paths-filter repository.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - uses: dorny/paths-filter@v3 | |
| - uses: dorny/paths-filter@de90cc6fb38fc0963ad72b210f1f284cd68cea36 # v3.0.2 | |
| id: filter |
🧰 Tools
🪛 zizmor (1.25.2)
[error] 35-35: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/e2e.yml at line 35, The workflow uses an unpinned action
reference `dorny/paths-filter@v3`; replace that tag with a specific commit SHA
for the intended v3 release to mitigate supply-chain risk. Locate the `uses:
dorny/paths-filter@v3` entry in the e2e.yml workflow, find the corresponding v3
commit SHA from the dorny/paths-filter GitHub releases or tags, and update the
line to `uses: dorny/paths-filter@<commit-sha>` ensuring the SHA matches the
v3.x release you want to pin to.
Source: Linters/SAST tools
| needs: changes | ||
| # Always run for tag pushes / manual dispatch; for PRs, run only when | ||
| # non-docs files changed. When skipped on a docs-only PR the check still | ||
| # reports (as "skipped" = passing), so the PR is not blocked. | ||
| if: ${{ github.event_name != 'pull_request' || needs.changes.outputs.code == 'true' }} |
There was a problem hiding this comment.
Shared root cause: needs: changes breaks non-PR execution in both workflows.
Both .github/workflows/e2e.yml (kamaji-datastore job) and .github/workflows/release-smoke.yml (smoke job) declare needs: changes, but the changes job only runs when github.event_name == 'pull_request'. In GitHub Actions, when a needed job is skipped, the dependent job is skipped by default—regardless of its if: condition. This means:
- On tag push (e2e.yml trigger line 17),
kamaji-datastorewill never run - On
workflow_dispatch(release-smoke.yml trigger line 20),smokewill never run
The shared fix is to wrap both jobs' conditions in always():
e2e.yml:
kamaji-datastore:
needs: changes
- if: ${{ github.event_name != 'pull_request' || needs.changes.outputs.code == 'true' }}
+ if: ${{ always() && (github.event_name != 'pull_request' || needs.changes.outputs.code == 'true') }}release-smoke.yml:
smoke:
needs: changes
- if: ${{ github.event_name != 'pull_request' || needs.changes.outputs.release == 'true' }}
+ if: ${{ always() && (github.event_name != 'pull_request' || needs.changes.outputs.release == 'true') }}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| needs: changes | |
| # Always run for tag pushes / manual dispatch; for PRs, run only when | |
| # non-docs files changed. When skipped on a docs-only PR the check still | |
| # reports (as "skipped" = passing), so the PR is not blocked. | |
| if: ${{ github.event_name != 'pull_request' || needs.changes.outputs.code == 'true' }} | |
| needs: changes | |
| # Always run for tag pushes / manual dispatch; for PRs, run only when | |
| # non-docs files changed. When skipped on a docs-only PR the check still | |
| # reports (as "skipped" = passing), so the PR is not blocked. | |
| if: ${{ always() && (github.event_name != 'pull_request' || needs.changes.outputs.code == 'true') }} |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/e2e.yml around lines 48 - 52, The dependent jobs
'kamaji-datastore' (in e2e.yml) and 'smoke' (in release-smoke.yml) currently
declare needs: changes but their existing if: condition is skipped when the
'changes' job itself is skipped; update each job's if to guard with always(),
e.g. if: ${{ always() && (github.event_name != 'pull_request' ||
needs.changes.outputs.code == 'true') }}, so the job evaluates the condition
even when 'changes' is skipped and will run correctly for tag pushes and
workflow_dispatch; keep the needs: changes declaration and only change the if
expression to include always().
| changes: | ||
| # Cheap (~seconds, no checkout) gate. PR-only: manual dispatch always runs | ||
| # the smoke matrix unconditionally (see the `if` on smoke). | ||
| if: github.event_name == 'pull_request' | ||
| runs-on: ubuntu-latest | ||
| outputs: | ||
| release: ${{ steps.filter.outputs.release }} | ||
| steps: | ||
| - uses: dorny/paths-filter@v3 | ||
| id: filter | ||
| with: | ||
| # True when the PR touches the tag-release machinery or anything it | ||
| # ships. Matches the paths this workflow used to filter on at the | ||
| # trigger; PRs that touch none of these skip the two kind smokes. | ||
| filters: | | ||
| release: | ||
| - '.github/workflows/release-smoke.yml' | ||
| - '.github/workflows/docker-publish.yml' | ||
| - '.github/workflows/release-assets.yml' | ||
| - '.github/workflows/helm-publish.yml' | ||
| - 'hack/release-smoke.sh' | ||
| - 'charts/**' | ||
| - 'Makefile' | ||
| - 'Dockerfile' | ||
| - 'api/**' |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Restrict permissions for the changes job.
The changes job uses default repository permissions. Since it only reads PR metadata via the GitHub API (no checkout, no writes), it should declare minimal read-only permissions.
🔐 Proposed permissions block
changes:
if: github.event_name == 'pull_request'
runs-on: ubuntu-latest
+ permissions:
+ pull-requests: read
outputs:
release: ${{ steps.filter.outputs.release }}🧰 Tools
🪛 zizmor (1.25.2)
[warning] 27-51: overly broad permissions (excessive-permissions): default permissions used due to no permissions: block
(excessive-permissions)
[error] 35-35: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/release-smoke.yml around lines 27 - 51, In the "changes"
job (the job block labeled changes that runs the dorny/paths-filter step with id
"filter"), replace the default repository permissions by adding a minimal
permissions block that grants only what is needed to read PR metadata (e.g., set
pull-requests: read and contents: none) so the job has explicit read-only access
and no write permissions; ensure the permissions block is placed at the same
indentation level as runs-on and steps within the "changes" job.
Source: Linters/SAST tools
| outputs: | ||
| release: ${{ steps.filter.outputs.release }} | ||
| steps: | ||
| - uses: dorny/paths-filter@v3 |
There was a problem hiding this comment.
Pin dorny/paths-filter to a commit SHA.
The action reference dorny/paths-filter@v3 is unpinned. Per static analysis policy, action references should be pinned to a commit SHA to prevent supply-chain attacks.
🔒 Proposed fix
- - uses: dorny/paths-filter@v3
+ - uses: dorny/paths-filter@de90cc6fb38fc0963ad72b210f1f284cd68cea36 # v3.0.2
id: filterVerify the SHA corresponds to the intended v3.x release in the dorny/paths-filter repository.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - uses: dorny/paths-filter@v3 | |
| - uses: dorny/paths-filter@de90cc6fb38fc0963ad72b210f1f284cd68cea36 # v3.0.2 |
🧰 Tools
🪛 zizmor (1.25.2)
[error] 35-35: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/release-smoke.yml at line 35, The workflow uses an
unpinned action reference "dorny/paths-filter@v3" which must be replaced with a
commit SHA to mitigate supply-chain risks; update the "uses:
dorny/paths-filter@v3" line to "uses: dorny/paths-filter@<commit-sha>" where
<commit-sha> is the full SHA of the v3 release commit (verify the SHA matches
the intended v3.x tag on the dorny/paths-filter repository before committing).
Source: Linters/SAST tools
1 participant
What
Move the path filtering on the e2e and release-smoke workflows off the
on:trigger and into a per-jobif:gate, so that PRs which touch none of the filtered paths (docs-only PRs in particular) become mergeable.Closes #333.
Why
Branch protection on
mainrequires six contexts:DCO,verify,image-multiarch,kamaji-datastore,smoke (helm),smoke (manifest). The last three come from workflows that filtered themselves out at the trigger:kamaji-datastore(e2e.yml) —pull_request: paths-ignore: ['**.md', 'docs/**']smoke (helm)/smoke (manifest)(release-smoke.yml) — apull_request: paths:allowlistA workflow skipped at the trigger never creates its check context, so branch protection holds the required context in
Expectedforever and the PR staysBLOCKED.enforce_adminsis on, so there is no admin-merge bypass. That is exactly what happened to #331.How
The fix relies on the distinction GitHub draws between the two ways a check can be absent:
if:→ context reported asskipped→ branch protection treats it as passing.So each workflow now:
pull_request(no path filter);changesjob (dorny/paths-filter, no checkout — it uses the PR API, ~seconds) that detects whether relevant paths changed;if:on that output, plusgithub.event_name != 'pull_request'so tag pushes / manual dispatch still run unconditionally.On a docs-only PR the workflows start, the detector runs, and the ~30–45 min
kamaji-datastoreand the two kind smokes are skipped (zero compute) — but each required context reportsskipped→ passing. The cost-saving intent of the original filters is preserved; only the deadlock is removed.The
smokematrix (mode: [manifest, helm]) is kept as-is so the required context namessmoke (helm)/smoke (manifest)are unchanged — splitting it into two jobs would rename the contexts and require editing branch protection.Verification needed before relying on it
Confirm that the
smokematrix job, when skipped via the job-levelif:, still emits the per-leg contextssmoke (helm)andsmoke (manifest)asskippedrather than a single collapsedsmoke. It normally does, but this PR is itself a non-docs change (it edits workflows), so its own run will exercise the jobs rather than skip them — the skip path is best confirmed with a throwaway docs-only PR stacked on this branch, or by watching the next real docs-only PR. If the matrix collapses, the fallback is a sentinel job.Note on #331
#331 (the docs PR that surfaced this) was unblocked out-of-band by dispatching both workflows against its head ref via
workflow_dispatch, which produces the three contexts on the head SHA. That is a one-off; this PR removes the need to repeat it.Summary by CodeRabbit