kacerr/gatewayapi-certificates-operator
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity
Files
b114da7a177e0d435e841660608f087b08b93995
gatewayapi-certificates-ope…/docs/reviews/2026-10-08-2040-house-conventions.md
T
kacerrandClaude Opus 5.5 60360ca5b9 fix: address review findings on the conventions branch 
- 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>
2026-10-08 20:43:02 +02:00

2.8 KiB
Raw Blame History

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

  • 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.)
  • Dockerfile: ARG GIT_COMMIT sat above go mod download, so every commit re-downloaded all modules. Moved to just above the build.
  • patch_test.go: TestMergeListeners_ownership compared names only; a mutation that kept stale listener content passed. Added TestMergeListeners_replacesManagedContent.
  • 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.
  • docs/flows.md §2 described the old main.go (line refs, hardcoded dev logger, no --version). Rewritten against the current file.
  • CLAUDE.md overstated the reverse watch. Reworded.
  • build.yaml: two dispatches from the same ref shared a concurrency group and cancelled each other. Group now includes the tag input.

Simplifications

  • patch.go: package-level tlsModeTerminate / namespacesFromAll were shared by address across every listener. Replaced with ptrTo(...); defaultPort is a const.
  • main.go: init() removed, scheme built in main() (CLAUDE.md says no init()).
  • 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

  • Assert the secret ref Group in TestBuildListenersFromCertificates_listenerFields.
Reference in New Issue View Git Blame Copy Permalink