Repository navigation
fix(registry): preserve dry-run in tap storage - #4225
Conversation
Tap Create and Delete bound the options parameter to _ and built their own metav1.CreateOptions and DeleteOptions for the dynamic client, so the caller's dry-run directive never reached the backing write. A --dry-run=server connect created the Flux OCIRepository and a dry-run disconnect deleted the PackageSource and its source for real, which is the opposite of what a server-side dry-run promises. Forward the directive to all five backing writes, including the orphan-source fallback that had no access to the options at all. Field manager stays as the storage sets it: the objects are synthesized here rather than supplied by the caller, so who owns them is a separate question from whether the write happens. The fake dynamic client's tracker ignores DryRun and persists regardless, so the tests assert the options recorded on the backing action instead of the absence of an object; reverting any one of the five turns the suite red. Assisted-by: LLM Signed-off-by: IvanHunters <[email protected]>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe TAP REST storage now forwards caller dry-run options to backing OCIRepository and PackageSource writes. New tests cover create, update, delete, orphan deletion, and nil-option behavior. ChangesTAP dry-run propagation
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to The dry-run propagation change is covered across create, update, delete, orphan cleanup, and nil-option paths, with no remaining actionable merge risk. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
github-actions
Bot
added
area/uncategorized
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
LGTM. Reverting any one of the five forwards does turn the suite red, and five is the whole set.
Business context: Create and Delete bound their options to _ and built fresh ones for the dynamic client, so a --dry-run=server connect created the Flux OCIRepository and a dry-run disconnect deleted the PackageSource for real.
I reverted each forward on its own, naming the expected failure before running it:
| reverted | predicted | actual |
|---|---|---|
rest.go:241 source create |
TestCreateForwardsDryRunToBackingSource |
that test, nothing else |
rest.go:273 repeat-connect update |
TestCreateForwardsDryRunWhenSourceAlreadyExists |
as predicted |
rest.go:326 PackageSource delete |
TestDeleteForwardsDryRunToBackingDeletes |
as predicted, line 118 |
rest.go:339 unreferenced OCIRepository delete |
same test | as predicted, line 119, separate assertion |
rest.go:363 orphan-source delete |
TestDeleteOrphanTapSourceForwardsDryRun |
as predicted |
Two mutations past the five. deleteDryRun returning DryRunAll unconditionally reddens TestWritesWithoutOptionsDoNotSynthesiseDryRun and nothing else, so that negative control is live rather than vacuous. Dropping the nil guard from createDryRun panics TestCreateRepeatPreservesFinalizerAndRevision, which predates this PR and passes nil options, so both guards are pinned.
On the enumeration: every call in the package that reaches the dynamic client with write intent is one of those five. No patch, no apply, no DeleteCollection, and compute.go makes no client calls at all.
The field manager reasoning holds. None of the five is a server side apply, and dry-run only touches field ownership through an apply's managedFields merge. The two writes that set a field manager set a constant, and the three deletes set none.
One fix for the body rather than the code:
tapswas the last write-capable storage in the aggregated apiserver still discarding its write options.
Read as present tense that is wrong. #3937 is open and unmerged, so its storages still discard on main today: pkg/registry/apps/application/rest.go:609 hands the backing HelmRelease update a fresh &metav1.UpdateOptions{} on the ordinary update path, with no Raw subtlety involved. The sentence holds only in the sense of "the last one no open change was already covering". Smaller point in the same sentence: the diff forwards DryRun alone, so "write options" claims more than it does. Propagation policy, grace period and preconditions still stop here. Leaving them is fine, since a Tap is a computed view over a different object and those fields have no honest mapping onto a PackageSource, and the title and the release note both scope to dry-run.
E2E is red on the opensearch suite and I don't attribute it here. The failing job dumps OpenSearch resources only, opensearch-test-nodes-0 and opensearch-test-bootstrap-0, while this diff touches pkg/registry/core/tap/ and cannot reach an OpenSearch StatefulSet. The same single-suite failure sits on unrelated PRs right now, and #4227 and #4230 look like its homes.
go test ./pkg/registry/... green, go vet and gofmt clean.
2 participants
What this PR does
Tap
CreateandDeletebound the options parameter to_and built their ownmetav1.CreateOptionsandDeleteOptionsfor the dynamic client, so the caller's dry-run directive never reached the backing write.kubectl create --dry-run=serveron a tap created the FluxOCIRepositoryfor real, andkubectl delete tap <name> --dry-run=serverremoved thePackageSourceand its source for real. That is the opposite of what a server-side dry-run promises, and it is reachable from the dashboard and from any client that previews a change before applying it.The directive now reaches all five backing writes: the source create, the update taken on a repeat connect, the
PackageSourcedelete, the delete of an unreferencedOCIRepository, and the orphan-source fallback, which had no access to the options at all and needed them threaded in.Field manager stays as the storage sets it. Tap synthesizes the backing objects rather than storing what the caller sent, so who owns their fields is a separate question from whether the write happens, and only the second one is a correctness bug.
tapswas the last write-capable storage in the aggregated apiserver still discarding its write options. #3937 covers application, tenant-secret and security-group, and excludes this one on the grounds that it uses a different client; the client is not what stood in the way, sincedynamic.Interfacetakes the options by value, dry-run included.Tests
The fake dynamic client's tracker ignores
DryRunand persists regardless, so asserting that an object is absent after a dry-run would pass with or without the fix. The tests assert the options recorded on the backing action instead. Reverting any one of the five forwards, one at a time, turns the suite red.Screenshots
Not a UI change.
Downstream repositories
Walked the trigger map in
docs/agents/contributing.mdagainst the diff. It touches only the write plumbing of thetapsaggregated storage: no package added, renamed or removed, novalues.schema.json, no version enum, no changed default, nokindorplural, norelease.prefix, no renamed output Secret or Service, no hand-typed CRD, and nothing underhack/.Release note
Summary by CodeRabbit
Bug Fixes
Tests