chore: code review pass (hibernation/cocoonset/docs/lint)#6
Merged
Conversation
The Phase/VMName equality short-circuit silently dropped status patches when only Generation moved, leaving ObservedGeneration stuck behind Generation and tricking clients that rely on it for completion polling. Require ObservedGeneration to also equal Generation before skipping the patch, and cover the path with a regression test.
Same class of bug as setPhase: the phase + message equality guard would swallow status patches when only Generation moved, hiding spec bumps behind a stale ObservedGeneration. Tighten the guard and add a regression test.
The webhook already validates this, but a stale CRD or a bypass would let one toolbox silently overwrite another's pod. Add a controller-level guard plus a unit test so the failure surfaces even when the webhook is absent or out of date.
Production wires Epoch unconditionally, so a nil registry at delete time means either a misconfigured deployment or a test fixture. Either way we silently skipped tag GC and left snapshots stranded. Emit a warning so the misconfig surfaces in operator logs.
The "Phase 1" label implies a "Phase 2" downstream that does not exist. Replace with a description of what actually happens.
…ecMatches podSpecMatchesAgent only copies ForkFrom into want for slot > 0, so any manually-edited fork-from on a main pod is treated as drift and triggers a recreate. Document this so future readers don't restore inheritance.
The interface package was renamed but the README directory tree, the architecture diagram, and the related-projects table still referred to it as epoch/SnapshotRegistry. Align all three with the actual snapshot.Registry name and fix the diagram box alignment that drifted along the way.
The cocoon-operator never reads EPOCH_CA_CERT itself; the value is consumed by epoch/registryclient inside NewFromEnv. Point readers at the canonical source so the env name stays accurate even if the registryclient adds siblings (EPOCH_TLS_INSECURE etc.) in the future.
run.tests is false, so test files are never linted and the _test\.go-scoped exclusions (errcheck, gosec, unparam, goconst, gocyclo) have no effect. Drop them to make the config self-consistent. Verified behavior unchanged by running both linux + darwin lint passes.
Adapt to commonlog.Setup now returning an error.
There was a problem hiding this comment.
Pull request overview
This PR is a multi-commit code review pass improving correctness in hibernation status updates, adding defensive validation to CocoonSet toolbox handling, refreshing docs, and simplifying lint configuration.
Changes:
- Fix hibernation status short-circuits so spec-only
Generationbumps still updatestatus.observedGeneration. - Add controller-side rejection for duplicate toolbox names and log a warning when delete runs without a snapshot registry configured.
- Update README/docs references and remove dead golangci-lint per-test exclusions.
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| README.md | Updates repo layout/architecture wording and clarifies epoch registry env var docs. |
| main.go | Handles commonlog.Setup failure explicitly (stderr + exit). |
| hibernation/reconciler.go | Tightens setPhase / markPending short-circuit conditions to include ObservedGeneration. |
| hibernation/reconciler_test.go | Adds regression tests for the ObservedGeneration patching behavior. |
| cocoonset/toolboxes.go | Adds duplicate toolbox-name guard inside ensureToolboxes. |
| cocoonset/reconciler_test.go | Adds test coverage for duplicate toolbox name rejection. |
| cocoonset/pods.go | Doc/comment updates around ForkFrom drift behavior and vmSpecMatches contract. |
| cocoonset/delete.go | Improves delete-path comments and warns when epoch registry is not configured. |
| .golangci.yml | Removes no-op exclusions for _test.go files given run.tests: false. |
| go.mod | Bumps github.com/cocoonstack/cocoon-common dependency version. |
| go.sum | Updates sums to match the cocoon-common bump. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Address Copilot review #4280787224. Hoists the duplicate-name check out of the main loop so an invalid spec like [a, b, a] no longer creates pods for a and b before erroring on the second a.
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
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.
Summary
Nine independent, single-responsibility commits from a focused code-review pass:
setPhase/markPendingshort-circuits so a spec-onlyGenerationbump still surfaces throughObservedGenerationinstead of being silently swallowed.epoch/references in the README, point readers atregistryclientforEPOCH_CA_CERT, replace the misleading "Phase 1" comment inreconcileDelete, and document the ForkFrom drift contract onvmSpecMatches._test\.goexclusion rules from.golangci.ymlsincerun.tests: falsealready skips them. Verified by togglingtests: trueand confirming the rules were no longer doing any work the surrounding config wouldn't.Commits (one logical change each):
fix(hibernation): surface generation bumps in setPhase short-circuitfix(hibernation): surface generation bumps in markPending short-circuitfix(cocoonset): reject duplicate toolbox names in ensureToolboxeschore(cocoonset): warn when delete runs without epoch registrydocs(cocoonset): clarify pod-delete comment in reconcileDeletedocs(cocoonset): note ForkFrom drift behavior for main agents in vmSpecMatchesdocs(readme): correct package name from epoch/ to snapshot/docs(readme): point EPOCH_CA_CERT readers at registryclientchore(golangci): drop dead per-test exclusion rulesTest plan
make lintclean onGOOS=linuxmake lintclean onGOOS=darwingo test -race -count=1 ./...cleanmake fmt-checkcleanmake buildsucceedssetPhase/markPendingpatches and the newensureToolboxesduplicate-name guard