🧹 chore: post-0151 cleanup — dead validation duplicate, ADR-0014 scope, goformat codegen drift - #188
Conversation
…e duplicate The bare variant lost its only caller when the v1alpha1 webhook was deleted (Phase 8e-8b); the webhook calls ...DeleteV1Beta1, which is byte-identical. Kept alive only by its own tests. Retarget the two tests carrying assertions the V1Beta1 suite lacks — the no-dependents allow branch and the multi-kind detail formatting — onto ...DeleteV1Beta1 rather than deleting them, so no assertion is lost. The three per-kind tests (Topic/Channel/auth) are dropped as redundant with TestValidateQueueManagerConnectionDeleteWithV1Beta1Dependents, which exercises all five dependent kinds.
…interval flags fail() returns TerminalRetryInterval for every non-transient QMC error, but ADR-0014, the --terminal-retry-interval help string and the code comment all described it as an auth-only carve-out. Correct the record to match shipped behaviour — the behaviour itself is unchanged and intentional, since a QMC's inputs (Secret, endpoint, TLS material) are all mutable. Also fill the OPERATOR_RUNTIME.md flags table, which listed none of the --*-interval tuning flags nor --mq-request-timeout.
gofmt and goimports were rewriting controller-gen deepcopy and mockery mocks, so a run of `task format` produced codegen drift that `task verify` then flagged — twice during the v0.15.1 fix lane, each costing a restore commit. Detect generated files by the standard "// Code generated ... DO NOT EDIT." header scanned across the file preamble (controller-gen emits it on line 7, below a build tag and license block) rather than by hardcoded path, so new generators are covered automatically. Drops the now-dead v1alpha1 golines exemption.
REQ-REL-2026-08 made workload reconciles return the error when the referenced QueueManagerConnection or Secret is missing, instead of waiting silently on the watch. Applying a directory in one shot (the normal GitOps case) therefore emits rate-limited Reconciler ERROR lines until the referenced object appears. Document that this is the deliberate retry backstop rather than an incident, and when it is genuinely actionable, so operators do not alert on a bare ERROR-rate threshold.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
F2 verified end-to-end against
Tree restored clean afterwards. |
Independent review — APPROVEReviewed diff-only by a reviewer that did not author the branch. No P0 or P1 survived adversarial re-check. Verified (claims that held):
P2 —
|
Independent review caught the runbook section asserting that a QMC applied before its credentials Secret emits rate-limited Reconciler ERROR lines. It does not: a missing Secret surfaces as SecretNotFoundError, which is neither transient nor terminal, so fail() sets Ready=False, emits a SecretNotFound Warning Event and returns RequeueAfter with a nil error — controller-runtime logs nothing. Only the workload-CR ordering produces the ERROR lines. Split the section so each ordering states its own observable signal, and point operators at the condition and Event for the QMC case rather than the log stream. Also tighten the ADR-0014 scope paragraph: the TerminalRetryInterval path is taken when the error is non-transient *and the status update succeeds* — a failed status update returns that error and gets controller-runtime backoff.
Follow-up review found the same wrong claim one layer up: the error-classification table still stated categorically that unclassified errors (incl. missing connection/Secret) return the error to controller-runtime and log at ERROR. True for workload reconcilers, false for the QMC — fail() returns RequeueAfter with a nil error, so nothing is logged. Scope the row per reconciler the way the Terminal and Transient rows already do, and split the follow-on sentence so the Secret case points at the condition and Event instead of the log. Swept the rest of the doc set for the same claim: ADR-0014's principle bullets and the INSTALL_AND_USE condition reference are both framed around Synced (the workload condition), so they are already workload-scoped and correct.
|
Independent review round 3 — APPROVE (head
|
| Round | Head | Verdict | Raised |
|---|---|---|---|
| 1 | 3aa70f8 |
APPROVE | 1×P2 (bootstrap-ordering prose wrong for the QMC path), 3×P3 |
| 2 | cbe6641 |
APPROVE | 1×P2 residual (same claim, unfixed in the table above) |
| 3 | 756d131 |
APPROVE | 2×P3 wording nits |
What round 3 verified against source
- Unclassified row, workload half — matches
setSyncedError(reconcile_shared.go:184-211): unclassified falls to thedefault:branch and returns the error, so controller-runtime emits the ERROR. - Unclassified row, QMC half — matches
fail()(queuemanagerconnection_controller.go:132-154):Ready=FalseviasetCondition, Warning Event viarecordReconcileWarning(which classifiesSecretNotFoundErrorto reasonSecretNotFound,events.go:43-46), andRequeueAfter: TerminalRetryInterval()with a nil error.fail()contains nologger.Error, so "no ERROR line" holds. - The watch claim is real — all five workload controllers (Queue, Topic, Channel, ChannelAuthRule, AuthorityRecord) wire
SetupWithManagerthroughsetupMQObjectController, which registers the QMC watch withconnectionWatchPredicates. - Anchor resolves —
#expected-error-logs-during-bootstrap-orderingmatches the unique heading under default toc slugify. - No contradictions elsewhere — ADR-0014's amendment bullets are explicitly prefixed "for workload reconcilers (
setSyncedError)";INSTALL_AND_USE.md:536,770are framed onSynced(workload-only; QMC usesReady). Neither is categorical.
P3 nits — recorded, deliberately not fixed
docs/OPERATOR_RUNTIME.md:204— "QMC:fail()treats any non-transient error the same way" can read as "same as the workload behaviour just described", which the following clause then contradicts; the intended referent is the Terminal row. Behaviour described after it is accurate.docs/OPERATOR_RUNTIME.md:207-211— the QMC half is described as retrying "on the 2m backstop", omitting its symmetric fast path (the Secret watch,queuemanagerconnection_controller.go:162-166). Not wrong — the linked section immediately below states the watch re-enqueues within seconds and the requeue is the fallback — but asymmetrically incomplete.
Stopping the polish here deliberately: both are wording nits on the same sentence, and another fix round would invalidate this APPROVE and cost a fourth review for no correctness gain. Logged to the INBOX instead.
Ready for maintainer merge. Note for the record: the branch was authored by the same session that coordinated these reviews, so the reviewers — not the author — are the gate. No reviewer approved its own work, and no APPROVE was carried across a change.
2 participants
Why
Post-v0.15.1 cleanup: the four follow-ups recorded against PR #186 and Phase 8e that need no
operator decision. No behaviour change ships in this PR — the one code deletion is unreachable,
and the one behaviour description that changes was wrong about what already ships.
What changed
:fire:Drop deadValidateQueueManagerConnectionDelete— the bare variant lost its onlycaller when the v1alpha1 webhook was deleted in 8e-8b; the live webhook calls the byte-identical
...DeleteV1Beta1. It survived only through its own tests.Deviation from the recorded follow-up, which said "delete it + its redundant tests": deleting all
five tests would have dropped two assertions that the V1Beta1 suite does not carry — the
no-dependents → allow branch, and the multi-kind
formatDependentsdetail assertion. Those twotests are retargeted onto
...DeleteV1Beta1instead; only the three per-kind tests are deleted,as
TestValidateQueueManagerConnectionDeleteWithV1Beta1Dependentsalready exercises all fivedependent kinds.
:memo:Correct the ADR-0014 carve-out scope —fail()returnsTerminalRetryIntervalforevery non-transient QMC error, but ADR-0014, the
--terminal-retry-intervalhelp string and thecode comment all described it as auth-only. This corrects the record to match shipped behaviour;
the behaviour is unchanged and deliberate (a QMC's inputs — Secret, endpoint, TLS material — are
all mutable, so no terminal QMC state is known to be permanent).
Also fills the
OPERATOR_RUNTIME.mdflags table, which listed none of the--*-intervaltuningflags nor
--mq-request-timeout.:wrench:Exclude generated files fromgoformat— gofmt/goimports were rewriting thecontroller-gen deepcopy and mockery mocks, so
task formatproduced codegen drift thattask verifythen flagged. This bit PR #186 twice, each time costing a restore commit. Detection is by the
standard
// Code generated ... DO NOT EDIT.header scanned across the file preamble (controller-genemits it on line 7, under a build tag and licence block) rather than by hardcoded path, so new
generators are covered without editing the script.
:memo:Runbook note on bootstrap-ordering ERROR logs — REQ-REL-2026-08 made a missing QMC orSecret return the error rather than wait silently on the watch, so applying a directory in one shot
(the normal GitOps case) emits rate-limited
Reconciler errorlines until the referenced objectappears. Documents that this is the retry backstop rather than an incident, and when it is
actionable.
What to review
internal/validation/queuemanagerconnection_test.go— specifically thatno assertion was lost relative to what the three deleted tests covered.
documented contract to match code rather than narrowing code to match the ADR.
hack/goformat.shpreamble scan —sed -n '1,/^package /p'.Test plan
Full local gate matrix, all green on
3aa70f8:task test:runtask lint(golangci + go-arch-lint)task verify(codegen, CRD fragments, samples)task format:checktask lint:shelltask lint:markdowntask scrub:treegoformatexclusion verified end-to-end: appended deliberately mis-formatted Go tozz_generated.deepcopy.go, ran the script, confirmed the file was left untouched, then restored it.Coverage was the specific risk here (deleting covered code plus its tests) and both floors hold.
Risks
Low. The deleted function is unreachable, the format change only narrows what gets rewritten, and
the rest is prose. The one thing worth a second pair of eyes is the ADR scope paragraph, which is a
statement about the contract rather than about code.
Out of scope
test work, not cleanup.
Tip
Review commit-by-commit — the four commits are independent and each stands alone.
Review note: this branch has had no independent reviewer. Gates and the coverage check are
machine-verified as above, but the code review is self-review only.