Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughOwnership moved from positional parameters into option-based APIs; Phase and Revision became interface-backed builders; objects use controller-runtime client.Object; comparator, object, phase, and revision engines accept variadic options (e.g., WithOwner); Reconciler gained ownerStrategy and revisions are built via NewRevision/NewRevisionWithOwner. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant RevEngine as RevisionEngine
participant PhaseEngine as PhaseEngine
participant ObjEngine as ObjectEngine
participant Comparator as Comparator
participant K8s as KubernetesAPI
Caller->>RevEngine: Reconcile(ctx, revision, opts...)
RevEngine->>PhaseEngine: Reconcile(revisionNumber, phase, opts...)
PhaseEngine->>ObjEngine: Reconcile(revisionNumber, desiredObj, options.ForObject(obj)...)
ObjEngine->>Comparator: Compare(desiredObj, actualObj, WithOwner(owner, strat))
Comparator->>ObjEngine: CompareResult
ObjEngine->>K8s: Apply/Update/Delete (use fieldOwner, ownerRef from options)
K8s-->>ObjEngine: API response
ObjEngine-->>PhaseEngine: ObjectResult
PhaseEngine-->>RevEngine: PhaseResult
RevEngine-->>Caller: Aggregate result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
cmd/reference/internal/util.go (1)
37-43: Consider adding nil check for owner object.If
getOwnerFromRev(rev)returnsnil(e.g., when a revision has no owner option set), the subsequent calls torefObj.GetObjectKind(),GetName(), etc. will panic.🛡️ Proposed defensive check
for _, rev := range prevRevisions { refObj := getOwnerFromRev(rev) + if refObj == nil { + continue // skip revisions without owners + } ref := unstructured.Unstructured{} ref.SetGroupVersionKind(refObj.GetObjectKind().GroupVersionKind())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/reference/internal/util.go` around lines 37 - 43, getOwnerFromRev(rev) can return nil, causing a panic when calling refObj.GetObjectKind()/GetName()/GetNamespace()/GetUID(); add a nil check after calling getOwnerFromRev(rev) in the block that builds ref (the refObj variable) and handle the nil case (e.g., skip appending to data or return an error) so you only call refObj methods when refObj != nil before creating and appending ref to data.machinery/types/types_test.go (1)
218-251: Consider usingNewPhaseconstructor for consistency.The test constructs phases using the internal
&phase{}struct directly (lines 219, 233) while other tests in this file use the publicNewPhaseconstructor. Using the constructor would improve consistency and decouple tests from internal implementation details.♻️ Suggested refactor
phases := []Phase{ - &phase{ - Name: "phase1", - Objects: []client.Object{ - &unstructured.Unstructured{ + NewPhase("phase1", []client.Object{ + &unstructured.Unstructured{ Object: map[string]interface{}{ "apiVersion": "v1", "kind": "ConfigMap", "metadata": map[string]interface{}{ "name": "test-cm", }, }, - }, }, - }, - &phase{ - Name: "phase2", - Objects: []client.Object{ - &unstructured.Unstructured{ + }), + NewPhase("phase2", []client.Object{ + &unstructured.Unstructured{ Object: map[string]interface{}{ "apiVersion": "v1", "kind": "Secret", "metadata": map[string]interface{}{ "name": "test-secret", }, }, - }, }, - }, + }), }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@machinery/types/types_test.go` around lines 218 - 251, The test directly instantiates the unexported phase struct (&phase{...}) instead of using the public NewPhase constructor; update the two phase creations to call NewPhase(...) (preserving Name and Objects) so the test uses Phase via the NewPhase constructor and no longer depends on the internal type, then pass the resulting Phase slices into NewRevision("test", 2, phases) as before.machinery/types/options.go (1)
380-410: Minor: Parameter namestartappears to be a typo.The parameter
start ownerStrategywould be clearer asstratorstrategyto match its purpose.✏️ Suggested parameter rename
-func WithOwner(obj client.Object, start ownerStrategy) interface { +func WithOwner(obj client.Object, strat ownerStrategy) interface { ComperatorOption ObjectReconcileOption ObjectTeardownOption } { if len(obj.GetUID()) == 0 { panic("owner must be persisted to cluster, empty UID") } return &combinedOpts{ fn: func(opts *ComperatorOptions) { opts.Owner = obj - opts.OwnerStrategy = start + opts.OwnerStrategy = strat }, optionFn: optionFn{ fn: func(opts *ObjectReconcileOptions) { opts.Owner = obj - opts.OwnerStrategy = start + opts.OwnerStrategy = strat }, }, teardownOptionFn: teardownOptionFn{ fn: func(opts *ObjectTeardownOptions) { opts.Owner = obj - opts.OwnerStrategy = start + opts.OwnerStrategy = strat }, }, } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@machinery/types/options.go` around lines 380 - 410, Rename the misleading parameter name "start" in the WithOwner function to a clearer name like "strategy" (signature: WithOwner(obj client.Object, strategy ownerStrategy)) and update all references inside the function (e.g., opts.OwnerStrategy = strategy) as well as any call sites that pass that argument to use the new parameter name; ensure the function still returns the same combinedOpts and assigns the OwnerStrategy field in ComperatorOptions, ObjectReconcileOptions, and ObjectTeardownOptions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/reference/internal/util.go`:
- Around line 64-71: The owner object returned by getOwnerFromRev (which calls
RevisionReconcileOptions.GetOwner) may be nil, so before using refObj (and
calling refObj.GetObjectKind().GroupVersionKind()) add a nil check and handle
the missing owner case (e.g., skip this revision with continue or return a clear
error); update the code that assigns refObj to verify refObj != nil and branch
accordingly to avoid a panic.
In `@machinery/objects.go`:
- Around line 270-299: When options.Owner is non-nil you must validate
options.OwnerStrategy is also non-nil before calling detectOwner or
types.WithOwner to avoid panics; add a guard in the reconcile path (the block
checking if options.Owner == nil) or in ObjectReconcileOptions.Default() that
returns an error if Owner is set but OwnerStrategy is nil, referencing the
detectOwner(...) call and the types.WithOwner(...) usage so callers are
prevented from passing a nil OwnerStrategy.
In `@machinery/types/options.go`:
- Around line 422-429: Rename the misspelled exported types ComperatorOptions
and ComperatorOption to ComparatorOptions and ComparatorOption respectively,
updating their type declarations and the method ApplyToComperatorOptions to
ApplyToComparatorOptions (and any receiver names) and all usages in
machinery/comparator.go, machinery/objects.go and test files to use the new
names; ensure you also update any functions, constructors, interfaces, or
variables that reference ComperatorOptions/ComperatorOption (including calls
that accept or return these types) so the public API matches the correctly
spelled existing Comparator type while preserving existing fields (Owner,
ownerStrategy) and behavior.
In `@machinery/types/types.go`:
- Around line 115-116: The doc comment above the method WithTeardownOptions is
incorrect: it reads "WithReconcileOptions" but should describe
WithTeardownOptions; update the comment to accurately state that
WithTeardownOptions sets PhaseTeardownOptions on this phase (or similar accurate
wording). Locate the comment immediately preceding the phase.WithTeardownOptions
method in types.go and replace the incorrect text so the comment and the method
name match.
- Around line 51-52: Update the doc comment above the WithTeardownOptions method
to correctly reference the method name and behavior: change the text from
"WithReconcileOptions sets PhaseReconcileOptions on this phase." to a concise
description like "WithTeardownOptions sets PhaseTeardownOptions on this phase."
Ensure the comment references WithTeardownOptions, PhaseTeardownOption, and
PhaseBuilder so the documentation matches the signature.
In `@validation/phase_test.go`:
- Around line 289-290: The test calls realValidator.Validate(t.Context(),
test.phase, types.WithOwner(test.owner, nil)) but WithOwner panics if the owner
UID is empty; guard by ensuring test.owner has a non-empty UID before passing it
to types.WithOwner (e.g., set a dummy UID when test.owner.UID is empty) so the
path using realValidator (controlled by useRealValidator) won't panic—update the
test setup around realValidator.Validate to normalize test.owner.UID when empty.
---
Nitpick comments:
In `@cmd/reference/internal/util.go`:
- Around line 37-43: getOwnerFromRev(rev) can return nil, causing a panic when
calling refObj.GetObjectKind()/GetName()/GetNamespace()/GetUID(); add a nil
check after calling getOwnerFromRev(rev) in the block that builds ref (the
refObj variable) and handle the nil case (e.g., skip appending to data or return
an error) so you only call refObj methods when refObj != nil before creating and
appending ref to data.
In `@machinery/types/options.go`:
- Around line 380-410: Rename the misleading parameter name "start" in the
WithOwner function to a clearer name like "strategy" (signature: WithOwner(obj
client.Object, strategy ownerStrategy)) and update all references inside the
function (e.g., opts.OwnerStrategy = strategy) as well as any call sites that
pass that argument to use the new parameter name; ensure the function still
returns the same combinedOpts and assigns the OwnerStrategy field in
ComperatorOptions, ObjectReconcileOptions, and ObjectTeardownOptions.
In `@machinery/types/types_test.go`:
- Around line 218-251: The test directly instantiates the unexported phase
struct (&phase{...}) instead of using the public NewPhase constructor; update
the two phase creations to call NewPhase(...) (preserving Name and Objects) so
the test uses Phase via the NewPhase constructor and no longer depends on the
internal type, then pass the resulting Phase slices into NewRevision("test", 2,
phases) as before.
thetechnick
force-pushed
the
owner-optional
branch
from
5404537 to
fcf8fcf
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
validation/object.go (1)
64-86:⚠️ Potential issue | 🟠 MajorAvoid skipping namespace/scope validation when owner is omitted.
In namespaced mode (
allowNamespaceEscalation=false), namespace validation now only runs whenoptions.Owner != nil. If a caller forgetsWithOwner, cluster-scoped or cross-namespace objects can pass preflight validation. Consider failing fast whenOwneris missing (or require an explicit “allow escalation” option) to avoid silently weakening guarantees.🔒 Possible guard
- if options.Owner != nil && !d.allowNamespaceEscalation { - // Ensure we are not leaving the namespace we are operating in. - if err := validateNamespace( - d.restMapper, options.Owner.GetNamespace(), obj, - ); err != nil { - errs = append(errs, err) - // we don't want to do a dry-run when this already fails. - return NewObjectValidationError(bctypes.ToObjectRef(obj), errs...) - } - } + if !d.allowNamespaceEscalation { + if options.Owner == nil { + errs = append(errs, errors.New("owner required for namespaced validation")) + return NewObjectValidationError(bctypes.ToObjectRef(obj), errs...) + } + // Ensure we are not leaving the namespace we are operating in. + if err := validateNamespace( + d.restMapper, options.Owner.GetNamespace(), obj, + ); err != nil { + errs = append(errs, err) + // we don't want to do a dry-run when this already fails. + return NewObjectValidationError(bctypes.ToObjectRef(obj), errs...) + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@validation/object.go` around lines 64 - 86, The Validate method currently skips namespace/scope checks when options.Owner is nil, allowing unintended cross-namespace/cluster objects in namespaced mode; update ObjectValidator.Validate to treat a missing Owner as a validation failure when d.allowNamespaceEscalation == false: if options.Owner == nil && !d.allowNamespaceEscalation, append an error (e.g., "missing owner prevents namespace validation" or similar) to errs and return NewObjectValidationError(bctypes.ToObjectRef(obj), errs...) before proceeding to validateNamespace; keep the existing validateNamespace call for the non-nil Owner path.
🧹 Nitpick comments (1)
machinery/phases_test.go (1)
97-98: Stale mock expectation may cause test issues.The mock at line 97 still uses the old signature with
ownerparameter:oe.On("Reconcile", mock.Anything, owner, revision, obj, mock.Anything)But the actual
objectEngineMock.Reconcilemethod (lines 159-168) has the new signature withoutowner. This mock expectation will never match, though the test passes becauseAssertNotCalledis used. Consider updating to:oe.On("Reconcile", mock.Anything, revision, obj, mock.Anything)♻️ Suggested fix
- oe.On("Reconcile", mock.Anything, owner, revision, obj, mock.Anything). + oe.On("Reconcile", mock.Anything, revision, obj, mock.Anything). Return(newObjectResultCreated(obj, types.ObjectReconcileOptions{}), nil)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@machinery/phases_test.go` around lines 97 - 98, The mock expectation for oe.On("Reconcile", ...) is using the old signature that includes the owner parameter and thus never matches the actual objectEngineMock.Reconcile signature; update the expectation to match the new signature used by objectEngineMock.Reconcile by removing the owner argument so the call becomes oe.On("Reconcile", mock.Anything, revision, obj, mock.Anything) (ensure you reference the oe.On mock setup and the objectEngineMock.Reconcile method to verify the parameter list).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@machinery/types/types.go`:
- Around line 210-211: Fix the typo in the doc comment for the method
revision.GetTeardownOptions: change "tearding" to "tearing" so the comment reads
"returns options for tearing down this revision" (update the comment immediately
above the GetTeardownOptions function declaration in the revision type).
- Around line 103-104: The doc comment for method GetTeardownOptions on type
phase has a typo ("tearding"); update the comment to "tearing" so it reads:
"GetTeardownOptions returns options for tearing down this phase." Ensure you
edit the comment immediately above the GetTeardownOptions method (and leave the
function signature and return type PhaseTeardownOption unchanged).
- Around line 132-133: Update the doc comment for the GetTeardownOptions method
to fix the typo "tearding" to "tearing" so it reads "GetTeardownOptions returns
options for tearing down this revision."; locate the comment immediately above
the GetTeardownOptions() []RevisionTeardownOption declaration and replace the
misspelled word while leaving the method signature and return type unchanged.
- Around line 42-43: Doc comment for GetTeardownOptions contains a typo
("tearding") — update the comment to read "tearing" so it correctly documents
GetTeardownOptions() []PhaseTeardownOption; locate the comment immediately above
the GetTeardownOptions method signature and replace "tearding down this phase"
with "tearing down this phase".
---
Outside diff comments:
In `@validation/object.go`:
- Around line 64-86: The Validate method currently skips namespace/scope checks
when options.Owner is nil, allowing unintended cross-namespace/cluster objects
in namespaced mode; update ObjectValidator.Validate to treat a missing Owner as
a validation failure when d.allowNamespaceEscalation == false: if options.Owner
== nil && !d.allowNamespaceEscalation, append an error (e.g., "missing owner
prevents namespace validation" or similar) to errs and return
NewObjectValidationError(bctypes.ToObjectRef(obj), errs...) before proceeding to
validateNamespace; keep the existing validateNamespace call for the non-nil
Owner path.
---
Duplicate comments:
In `@cmd/reference/internal/util.go`:
- Around line 36-42: prevJSON currently dereferences the result of
getOwnerFromRev(rev) into refObj without checking for nil, which can panic when
no owner is set; update prevJSON to check if refObj == nil after calling
getOwnerFromRev and handle it (either skip that revision with a clear
log/continue or return an explicit error) before calling methods on refObj
(GetObjectKind, GetName, GetNamespace, GetUID); apply the same nil-guard pattern
to the similar block around lines 64-70 so both uses of getOwnerFromRev are
protected.
In `@machinery/objects.go`:
- Around line 270-299: The code can panic if callers set
ObjectReconcileOptions.Owner directly without setting OwnerStrategy; add a
validation in ObjectReconcileOptions.Default() to detect when Owner != nil and
OwnerStrategy == nil and either assign a sensible default OwnerStrategy or
return an error, so callers who bypass types.WithOwner() don't cause panics in
detectOwner or comparator.Compare; update Default() to enforce or normalize
options.Owner and options.OwnerStrategy consistently (reference
ObjectReconcileOptions.Default, WithOwner, detectOwner, comparator.Compare, and
the fields Owner/OwnerStrategy).
In `@machinery/types/options.go`:
- Around line 426-433: The exported types are misspelled: rename
ComperatorOptions -> ComparatorOptions and ComperatorOption -> ComparatorOption
across the file and update their methods/signatures (e.g.,
ApplyToComperatorOptions -> ApplyToComparatorOptions) so they match the
correctly-spelled Comparator in machinery/comparator.go; update all references
to the fields and types (Owner, ownerStrategy, any implementations of
ComperatorOption) to use the new names to restore consistency with Comparator
types and avoid breaking imports.
In `@validation/phase_test.go`:
- Around line 288-289: The test calls types.WithOwner(test.owner, nil) which
panics if the owner's UID is empty; to fix, ensure the test owner has a
non-empty UID before constructing the WithOwner option: before creating
realValidator/ calling Validate, add a small guard that if test.owner != nil and
test.owner.GetUID() is empty, set a deterministic UID (e.g. "test-owner-uid") on
test.owner (use the owner object's SetUID method); this prevents WithOwner from
panicking when NewClusterPhaseValidator.Validate is exercised.
---
Nitpick comments:
In `@machinery/phases_test.go`:
- Around line 97-98: The mock expectation for oe.On("Reconcile", ...) is using
the old signature that includes the owner parameter and thus never matches the
actual objectEngineMock.Reconcile signature; update the expectation to match the
new signature used by objectEngineMock.Reconcile by removing the owner argument
so the call becomes oe.On("Reconcile", mock.Anything, revision, obj,
mock.Anything) (ensure you reference the oe.On mock setup and the
objectEngineMock.Reconcile method to verify the parameter list).
machinery/types/types.go
Outdated
Comment on lines
132
to
133
| // GetTeardownOptions returns options for tearding down this revision. | ||
| GetTeardownOptions() []RevisionTeardownOption |
There was a problem hiding this comment.
Typo in doc comment: "tearding" → "tearing".
📝 Proposed fix
- // GetTeardownOptions returns options for tearding down this revision.
+ // GetTeardownOptions returns options for tearing down this revision.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // GetTeardownOptions returns options for tearding down this revision. | |
| GetTeardownOptions() []RevisionTeardownOption | |
| // GetTeardownOptions returns options for tearing down this revision. | |
| GetTeardownOptions() []RevisionTeardownOption |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@machinery/types/types.go` around lines 132 - 133, Update the doc comment for
the GetTeardownOptions method to fix the typo "tearding" to "tearing" so it
reads "GetTeardownOptions returns options for tearing down this revision.";
locate the comment immediately above the GetTeardownOptions()
[]RevisionTeardownOption declaration and replace the misspelled word while
leaving the method signature and return type unchanged.
✅ Addressed in commit 06bcb93
thetechnick
force-pushed
the
owner-optional
branch
from
fcf8fcf to
70926c0
Compare
Co-Authored-By Claude
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #459 +/- ##
==========================================
+ Coverage 69.05% 71.73% +2.67%
==========================================
Files 35 35
Lines 2870 2986 +116
==========================================
+ Hits 1982 2142 +160
+ Misses 750 715 -35
+ Partials 138 129 -9 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
validation/phase_test.go (1)
22-52: Align test helper with option-based validator signatures.
The mock still models the old owner-parameter API, so option wiring isn’t exercised in these tests.♻️ Suggested update
-func (m *mockObjectValidator) Validate(ctx context.Context, owner client.Object, obj client.Object) error { - args := m.Called(ctx, owner, obj) +func (m *mockObjectValidator) Validate(ctx context.Context, obj client.Object, opts ...types.ObjectReconcileOption) error { + args := m.Called(ctx, obj, opts) return args.Error(0) }-func (v *testablePhaseValidator) Validate(ctx context.Context, owner client.Object, phase types.Phase) error { - phaseError := validatePhaseName(phase) +func (v *testablePhaseValidator) Validate(ctx context.Context, phase types.Phase, opts ...types.PhaseReconcileOption) error { + var options types.PhaseReconcileOptions + for _, opt := range opts { + opt.ApplyToPhaseReconcileOptions(&options) + } + phaseError := validatePhaseName(phase) var ( objectErrors []ObjectValidationError errs []error ) for _, obj := range phase.GetObjects() { - err := v.mockObjValidator.Validate(ctx, owner, obj) + err := v.mockObjValidator.Validate(ctx, obj, options.ForObject(obj)...)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@validation/phase_test.go` around lines 22 - 52, Tests use a mockObjectValidator whose Validate still takes an explicit owner parameter, so option-based wiring isn’t tested; change mockObjectValidator.Validate to match the new signature by accepting ctx, obj client.Object, opts ...PhaseValidationOption and update testablePhaseValidator.Validate to call v.mockObjValidator.Validate(ctx, obj, WithOwner(owner)) (or your project's equivalent owner PhaseValidationOption) for each obj instead of passing owner as a separate arg, keeping the existing handling of returned errors (ObjectValidationError, errs) and leaving validatePhaseName/PhaseValidator logic unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@machinery/phases_test.go`:
- Line 62: The test is calling types.WithOwner(owner, nil) which leaves
OwnerStrategy nil and will panic when ObjectReconcileOptions.Default() is
invoked; update the test to provide a non-nil OwnerStrategy (either a
lightweight real implementation or a test/mock implementation) when calling
types.WithOwner so Owner and OwnerStrategy are consistent, ensuring calls to
ObjectReconcileOptions.Default() do not panic; locate usages of types.WithOwner
in the test (and any helper that constructs reconcile options) and replace the
nil strategy with a suitable concrete or mock OwnerStrategy instance.
---
Duplicate comments:
In `@cmd/reference/internal/util.go`:
- Around line 37-44: The code dereferences refObj returned by
getOwnerFromRev(rev) without a nil check and may panic; update the loop handling
to first check if refObj == nil and skip creating/refilling the
unstructured.Unstructured (ref) when nil (i.e., continue), otherwise proceed to
call ref.SetGroupVersionKind(...), ref.SetName(...), ref.SetNamespace(...),
ref.SetUID(...), and append to data; ensure getOwnerFromRev is referenced and
ref is only used when refObj is non-nil.
In `@machinery/types/types.go`:
- Around line 210-211: Doc comment for GetTeardownOptions contains a typo
"tearding" — update the comment(s) to read "tearing" instead; locate the
GetTeardownOptions declaration and the revision.GetTeardownOptions
implementation and fix the doc string in both places so they use "tearing down"
rather than "tearding".
- Around line 42-43: The doc comment for GetTeardownOptions contains a typo
("tearding"); update the comment to read "tearing down this phase" so the method
signature GetTeardownOptions() []PhaseTeardownOption has a correct description
and matches project docs/conventions.
- Around line 103-104: Doc comment for GetTeardownOptions contains a typo
"tearding" — change it to "tearing"; update the comment above the method on type
phase (GetTeardownOptions) and the matching comment in the Phase interface to
use "tearing" instead of "tearding" so both declarations/docs stay consistent;
verify the comments immediately above the GetTeardownOptions func and the Phase
interface method signature are corrected to "tearing down this phase."
- Around line 132-133: Fix the typo in the doc comments: change "tearding down
this revision" to "tearing down this revision" for the GetTeardownOptions method
comment and the same doc comment on the Revision interface so both references
use "tearing" instead of "tearding".
In `@validation/phase_test.go`:
- Around line 288-290: The test calls types.WithOwner(test.owner, nil) which
will panic if test.owner.UID is empty; update the real validator path around
NewClusterPhaseValidator/realValidator.Validate to guard against an empty owner
UID by checking test.owner != nil && test.owner.UID != "" before calling
types.WithOwner, and if empty either pass nil as the owner option or skip
WithOwner and call Validate without that option so the code no longer panics
when owner UID is missing.
---
Nitpick comments:
In `@validation/phase_test.go`:
- Around line 22-52: Tests use a mockObjectValidator whose Validate still takes
an explicit owner parameter, so option-based wiring isn’t tested; change
mockObjectValidator.Validate to match the new signature by accepting ctx, obj
client.Object, opts ...PhaseValidationOption and update
testablePhaseValidator.Validate to call v.mockObjValidator.Validate(ctx, obj,
WithOwner(owner)) (or your project's equivalent owner PhaseValidationOption) for
each obj instead of passing owner as a separate arg, keeping the existing
handling of returned errors (ObjectValidationError, errs) and leaving
validatePhaseName/PhaseValidator logic unchanged.
| obj, | ||
| }, | ||
| }) | ||
| ), types.WithOwner(owner, nil)) |
There was a problem hiding this comment.
Passing nil ownerStrategy will panic if Default() is called.
types.WithOwner(owner, nil) sets Owner to a non-nil value while OwnerStrategy remains nil. When ObjectReconcileOptions.Default() is called (line 128-130 in options.go), this combination triggers a panic. The tests likely pass because the mock doesn't actually call Default(), but this creates inconsistency with production behavior.
Consider using a real or mock OwnerStrategy in tests:
🧪 Proposed fix
+ ownerStrat := ownerhandling.NewNative(scheme.Scheme)
+
_, err := pe.Reconcile(t.Context(), revision, types.NewPhase(
"test",
[]client.Object{
obj,
},
- ), types.WithOwner(owner, nil))
+ ), types.WithOwner(owner, ownerStrat))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@machinery/phases_test.go` at line 62, The test is calling
types.WithOwner(owner, nil) which leaves OwnerStrategy nil and will panic when
ObjectReconcileOptions.Default() is invoked; update the test to provide a
non-nil OwnerStrategy (either a lightweight real implementation or a test/mock
implementation) when calling types.WithOwner so Owner and OwnerStrategy are
consistent, ensuring calls to ObjectReconcileOptions.Default() do not panic;
locate usages of types.WithOwner in the test (and any helper that constructs
reconcile options) and replace the nil strategy with a suitable concrete or mock
OwnerStrategy instance.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
machinery/types/types.go (1)
109-120: Builder methods replace options rather than append.Both
WithReconcileOptionsandWithTeardownOptionsreplace existing options entirely rather than appending to them. This is a valid design choice for a builder pattern, but be aware that calling these methods multiple times will overwrite previous options.If accumulating options is needed, consider documenting this behavior or providing
AddReconcileOptions/AddTeardownOptionsvariants.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@machinery/types/types.go` around lines 109 - 120, The current builder methods WithReconcileOptions and WithTeardownOptions overwrite existing slices (p.ReconcileOptions and p.TeardownOptions) instead of appending, so repeated calls clobber previous options; to fix, either change each method to append new opts to the existing slice (e.g., p.ReconcileOptions = append(p.ReconcileOptions, opts...)) and likewise for p.TeardownOptions, or add new methods AddReconcileOptions/AddTeardownOptions that perform the append and update the PhaseBuilder interface and docs to expose the new behavior; update comments on With* to document that they replace options if you keep the current behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@machinery/types/types.go`:
- Around line 63-75: The exported function NewPhaseWithOwner currently accepts
the unexported ownerStrategy type which prevents external callers from passing a
strategy; change the parameter type from ownerStrategy to the exported
OwnerStrategy interface (use the OwnerStrategy defined in the boxcutter package)
and update any references where ownerStrat is passed (e.g. the call to
WithOwner(owner, ownerStrat)) so
WithOwner/WithReconcileOptions/WithTeardownOptions receive the exported
OwnerStrategy; keep the rest of the function (creation of phase and chaining of
WithReconcileOptions/WithTeardownOptions) unchanged and ensure imports include
the package that defines OwnerStrategy.
---
Duplicate comments:
In `@machinery/types/types.go`:
- Around line 103-104: Fix the doc comment on the GetTeardownOptions method:
change the misspelled word "tearding" to "tearing" in the comment above func (p
*phase) GetTeardownOptions() []PhaseTeardownOption; ensure the comment reads
"GetTeardownOptions returns options for tearing down this phase." and keep the
rest of the comment format intact.
- Around line 132-133: The doc comment for GetTeardownOptions contains a typo
("tearding"); update the comment above the GetTeardownOptions() method to read
"GetTeardownOptions returns options for tearing down this revision." and ensure
the signature GetTeardownOptions() []RevisionTeardownOption and the referenced
type RevisionTeardownOption remain unchanged.
- Around line 42-43: Fix the typo in the doc comment for the GetTeardownOptions
method: change "tearding" to "tearing" in the comment above GetTeardownOptions()
[]PhaseTeardownOption so the comment reads "GetTeardownOptions returns options
for tearing down this phase." and ensure the comment stays associated with the
GetTeardownOptions declaration.
---
Nitpick comments:
In `@machinery/types/types.go`:
- Around line 109-120: The current builder methods WithReconcileOptions and
WithTeardownOptions overwrite existing slices (p.ReconcileOptions and
p.TeardownOptions) instead of appending, so repeated calls clobber previous
options; to fix, either change each method to append new opts to the existing
slice (e.g., p.ReconcileOptions = append(p.ReconcileOptions, opts...)) and
likewise for p.TeardownOptions, or add new methods
AddReconcileOptions/AddTeardownOptions that perform the append and update the
PhaseBuilder interface and docs to expose the new behavior; update comments on
With* to document that they replace options if you keep the current behavior.
Comment on lines
+63
to
+75
| // NewPhaseWithOwner creates a new PhaseBuilder with the given name, objects and owner. | ||
| func NewPhaseWithOwner( | ||
| name string, objects []client.Object, | ||
| owner client.Object, ownerStrat ownerStrategy, | ||
| ) PhaseBuilder { | ||
| oo := WithOwner(owner, ownerStrat) | ||
| p := &phase{ | ||
| Name: name, | ||
| Objects: objects, | ||
| } | ||
|
|
||
| return p.WithReconcileOptions(oo).WithTeardownOptions(oo) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find ownerStrategy type definition and WithOwner function
echo "=== ownerStrategy type definition ==="
rg -n "type ownerStrategy" --type go
echo -e "\n=== WithOwner function definition ==="
rg -n "func WithOwner" --type go -A 5
echo -e "\n=== All ownerStrategy references in types.go ==="
rg -n "ownerStrategy" machinery/types/types.go
echo -e "\n=== Check for exported OwnerStrategy type ==="
rg -n "type OwnerStrategy" --type goRepository: package-operator/boxcutter
Length of output: 993
🏁 Script executed:
#!/bin/bash
# Check the exported OwnerStrategy interface in boxcutter.go
echo "=== Exported OwnerStrategy in boxcutter.go ==="
sed -n '115,130p' boxcutter.go
# Check the unexported ownerStrategy interface in options.go
echo -e "\n=== Unexported ownerStrategy in machinery/types/options.go ==="
sed -n '373,385p' machinery/types/options.go
# Check if there's a conversion or relationship between them
echo -e "\n=== Check package of ownerStrategy definitions ==="
head -20 machinery/types/options.go | grep "^package"
head -20 boxcutter.go | grep "^package"
# Search for how external code is supposed to use NewPhaseWithOwner
echo -e "\n=== Check for usage examples or tests of NewPhaseWithOwner ==="
rg -n "NewPhaseWithOwner" --type go -B 2 -A 2 | head -40Repository: package-operator/boxcutter
Length of output: 2704
Use exported OwnerStrategy interface instead of unexported ownerStrategy in function signature.
The NewPhaseWithOwner function is exported (via boxcutter.go), but its ownerStrat parameter accepts the unexported ownerStrategy interface from machinery/types/options.go. An exported OwnerStrategy interface with identical method signatures already exists in the boxcutter package. External consumers cannot call this public function because they cannot create values of the unexported ownerStrategy type. The function signature should accept the exported OwnerStrategy type to make the API usable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@machinery/types/types.go` around lines 63 - 75, The exported function
NewPhaseWithOwner currently accepts the unexported ownerStrategy type which
prevents external callers from passing a strategy; change the parameter type
from ownerStrategy to the exported OwnerStrategy interface (use the
OwnerStrategy defined in the boxcutter package) and update any references where
ownerStrat is passed (e.g. the call to WithOwner(owner, ownerStrat)) so
WithOwner/WithReconcileOptions/WithTeardownOptions receive the exported
OwnerStrategy; keep the rest of the function (creation of phase and chaining of
WithReconcileOptions/WithTeardownOptions) unchanged and ensure imports include
the package that defines OwnerStrategy.
There was a problem hiding this comment.
@erdii, thank you for confirming! Let me know if you'd like any assistance with the refactoring.
mdbooth
reviewed
Feb 17, 2026
There was a problem hiding this comment.
I think there are some bugs to work out in the PR, but I very much like this approach. I haven't reviewed the new tests, but I wonder if the cases I've highlighted can be covered somewhere?
I think this would work for what I want to do in the CAPI operator. Thanks for doing this 🙏
| Name string | ||
| // Owner object will be added as OwnerReference | ||
| // to all objects managed by this revision. | ||
| Owner client.Object |
| } | ||
|
|
||
| // GetOwner returns the owning object. | ||
| func (r *Revision) GetOwner() client.Object { |
machinery/objects.go
Outdated
Comment on lines
429
to
436
| options.OwnerStrategy.CopyOwnerReferences(actualObject, desiredObject) | ||
| options.OwnerStrategy.ReleaseController(desiredObject) | ||
|
|
||
| if err := e.ownerStrategy.SetControllerReference( | ||
| owner, desiredObject, | ||
| if err := options.OwnerStrategy.SetControllerReference( | ||
| options.Owner, desiredObject, | ||
| ); err != nil { | ||
| return nil, fmt.Errorf("set controller reference: %w", err) | ||
| } |
There was a problem hiding this comment.
Needs a guard (everything except setObjectRevision)
machinery/objects.go
Outdated
| err error | ||
| ) | ||
| if options.Owner == nil { | ||
| ctrlSit = ctrlSituationIsController |
There was a problem hiding this comment.
I think this is not quite right because we lose conflict detection with unmanaged objects.
How about if we inspect isBoxcutterManager() and return ctrlSituationUnknownController if it's not? Or the revision annotation.
machinery/objects.go
Outdated
| // Remove us from owners list: | ||
| patch := actualObject.DeepCopyObject().(Object) | ||
| e.ownerStrategy.RemoveOwner(owner, patch) | ||
| var ctrlSit ctrlSituation |
There was a problem hiding this comment.
nit: I don't think ctrlSituation is used outside of the block below. You could move it in there so the reader doesn't have to check for use of an uninitialised value.
machinery/objects.go
Outdated
Comment on lines
143
to
152
| if options.Owner != nil { | ||
| ctrlSit, _ = e.detectOwner(options.Owner, options.OwnerStrategy, actualObject, nil) | ||
| if actualRevision != revision || ctrlSit != ctrlSituationIsController { | ||
| // Remove us from owners list: | ||
| patch := actualObject.DeepCopyObject().(Object) | ||
| options.OwnerStrategy.RemoveOwner(options.Owner, patch) | ||
|
|
||
| return true, e.writer.Patch(ctx, patch, client.MergeFrom(actualObject)) | ||
| return true, e.writer.Patch(ctx, patch, client.MergeFrom(actualObject)) | ||
| } | ||
| } |
There was a problem hiding this comment.
I think this is wrong. I think we still need the revision check even if we don't have an owner, otherwise we're going to delete objects which are still in use by a newer revision. Is the owner check actually doing anything for us, here? Could we simplify this to just the revision index check:
Suggested change
| if options.Owner != nil { | |
| ctrlSit, _ = e.detectOwner(options.Owner, options.OwnerStrategy, actualObject, nil) | |
| if actualRevision != revision || ctrlSit != ctrlSituationIsController { | |
| // Remove us from owners list: | |
| patch := actualObject.DeepCopyObject().(Object) | |
| options.OwnerStrategy.RemoveOwner(options.Owner, patch) | |
| return true, e.writer.Patch(ctx, patch, client.MergeFrom(actualObject)) | |
| return true, e.writer.Patch(ctx, patch, client.MergeFrom(actualObject)) | |
| } | |
| } | |
| // Object is not owned by this revision | |
| if actualRevision != revision { | |
| if options.Owner == nil { | |
| return true, nil | |
| } else { | |
| // Remove us from owners list: | |
| patch := actualObject.DeepCopyObject().(Object) | |
| options.OwnerStrategy.RemoveOwner(options.Owner, patch) | |
| return true, e.writer.Patch(ctx, patch, client.MergeFrom(actualObject)) | |
| } | |
| } |
machinery/objects.go
Outdated
Comment on lines
101
to
112
| if options.Owner != nil { | ||
| // Shortcut when Owner is orphaning its dependents. | ||
| // If we don't check this, we might be too quick and start deleting | ||
| // dependents that should be kept on the cluster! | ||
| if controllerutil.ContainsFinalizer(options.Owner, "orphan") || options.Orphan { | ||
| err := removeBoxcutterManagedLabel(ctx, e.writer, desiredObject.(*unstructured.Unstructured)) | ||
| if err != nil { | ||
| return false, err | ||
| } | ||
|
|
||
| // Shortcut when Owner is orphaning its dependents. | ||
| // If we don't check this, we might be too quick and start deleting | ||
| // dependents that should be kept on the cluster! | ||
| if controllerutil.ContainsFinalizer(owner, "orphan") || options.Orphan { | ||
| err := removeBoxcutterManagedLabel(ctx, e.writer, desiredObject.(*unstructured.Unstructured)) | ||
| if err != nil { | ||
| return false, err | ||
| return true, nil | ||
| } |
There was a problem hiding this comment.
We still need to be able to orphan objects when not using an Owner.
There was a problem hiding this comment.
I don't want to lose track of this one, btw. We have a specific use case in our installer where another actor can designate a CRD 'unmanaged'. If a CRD is added to the unmanaged set we will create a new revision which excludes it. However, we MUST NOT delete unmanaged CRDs, even though they are no longer in the current revision.
I'm anticipating that when calling Teardown we will pass a WithOrphan() option specific to each individual unmanaged CRD object.
mdbooth
reviewed
Feb 18, 2026
mdbooth
left a comment
mdbooth
left a comment
There was a problem hiding this comment.
Thanks. I'll look into writing some test cases.
machinery/objects.go
Outdated
Comment on lines
101
to
112
| if options.Owner != nil { | ||
| // Shortcut when Owner is orphaning its dependents. | ||
| // If we don't check this, we might be too quick and start deleting | ||
| // dependents that should be kept on the cluster! | ||
| if controllerutil.ContainsFinalizer(options.Owner, "orphan") || options.Orphan { | ||
| err := removeBoxcutterManagedLabel(ctx, e.writer, desiredObject.(*unstructured.Unstructured)) | ||
| if err != nil { | ||
| return false, err | ||
| } | ||
|
|
||
| // Shortcut when Owner is orphaning its dependents. | ||
| // If we don't check this, we might be too quick and start deleting | ||
| // dependents that should be kept on the cluster! | ||
| if controllerutil.ContainsFinalizer(owner, "orphan") || options.Orphan { | ||
| err := removeBoxcutterManagedLabel(ctx, e.writer, desiredObject.(*unstructured.Unstructured)) | ||
| if err != nil { | ||
| return false, err | ||
| return true, nil | ||
| } |
There was a problem hiding this comment.
I don't want to lose track of this one, btw. We have a specific use case in our installer where another actor can designate a CRD 'unmanaged'. If a CRD is added to the unmanaged set we will create a new revision which excludes it. However, we MUST NOT delete unmanaged CRDs, even though they are no longer in the current revision.
I'm anticipating that when calling Teardown we will pass a WithOrphan() option specific to each individual unmanaged CRD object.
machinery/objects.go
Outdated
| options.OwnerStrategy.CopyOwnerReferences(actualObject, desiredObject) | ||
| options.OwnerStrategy.ReleaseController(desiredObject) | ||
| if options.Owner != nil { | ||
| e.setObjectRevision(desiredObject, revision) |
There was a problem hiding this comment.
setObjectRevision() needs to move outside the guard.
Improves test coverage from 84.1% to 86.9% by adding tests for: - Reconcile without owner (boxcutter-managed and unmanaged cases) - Paused mode behavior (create/apply skipped, recovery scenarios) - CollisionProtectionNone edge cases - Error handling paths (cache errors, revision parsing, migration) - Teardown scenarios (NoMatchError, orphan finalizers) Notable coverage improvements: - checkSituation: 33.3% → 86.7% - objectUpdateHandling: 82.9% → 90.2% - Teardown: 82.5% → 87.5% - Reconcile: 80.6% → 83.9% 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
erdii
reviewed
Feb 19, 2026
| opt.ApplyToObjectReconcileOptions(&objectOptions) | ||
| } | ||
|
|
||
| return objectOptions.Owner |
Member
There was a problem hiding this comment.
While I support the general API changes of this PR, that's a very ineffective way to get the owner.
I don't have anything better in mind for now though.
erdii
reviewed
Feb 19, 2026
Comment on lines
+128
to
+130
| if opts.Owner != nil && opts.OwnerStrategy == nil { | ||
| panic("Owner without ownerStrategy set") | ||
| } |
Member
There was a problem hiding this comment.
Since Owner and OwnerStrategy belong together, they should be colocated in a new struct (eg type OwnerAndStrategy struct{} under a shared key of opts.
* Consolidates Reconcile and Teardown tests into table-driven TestObjectEngine and TestObjectEngine_Teardown where possible * Moves to a builder pattern for test fixtures * Executes all Reconcile and Teardown tests in both 'owner' and 'without owner' modes where this is meaningful In general, tests where: * Ownership is not relevant * differentiate only 'managed' vs 'no managed' run in both modes. This is most tests. Tests which exercise the concept of having a different (as opposed to no) controller run only in 'owner' mode, because they have no equivalent. This is specified via the optional 'modes' field on the tests. Notable coverage improvements: - Teardown: 87.2% → 94.9% No coverage has been lost.
This should return NoController rathern than UnknownController.
Refactor Reconcile tests to run with and without owner
perdasilva
reviewed
Feb 25, 2026
|
|
||
| // Preflight | ||
| err := e.phaseValidator.Validate(ctx, owner, phase) | ||
| err := e.phaseValidator.Validate(ctx, phase, opts...) |
Contributor
There was a problem hiding this comment.
Do we want just opts here or opts + phase.GetReconciliationOptions() ?
perdasilva
reviewed
Feb 25, 2026
| Objects: objects, | ||
| } | ||
|
|
||
| return p.WithReconcileOptions(oo).WithTeardownOptions(oo) |
Contributor
There was a problem hiding this comment.
We should be careful here, probably. If I do:
p := NewPhaseWithOwner(...).WithReconcileOptions(...)
We might override the WithOwner options, since:
func (p *phase) WithReconcileOptions(opts ...PhaseReconcileOption) PhaseBuilder {
p.ReconcileOptions = opts
return p
}
right?
perdasilva
reviewed
Feb 25, 2026
| func (opts *ObjectReconcileOptions) Default() { | ||
| if opts.Owner != nil && opts.OwnerStrategy == nil { | ||
| panic("Owner without ownerStrategy set") | ||
| } |
Contributor
There was a problem hiding this comment.
Also, should we do the same check in ObjectTeardownOptions Default()?
perdasilva
reviewed
Feb 25, 2026
perdasilva
reviewed
Feb 25, 2026
perdasilva
reviewed
Feb 25, 2026
| ObjectReconcileOption | ||
| ObjectTeardownOption | ||
| } { | ||
| if len(obj.GetUID()) == 0 { |
Contributor
There was a problem hiding this comment.
is it worth doing a nil check on obj?
perdasilva
reviewed
Feb 25, 2026
|
|
||
| func (e *ObjectEngine) isBoxcutterManaged(obj client.Object) bool { | ||
| for k := range obj.GetLabels() { | ||
| if strings.HasPrefix(k, boxcutterManagedLabel) { |
Contributor
There was a problem hiding this comment.
is HasPrefix the right choice here? We could be matching things we don't mean to maybe a full comparison?
perdasilva
reviewed
Feb 25, 2026
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
4 participants
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.
Make Owner Optional in Boxcutter Machinery
Summary
ObjectEngine,PhaseEngine,RevisionEngine,Comparator, and validation)WithOwner(obj, strategy)option that can be attached at the revision, phase, or object level, replacing the hard-coded owner parameterPhaseandRevisionfrom concrete structs to interfaces with builder constructors (NewPhase,NewPhaseWithOwner,NewRevision,NewRevisionWithOwner)Motivation
The boxcutter library previously required an owner object (a Kubernetes resource with a UID) at every level of the API. The owner was used to set
ownerReferenceson managed objects for garbage collection, detect controller ownership conflicts, and enforce namespace restrictions during validation.This hard requirement prevented the library from being used in scenarios where no owner object exists, such as:
API Changes
New Types and Constructors
NewPhase(name, objects)NewPhaseWithOwner(name, objects, owner, strategy)NewRevision(name, rev, phases)NewRevisionWithOwner(name, rev, phases, owner, strategy)WithOwner(obj, strategy)ComparatorOption,ObjectReconcileOption, andObjectTeardownOptionPhaseBuilder/RevisionBuilderWithReconcileOptions()andWithTeardownOptions()methodsSignature Changes
NewComparator(ownerStrategy, discoveryClient, scheme, fieldOwner)(discoveryClient, scheme, fieldOwner)Comparator.Compare(owner, desired, actual)(desired, actual, opts...)NewObjectEngine(ownerStrategy, ...)(...)— owner strategy removedObjectEngine.Reconcile(ctx, owner, revision, obj, opts...)(ctx, revision, obj, opts...)ObjectEngine.Teardown(ctx, owner, revision, obj, opts...)(ctx, revision, obj, opts...)PhaseEngine.Reconcile/Teardown(ctx, owner, phase, opts...)(ctx, phase, opts...)ObjectValidator.Validate(ctx, owner, obj)(ctx, obj, opts...)PhaseValidator.Validate(ctx, owner, phase)(ctx, phase, opts...)Other Breaking Changes
PhaseandRevisionare now interfaces, not structs — use theNew*constructors instead of struct literalsPhase.Objectschanged from[]unstructured.Unstructuredto[]client.ObjectRevision.Ownerfield andGetOwner()method removed — useWithOwneroption orRevisionReconcileOptions.GetOwner()insteadRevisionEngineOptions.OwnerStrategyfield removed — owner strategy is now provided per-revision/phase viaWithOwnerInternal Machinery Changes
ObjectEngine: Owner-related logic (setting controller references, detecting ownership, handling handover) is now conditional onoptions.Owner != nil. A newcheckSituationmethod consolidates controller-situation detection: when no owner is present, theboxcutter-managedlabel is used to determine control status instead ofownerReferences.PhaseEngine/RevisionEngine: Options are now merged from both explicit parameters andphase.GetReconcileOptions()/rev.GetReconcileOptions(), allowing owner to flow through the option hierarchy.Comparator: Owner reference setting ondesiredObjectis now conditional. TheownerStrategyfield was removed from the struct; it is passed in viaComparatorOptionwhen needed.client.Objectinstead of*unstructured.Unstructured.Bug Fixes
controllerutil.ContainsFinalizer(owner, "orphan")— now short-circuits onoptions.OrphanfirstcheckSituationmethod that handles both owner and no-owner cases. Fixed teardown revision-mismatch logic to returntrue(gone) when there is no owner instead of attempting to remove a non-existent owner reference"owner must be persistet"to"owner must be persisted"Test Changes
ObjectEnginetests using anownerModeabstraction with two modes (withNativeOwnerModeandwithoutOwnerMode), so most tests run in both modes by defaultbuildObj,objBuilder) for constructing test objectsRevisionEnginetests covering success, waiting on incomplete phases, validation errors, and option forwardingWithOwner,WithOrphan,WithTeardownWriter) and builder methodsMigration Guide
The reference implementation in
cmd/reference/internal/serves as a migration example:ownerStrategyon your reconciler struct instead of passing it to engine constructorsNewRevisionWithOwner(name, rev, phases, owner, strategy)orNewRevision(name, rev, phases)instead ofRevision{...}struct literalsNewPhase(name, objects)instead ofPhase{...}struct literalsownerfrom allReconcile/TeardowncallsRevisionReconcileOptions.GetOwner()Test plan
🤖 Generated with Claude Code