feat(chart): Redis-optional install by default; rename modes to configured-author/attributed-author#211
Merged
Conversation
…gured-author/attributed-author Rename the two operating modes to name the field that actually varies (the Git author): committer-only -> configured-author, attributed -> attributed-author. Applied across docs, chart, and Go comments/logs/tests. API/flag terms (AuthorAttributed condition, attribution.enabled, committer role) are unchanged. Make Redis optional but advised, so a bare `helm install` comes up healthy with no external infrastructure: - queue.redis.addr and queue.redis.auth.existingSecret default to "". Without Redis the operator runs configured-author and watches cold-replay on restart. - The admission webhook (servers.admission.enabled) stays ON by default, but no longer requires Redis: without it, command-author capture is a graceful no-op and CommitRequests commit as the committer (AuthorAttributed=False). The startup guard is relaxed, the command-author store is only built when Redis is present, and the handler no-ops on a nil store. - attribution.enabled=true still requires Redis; the chart fails that combination at render time (validate-redis.yaml) instead of crash-looping the pod. - quickstart.namespace defaults to gitops-reverser-quickstart-demo; new quickstart.createNamespace (default false) so the namespace and git-creds Secret can be created before install (the GitProvider only re-checks every ~5m). - dist/install.yaml keeps the full Redis-backed config; regenerated byte-identical. Docs: shorter, punchier README with author-vs-committer tables and a single-command demo flow (no em dashes); new docs/attribution-setup-guide.md; UPGRADING entry for the default changes; HA goal added to docs/TODO.md. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR renames author-attribution modes to configured-author and attributed-author, makes Redis optional for the configured-author path, adds nil-store guards in webhook handling, updates Helm quickstart behavior, and refreshes docs and tests to match the new mode semantics. ChangesConfigured-author mode and optional Redis support
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Main as cmd/main.go
participant ValidateOperatorTypesHandler
participant CommandAuthorStore
Client->>Main: start with configured-author settings
Main->>Main: validate Redis / attribution flags
Main->>ValidateOperatorTypesHandler: register webhook handler
ValidateOperatorTypesHandler->>ValidateOperatorTypesHandler: check Store nil?
alt Redis configured
ValidateOperatorTypesHandler->>CommandAuthorStore: record command author
else Redis absent
ValidateOperatorTypesHandler-->>ValidateOperatorTypesHandler: no-op
end
ValidateOperatorTypesHandler-->>Client: allow admission
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
cmd/main.go (2)
413-417: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winStale flag help text: admission webhook no longer requires Redis.
The
--redis-addrhelp text says leaving it empty is "Incompatible with--author-attribution=true or --admission-webhook." However, this PR explicitly makes the admission webhook work without Redis (lines 272-281, 562-564). Only--author-attribution=trueis now incompatible with an empty--redis-addr.📝 Proposed fix
fs.StringVar(&cfg.redisAddr, "redis-addr", "valkey:6379", "Redis/Valkey address (host:port). Holds each GitTarget's watch resume cursors (state continuity) "+ "and, when author attribution is enabled, the attribution facts. Leave empty to run without "+ - "Redis: watches cold-replay on restart instead of resuming. Incompatible with "+ - "--author-attribution=true or --admission-webhook.") + "Redis: watches cold-replay on restart instead of resuming. Incompatible with "+ + "--author-attribution=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 `@cmd/main.go` around lines 413 - 417, The `--redis-addr` flag help text is stale: `fs.StringVar` in `main` still says empty Redis is incompatible with `--admission-webhook`, but the admission webhook no longer requires Redis. Update the help string so it only states incompatibility with `--author-attribution=true`, and keep the rest of the resume-cursor/attribution description consistent with the current behavior.
176-180: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStale comment: admission webhook no longer requires Redis.
The comment says "Author attribution and the admission webhook both require Redis; validation has already rejected those combinations when redis-addr is empty." This PR removes that validation for the admission webhook (lines 562-564), so the comment is now incorrect.
📝 Proposed fix
// Valkey/Redis is optional. When configured it holds each GitTarget's watch resume cursor so work // is re-picked up exactly where it left off after a restart or reconnect. When not configured the // WatchCursorStore stays nil and watches cold-replay from scratch on restart instead of resuming. -// Author attribution and the admission webhook both require Redis; validation has already rejected -// those combinations when redis-addr is empty. +// Author attribution requires Redis; validation has already rejected that combination when +// redis-addr is empty. The admission webhook stays enabled without Redis and no-ops command-author +// capture (commits fall back to the committer).🤖 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 `@cmd/main.go` around lines 176 - 180, Update the Redis configuration comment in main to match the current validation logic: the admission webhook no longer requires Redis, so remove that claim from the explanatory comment near the WatchCursorStore setup. Keep the note consistent with the remaining Redis-dependent behavior by referring only to the author attribution path and the nil WatchCursorStore fallback in the relevant main initialization section.docs/design/watch-first-ingestion-architecture.md (1)
32-40: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRewrite the Redis dependency claim.
This paragraph still says Redis/Valkey is required in every mode, but the shipped contract makes
configured-authorrun withqueue.redis.addr="". Please update the summary so it reflects the Redis-free default instead of the old always-on dependency.🤖 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 `@docs/design/watch-first-ingestion-architecture.md` around lines 32 - 40, Rewrite the Redis dependency summary in the watch-first ingestion architecture doc so it no longer claims Redis/Valkey is required in every mode. Update the paragraph around the configured-author and attribution explanation to reflect that the shipped default can run with queue.redis.addr="" and therefore Redis is optional in the default Redis-free mode, while keeping attribution as the separate opt-in layer.docs/facts/subresources.md (1)
434-441: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCorrect the
Attributedexample.In configured-author mode the controller records
AuthorAttributed=False/CommitterFallback, notTrue/AttributedFromAuditEvent. The sample status should match the real condition semantics.🤖 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 `@docs/facts/subresources.md` around lines 434 - 441, The Attributed example is using the wrong status and reason for configured-author mode. Update the sample in the subresources documentation so the condition emitted by the controller matches the real semantics in that mode: use the same Attributed condition shape shown in the example, but set the status/reason to reflect AuthorAttributed=False with CommitterFallback instead of True with AttributedFromAuditEvent.docs/design/watch-first-merge-readiness.md (1)
119-124: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAlign the readiness note with the Redis-free default.
The new text says empty
--redis-addris an error and that configured-author still runs with Redis on, butvalidateAuditConfigonly rejects empty Redis when attribution is enabled. Please revise this section so the readiness doc matches the actual default install contract.🤖 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 `@docs/design/watch-first-merge-readiness.md` around lines 119 - 124, Update the readiness note to match the actual default install behavior in validateAuditConfig and cmd/main.go. The current wording incorrectly says empty --redis-addr is always an error and that configured-author still requires Redis; revise it so Redis is only required when --author-attribution is enabled, while the Redis-free default remains valid. Keep the description aligned with the watch-first merge contract and the attribution toggle wording used by validateAuditConfig, WatchCursorStore, and the configured-author example.
🤖 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 `@charts/gitops-reverser/README.md`:
- Around line 178-192: The README table has conflicting Redis/admission guidance
between queue.redis.addr and servers.admission.enabled, so make the contract
consistent in one place. Update the descriptions for queue.redis.addr and
servers.admission.enabled so they agree on when Redis is required and when the
admission webhook is a no-op, using those exact symbols to keep the
cross-reference clear and removing the duplicate/contradictory wording.
In `@docs/architecture.md`:
- Around line 39-42: The Redis guidance in the architecture doc is outdated for
configured-author mode: it still says Redis is always required, but the default
flow should be Redis-free. Update the paragraph to state that Redis/Valkey is
optional in configured-author mode and only required when attribution is
enabled, while keeping the wording aligned with the operator’s
`CommitRequest`/attribution/HA behavior and the plain `helm install` flow.
In `@docs/design/watch-first-ingestion-architecture.md`:
- Around line 443-444: Update the stage-4 note so it no longer treats
configured-author as meaning Redis must back resume cursors; revise the wording
in the design note to clearly separate “Redis configured” from “Redis required”
and reflect the implemented Redis-optional path. Use the existing stage-4
section and the configured-author/audit ingress wording as the anchor for the
edit, and make sure the note states that Redis may be present without being
mandatory for resume cursors.
- Around line 383-386: The operating-modes matrix in the architecture doc is
inconsistent: the configured-author row still implies Redis is part of that
mode, which conflicts with the Redis-free default install. Update the table so
configured-author does not present Redis as required, and make clear that Redis
is only needed for attributed-author; use the existing mode labels in the matrix
to keep the distinction between configured-author and attributed-author
explicit.
In `@test/e2e/e2e_suite_test.go`:
- Around line 126-130: The warmup sentinel check is out of sync between the e2e
suite and the warmup script. Update the downstream grep in
hack/e2e/inject-webhook-tls.sh to use the same sentinel string that
e2e_suite_test.go’s checkConfiguredAuthorMode expects, so the mode gating stays
consistent. Refer to checkConfiguredAuthorMode and the warmup script’s
mode-check logic when making the change.
---
Outside diff comments:
In `@cmd/main.go`:
- Around line 413-417: The `--redis-addr` flag help text is stale:
`fs.StringVar` in `main` still says empty Redis is incompatible with
`--admission-webhook`, but the admission webhook no longer requires Redis.
Update the help string so it only states incompatibility with
`--author-attribution=true`, and keep the rest of the resume-cursor/attribution
description consistent with the current behavior.
- Around line 176-180: Update the Redis configuration comment in main to match
the current validation logic: the admission webhook no longer requires Redis, so
remove that claim from the explanatory comment near the WatchCursorStore setup.
Keep the note consistent with the remaining Redis-dependent behavior by
referring only to the author attribution path and the nil WatchCursorStore
fallback in the relevant main initialization section.
In `@docs/design/watch-first-ingestion-architecture.md`:
- Around line 32-40: Rewrite the Redis dependency summary in the watch-first
ingestion architecture doc so it no longer claims Redis/Valkey is required in
every mode. Update the paragraph around the configured-author and attribution
explanation to reflect that the shipped default can run with queue.redis.addr=""
and therefore Redis is optional in the default Redis-free mode, while keeping
attribution as the separate opt-in layer.
In `@docs/design/watch-first-merge-readiness.md`:
- Around line 119-124: Update the readiness note to match the actual default
install behavior in validateAuditConfig and cmd/main.go. The current wording
incorrectly says empty --redis-addr is always an error and that
configured-author still requires Redis; revise it so Redis is only required when
--author-attribution is enabled, while the Redis-free default remains valid.
Keep the description aligned with the watch-first merge contract and the
attribution toggle wording used by validateAuditConfig, WatchCursorStore, and
the configured-author example.
In `@docs/facts/subresources.md`:
- Around line 434-441: The Attributed example is using the wrong status and
reason for configured-author mode. Update the sample in the subresources
documentation so the condition emitted by the controller matches the real
semantics in that mode: use the same Attributed condition shape shown in the
example, but set the status/reason to reflect AuthorAttributed=False with
CommitterFallback instead of True with AttributedFromAuditEvent.
🪄 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 Plus
Run ID: 4982b063-ab27-4894-b0e6-9fe2ed8c17f6
📒 Files selected for processing (36)
README.mdTaskfile-build.ymlcharts/gitops-reverser/README.mdcharts/gitops-reverser/templates/NOTES.txtcharts/gitops-reverser/templates/deployment.yamlcharts/gitops-reverser/templates/quickstart.yamlcharts/gitops-reverser/templates/validate-redis.yamlcharts/gitops-reverser/values.yamlcmd/main.gocmd/main_audit_server_test.godocs/README.mddocs/TODO.mddocs/UPGRADING.mddocs/architecture.mddocs/attribution-setup-guide.mddocs/configuration.mddocs/design/commitrequest-admission-authorship.mddocs/design/deletecollection-attribution-expander.mddocs/design/status-conditions-guide.mddocs/design/watch-first-ingestion-architecture.mddocs/design/watch-first-merge-readiness.mddocs/facts/subresources.mdinternal/controller/commitrequest_controller_test.gointernal/controller/commitrequest_controller_unit_test.gointernal/watch/author_resolver.gointernal/watch/manager.gointernal/webhook/audit_handler.gointernal/webhook/audit_handler_test.gointernal/webhook/validate_operator_types_handler.gointernal/webhook/validate_operator_types_handler_test.gotest/e2e/commit_author_attribution_e2e_test.gotest/e2e/commit_request_e2e_test.gotest/e2e/controller_basics_e2e_test.gotest/e2e/deletecollection_intent_e2e_test.gotest/e2e/deployment_scale_author_attribution_e2e_test.gotest/e2e/e2e_suite_test.go
…ripts/tests Two review rounds (local agent + CodeRabbit): - README quick start: shell-safe git URL (--set-string OWNER/REPO, no angle brackets that bash treats as redirection), idempotent namespace/Secret creation (--dry-run=client | kubectl apply), HOST:PORT/SECRET placeholders, and a note that the starter GitTarget's SOPS encryption generates a sops-age-key Secret (expected). - charts/README: reconciled the Redis/admission contract (admission runs as a no-op without Redis; Redis required only for attribution) and removed a duplicate servers.admission.enabled row. - Redis-optional consistency in architecture.md and the watch-first design doc (modes matrix + stage-4 note no longer call Redis a hard dependency for configured-author). Rename completeness (missed by the earlier sed, which excluded shell scripts and lowercase-camel identifiers): - committerOnlyModeEnabled -> configuredAuthorModeEnabled (e2e helpers + callers). - hack/e2e/inject-webhook-tls.sh terminology (behavior-neutral: the fast-path grep is unreachable; the audit-root-CA check does the real configured-author detect). - one stray comment in internal/watch/target_watch.go. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…path The quickstart-framework test (helm mode) now drives the README quick start verbatim instead of applying Go-built config objects against the Redis-backed controller: - creates the gitops-reverser-quickstart-demo namespace + Gitea git-creds, then helm-upgrades the release to the README config (queue.redis.addr="", quickstart.enabled=true, that namespace, the Gitea URL/creds); - asserts the controller is healthy in no-Redis configured-author mode (logs the "no Redis configured" sentinel, zero restarts -> admission-on-without-Redis is a healthy no-op, not a crash); - verifies the chart's own example-provider/target/watchrule become Ready; - verifies the starter GitTarget auto-generated its sops-age-key; - verifies a ConfigMap in the demo namespace lands (and delete removes) a commit under live-cluster/<ns>/configmaps/. Manifest/config-dir modes keep the existing Redis-backed config-object flow (the README is a Helm flow, and dist/install.yaml bakes in Redis). Validated: `task test-e2e-quickstart-helm` green (1 passed, README steps exercised). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
test/e2e/quickstart_framework_e2e_test.go (1)
198-200: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueComment says "updates" but code only creates and deletes.
The docstring at line 198 states the function "creates, updates, and deletes" a ConfigMap, but the implementation only creates and deletes — there's no update/patch step. Either add an update step for parity with
verifyQuickstartConfigMapCommitsor correct the comment.📝 Proposed fix: correct the comment
-// verifyStarterConfigMapCommit creates, updates, and deletes a ConfigMap in the +// verifyStarterConfigMapCommit creates and deletes a ConfigMap in the // demo namespace and asserts the chart's starter GitTarget mirrors each change to // live-cluster/<ns>/configmaps/<name>.yaml (the chart default path).🤖 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 `@test/e2e/quickstart_framework_e2e_test.go` around lines 198 - 200, The docstring on verifyStarterConfigMapCommit is inaccurate because the helper only creates and deletes the ConfigMap, but does not perform an update/patch like verifyQuickstartConfigMapCommits. Either add the missing update step to the implementation so the behavior matches the description, or revise the comment to say the function only creates and deletes the ConfigMap and mirrors those changes to the starter GitTarget path.
🤖 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 `@hack/e2e/inject-webhook-tls.sh`:
- Around line 78-79: The configured-author detection in inject-webhook-tls.sh is
matching the wrong startup banner, so the warmup short-circuit never triggers
for that mode. Update the grep in this branch to use the actual log text emitted
by cmd/main.go for configured-author startup, such as the specific
“configured-author mode: no Redis configured” message or a broader
“configured-author mode:” match, so the audit warmup is skipped when
configured-author is detected.
In `@test/e2e/quickstart_framework_e2e_test.go`:
- Around line 186-195: The restart-count assertion in the quickstart E2E test
can pass even when no pods match the selector because `strings.Fields(restarts)`
becomes empty and the loop never runs. In `quickstart_framework_e2e_test.go`,
update the check around `kubectlRunInNamespace(...)` so it first verifies that
at least one pod restart entry was returned before iterating, using the existing
`restarts` value and the `Expect(...)` assertions in this test to fail fast when
the label selector matches nothing.
---
Nitpick comments:
In `@test/e2e/quickstart_framework_e2e_test.go`:
- Around line 198-200: The docstring on verifyStarterConfigMapCommit is
inaccurate because the helper only creates and deletes the ConfigMap, but does
not perform an update/patch like verifyQuickstartConfigMapCommits. Either add
the missing update step to the implementation so the behavior matches the
description, or revise the comment to say the function only creates and deletes
the ConfigMap and mirrors those changes to the starter GitTarget path.
🪄 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 Plus
Run ID: e48a9a5a-7e4a-4035-b440-f8b6691e73f3
📒 Files selected for processing (13)
README.mdcharts/gitops-reverser/README.mddocs/architecture.mddocs/design/watch-first-ingestion-architecture.mdhack/e2e/inject-webhook-tls.shinternal/watch/target_watch.gotest/e2e/commit_author_attribution_e2e_test.gotest/e2e/controller_basics_e2e_test.gotest/e2e/deletecollection_intent_e2e_test.gotest/e2e/deployment_scale_author_attribution_e2e_test.gotest/e2e/e2e_suite_test.gotest/e2e/quickstart_framework_e2e_test.gotest/e2e/watchrule_configmap_secret_e2e_test.go
✅ Files skipped from review due to trivial changes (2)
- internal/watch/target_watch.go
- docs/architecture.md
🚧 Files skipped from review as they are similar to previous changes (5)
- test/e2e/deletecollection_intent_e2e_test.go
- test/e2e/e2e_suite_test.go
- charts/gitops-reverser/README.md
- docs/design/watch-first-ingestion-architecture.md
- README.md
Comment on lines
+78
to
+79
| --since=10m 2>/dev/null | grep -q "watch-first configured-author mode enabled"; then | ||
| echo "✅ Watch-first configured-author mode detected — audit warmup not required" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for the configured-author startup log line in cmd/main.go and related files.
rg -nP 'watch-first.*configured-author|configured-author.*mode.*enabled' cmd/ internal/ --type=go -C2
# Also check for any remaining committer-only references that should have been renamed.
rg -nP 'committer.only|committerOnly' --type=go --type=sh -g '!vendor' -C1Repository: ConfigButler/gitops-reverser
Length of output: 166
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the relevant startup log strings across the repo.
rg -n --hidden --glob '!**/vendor/**' \
'watch-first configured-author mode enabled|configured-author mode:|no Redis configured|watch-first' .
# Map candidate Go files that may emit the startup banner.
fd -t f 'main.go|*.go' cmd internal hack | head -200
# If cmd/main.go exists, show the surrounding lines for any startup logging.
if [ -f cmd/main.go ]; then
rg -n -C 3 'configured-author|watch-first|Redis|warmup|audit' cmd/main.go
fiRepository: ConfigButler/gitops-reverser
Length of output: 12096
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the actual configured-author startup logging in cmd/main.go.
sed -n '224,238p' cmd/main.go | cat -n
# Show the warmup-detection block in hack/e2e/inject-webhook-tls.sh.
sed -n '70,86p' hack/e2e/inject-webhook-tls.sh | cat -n
# Show any nearby comments that explain the intended behavior.
sed -n '100,112p' hack/e2e/inject-webhook-tls.sh | cat -nRepository: ConfigButler/gitops-reverser
Length of output: 3020
Match the startup banner in cmd/main.go Replace grep -q "watch-first configured-author mode enabled" with the actual configured-author log text (configured-author mode: no Redis configured, or a broader configured-author mode: match). As written, this branch never short-circuits for configured-author installs and always runs the audit warmup.
🤖 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 `@hack/e2e/inject-webhook-tls.sh` around lines 78 - 79, The configured-author
detection in inject-webhook-tls.sh is matching the wrong startup banner, so the
warmup short-circuit never triggers for that mode. Update the grep in this
branch to use the actual log text emitted by cmd/main.go for configured-author
startup, such as the specific “configured-author mode: no Redis configured”
message or a broader “configured-author mode:” match, so the audit warmup is
skipped when configured-author is detected.
Comment on lines
+186
to
+195
| restarts, err := kubectlRunInNamespace( | ||
| releaseNs, "get", "pods", "-l", fmt.Sprintf("app.kubernetes.io/instance=%s", release), | ||
| "-o", "jsonpath={.items[*].status.containerStatuses[*].restartCount}", | ||
| ) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| for _, field := range strings.Fields(restarts) { | ||
| count, convErr := strconv.Atoi(field) | ||
| Expect(convErr).NotTo(HaveOccurred()) | ||
| Expect(count).To(Equal(0), "controller must not crash-loop with admission on and no Redis") | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Assert at least one pod was found before iterating restart counts.
If no pods match the app.kubernetes.io/instance=<release> label selector, restarts is empty, strings.Fields returns an empty slice, and the loop body never executes — the test silently passes without verifying any pod's restart count. This defeats the purpose of proving the controller doesn't crash-loop.
🛡️ Proposed fix: guard against empty pod list
Expect(err).NotTo(HaveOccurred())
+ restartFields := strings.Fields(restarts)
+ Expect(restartFields).NotTo(BeEmpty(),
+ "no pods found for release selector — cannot verify restart counts")
- for _, field := range strings.Fields(restarts) {
+ for _, field := range restartFields {
count, convErr := strconv.Atoi(field)
Expect(convErr).NotTo(HaveOccurred())
Expect(count).To(Equal(0), "controller must not crash-loop with admission on and no Redis")
}📝 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.
Suggested change
| restarts, err := kubectlRunInNamespace( | |
| releaseNs, "get", "pods", "-l", fmt.Sprintf("app.kubernetes.io/instance=%s", release), | |
| "-o", "jsonpath={.items[*].status.containerStatuses[*].restartCount}", | |
| ) | |
| Expect(err).NotTo(HaveOccurred()) | |
| for _, field := range strings.Fields(restarts) { | |
| count, convErr := strconv.Atoi(field) | |
| Expect(convErr).NotTo(HaveOccurred()) | |
| Expect(count).To(Equal(0), "controller must not crash-loop with admission on and no Redis") | |
| } | |
| restarts, err := kubectlRunInNamespace( | |
| releaseNs, "get", "pods", "-l", fmt.Sprintf("app.kubernetes.io/instance=%s", release), | |
| "-o", "jsonpath={.items[*].status.containerStatuses[*].restartCount}", | |
| ) | |
| Expect(err).NotTo(HaveOccurred()) | |
| restartFields := strings.Fields(restarts) | |
| Expect(restartFields).NotTo(BeEmpty(), | |
| "no pods found for release selector — cannot verify restart counts") | |
| for _, field := range restartFields { | |
| count, convErr := strconv.Atoi(field) | |
| Expect(convErr).NotTo(HaveOccurred()) | |
| Expect(count).To(Equal(0), "controller must not crash-loop with admission on and no Redis") | |
| } |
🤖 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 `@test/e2e/quickstart_framework_e2e_test.go` around lines 186 - 195, The
restart-count assertion in the quickstart E2E test can pass even when no pods
match the selector because `strings.Fields(restarts)` becomes empty and the loop
never runs. In `quickstart_framework_e2e_test.go`, update the check around
`kubectlRunInNamespace(...)` so it first verifies that at least one pod restart
entry was returned before iterating, using the existing `restarts` value and the
`Expect(...)` assertions in this test to fail fast when the label selector
matches nothing.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What & why
Two connected changes: a terminology rename for the operating modes, and making Redis optional so a bare
helm installis a coherent, infrastructure-free demo.Terminology
Renamed the two modes to name the field that actually varies (the Git author):
committer-only→configured-authorattributed→attributed-authorApplied across docs, chart, and Go comments/logs/tests. API/flag terms are unchanged: the
AuthorAttributedcondition,attribution.enabled, and the Gitcommitterrole keep their names.Redis is now optional but advised
The default install now runs
configured-authorwith no external infrastructure:queue.redis.addrandqueue.redis.auth.existingSecretdefault to"". Without Redis the operator runsconfigured-authorand watches cold-replay on restart.AuthorAttributed=False); it captures authors once Redis is configured. (Startup guard relaxed, command-author store built only when Redis is present, handler no-ops on a nil store — with a unit test.)attribution.enabled=truestill requires Redis; the chart fails that combination at render time (validate-redis.yaml) instead of crash-looping the pod.dist/install.yamlkeeps the full Redis-backed config and regenerates byte-identical — the plain-manifest install path is unchanged.Quickstart demo
gitops-reverser-quickstart-demonamespace.quickstart.createNamespace(defaultfalse): you create the namespace andgit-credsSecret before install, so the starterGitProvideris Ready on the first reconcile instead of waiting up to ~5 min (RequeueSteadyInterval) for the Secret.helm install→ test → teardown).Docs
docs/attribution-setup-guide.md;docs/UPGRADING.mdentry for the default changes; HA goal added todocs/TODO.md.Review findings addressed
helmadded to prerequisites; the "add Valkey" aside now covers what it unlocks + auth.ci.yml.Testing
task vet,task lint(golangci + helm lint),task test(unit + coverage ratchet): pass.dist/install.yaml: no drift.task test-e2e: green on the pre-final state; a final run on this exact code is in progress and will be reported.Not included (follow-up)
Bundling Valkey/Redis in the chart by default was considered and deferred: with the graceful no-op, the default mode needs no Redis, so bundling would only add weight to the minimal demo. A better fit is an opt-in Valkey subchart later. HA (multi-replica) is tracked in
docs/TODO.mdas a near-term goal.🤖 Generated with Claude Code
Summary by CodeRabbit