kacerrandClaude Opus 5.5
60360ca5b9
- ci: reject a manual dispatch with tag "latest"; concurrency group per tag - Dockerfile: declare GIT_COMMIT after the module download layer - Makefile: docker-build targets linux/amd64 by default - patch.go: no shared package-level pointers; main.go: no init() - tests: assert that merge replaces managed listener content - docs: flows.md startup section matches main.go; review saved to docs/reviews Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
57 lines
2.8 KiB
Markdown
57 lines
2.8 KiB
Markdown
# Review: house conventions port
|
|
|
|
**Created:** 2026-10-08 20:40 CEST
|
|
**Scope:** `main...feat/house-conventions` at `8c07428` (7 commits)
|
|
**Reviewers:** `operator-reviewer`, `go-operator-reviewer`
|
|
|
|
Both reviewers ran `make check` and `go test -race ./...` (green) and confirmed the
|
|
branch changes no reconcile behaviour. No merge-blockers.
|
|
|
|
## Worth fixing
|
|
|
|
- [x] `build.yaml`: a manual dispatch with the default `tag: latest` overwrote
|
|
`:latest`, the tag the Deployment runs. Default removed; `latest` is rejected.
|
|
(Inherited from the egress workflow, which still has it.)
|
|
- [x] `Dockerfile`: `ARG GIT_COMMIT` sat above `go mod download`, so every commit
|
|
re-downloaded all modules. Moved to just above the build.
|
|
- [x] `patch_test.go`: `TestMergeListeners_ownership` compared names only; a
|
|
mutation that kept stale listener content passed. Added
|
|
`TestMergeListeners_replacesManagedContent`.
|
|
- [x] `make docker-build` followed the host architecture (arm64 on this Mac, where
|
|
the Dockerfile used to hard-code amd64). Now builds `linux/amd64` by default.
|
|
- [x] `docs/flows.md` §2 described the old `main.go` (line refs, hardcoded dev
|
|
logger, no `--version`). Rewritten against the current file.
|
|
- [x] `CLAUDE.md` overstated the reverse watch. Reworded.
|
|
- [x] `build.yaml`: two dispatches from the same ref shared a concurrency group and
|
|
cancelled each other. Group now includes the tag input.
|
|
|
|
## Simplifications
|
|
|
|
- [x] `patch.go`: package-level `tlsModeTerminate` / `namespacesFromAll` were shared
|
|
by address across every listener. Replaced with `ptrTo(...)`; `defaultPort` is a
|
|
const.
|
|
- [x] `main.go`: `init()` removed, scheme built in `main()` (CLAUDE.md says no
|
|
`init()`).
|
|
- [x] `Makefile` comment claimed the lint toolchain pin "matches CI"; CI does not
|
|
lint. Claim dropped.
|
|
- [ ] `TestListenerNameFor_sanitizesDots` tests an unexported helper and overlaps the
|
|
names table. Kept as the home for truncation and collision cases in the bug-fix
|
|
phase.
|
|
|
|
## Deferred to the bug-fix phase (new, not in the plan's backlog)
|
|
|
|
- [ ] Empty desired set: when a Gateway has only `auto-*` listeners and no cert
|
|
yields any, `MergeListeners` returns nil and the patch sends `listeners: null`,
|
|
which the API server rejects (`MinItems=1`). Permanent error requeue. Any fix for
|
|
"last cert deleted" runs straight into this. `patch.go` `MergeListeners`,
|
|
`certificate_controller.go:119-120`.
|
|
- [ ] Listeners are added before the Certificate is Ready, so they reference a
|
|
Secret that may not exist yet.
|
|
- [ ] ClusterRole grants `events` create/patch; nothing records events.
|
|
- [ ] CI `check` job has no golangci-lint and no `-race`. Needs a linter install
|
|
step on the runner (uh-cli does it from a pinned tarball).
|
|
|
|
## Nice-to-have
|
|
|
|
- [x] Assert the secret ref `Group` in `TestBuildListenersFromCertificates_listenerFields`.
|