ci: address review nits on PR allowlist drift check (ARMO + Rapid7)#37
Conversation
Same fixes as kubescape/helm-charts #878 (the identical workflow landed here via #36); applying them here as a follow-up since #36 already merged: - Keep the check advisory (not a required status): a path-filtered workflow can't be required without deadlocking non-matching PRs. Doc updated. - Skip the job for label events other than allowlist-drift-ack. - On acknowledged drift, replace the warning comment with an 'acknowledged' note instead of leaving a red warning next to a green check. - Pass the drift report via base64 instead of a fixed GITHUB_OUTPUT heredoc delimiter. Signed-off-by: Ben <ben@armosec.io>
slashben
added
the
ai-assisted
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe allowlist drift workflow now documents its advisory behavior, skips unrelated label events, and transports drift reports through base64-encoded outputs. Its notification script handles acknowledged pull requests with a success comment and notice, while unacknowledged drift produces a standardized failure. Documentation covers paths filtering, hard-gate conversion, and fork pull request limitations. Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant DriftCheckJob
participant NotificationScript
participant PullRequest
DriftCheckJob->>NotificationScript: Pass encoded drift report and return code
NotificationScript->>NotificationScript: Decode drift report
NotificationScript->>PullRequest: Upsert acknowledged comment when label is present
NotificationScript->>NotificationScript: Fail workflow for unacknowledged drift
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
There was a problem hiding this comment.
Pull request overview
This PR updates the GKE Autopilot allowlist drift PR workflow and its documentation to incorporate review feedback from the upstream kubescape/helm-charts implementation, focusing on clearer guidance (advisory vs required checks), reduced unnecessary reruns on label changes, improved “acknowledged drift” UX, and safer report handling.
Changes:
- Document that the PR drift check must remain advisory while a
paths:filter is used, and describe how to convert it into a hard gate. - Skip reruns for label events unrelated to
allowlist-drift-ack, and update the sticky comment UX when drift is acknowledged. - Harden step output handling by passing the drift report via base64 rather than a fixed heredoc delimiter.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| docs/features/gke-autopilot-allowlist-tooling.md | Clarifies advisory vs required-status behavior for the path-filtered PR drift workflow and notes fork limitations. |
| .github/workflows/pr-allowlist-drift.yaml | Adjusts workflow behavior for label events, acknowledged drift messaging, and output hardening for the drift report. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| echo "rc=$rc" >> "$GITHUB_OUTPUT" | ||
| { | ||
| echo "report<<DRIFT_EOF" | ||
| echo "$OUT" | ||
| echo "DRIFT_EOF" | ||
| } >> "$GITHUB_OUTPUT" | ||
| echo "report_b64=$(printf '%s' "$OUT" | base64 -w0)" >> "$GITHUB_OUTPUT" |
Follow-up to #36 (already merged), applying the same review nits @matthyx raised on kubescape/helm-charts #878 to the identical workflow here.
paths:-filtered workflow can't be a required status check without deadlocking PRs that don't match the filter (they'd hang on "Expected — waiting for status"). Kept it advisory; the doc now says so and how to convert it to a hard gate.allowlist-drift-ack(was re-running the full check on every label change).GITHUB_OUTPUTheredoc delimiter, so an arbitrary report line can't break or inject step outputs.Mirrors kubescape/helm-charts#878.
AI-skills: armosec-shared-rules:writing-docs,schedule,loop | cmds: /model,/login,/compact