apis: Eliminate imports of internal packages in apis#7002
Conversation
📝 WalkthroughWalkthroughRemoves public accessor implementations for Usage/ClusterUsage and tests, introduces internal wrapper types implementing a new internal Usage interface with Unwrap(), migrates DAG node types into an internal dag package, updates reconcilers to accept a usage factory and operate on unwrapped objects, and redirects codegen targets to internal/protection. Changes
Sequence Diagram(s)sequenceDiagram
participant Recon as Reconciler
participant Factory as usage factory (func() protection.Usage)
participant Usage as protection.Usage (wrapper)
participant Obj as Unwrapped API object
participant DAG as internal.dag
participant K8s as Kubernetes API (client)
Recon->>Factory: r.newUsage()
Factory-->>Recon: Usage
Recon->>Usage: Unwrap()
Usage-->>Recon: Obj
Recon->>DAG: build graph (PackagesToNodes)
DAG-->>Recon: nodes
Recon->>K8s: Get/Update/Status.Update using Obj
K8s-->>Recon: response / updated object
Recon->>Recon: record events / set owners using Obj metadata
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Would you like me to highlight specific spots in the reconciler or DAG changes that typically need the most scrutiny (concurrency, ownership, or conversion edge cases)? Thank you for the contribution. Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (3 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/webhook/protection/usage/handler.go (1)
163-182: Good UX: message now reports the underlying usage type viaUnwrap(); please harden against nils to prevent webhook panics.If
Unwrap()(orby.ResourceRef) could ever be nil in practice, this admission path would panic. I’d suggest a tiny defensive fallback since this is user-facing admission logic.Proposed hardening
func inUseMessage(u []protection.Usage) string { first := u[0] by := first.GetUsedBy() + uw := first.Unwrap() + if uw == nil { + uw = first + } + id := fmt.Sprintf("%q", first.GetName()) if first.GetNamespace() != "" { id = fmt.Sprintf("%q (in namespace %q)", first.GetName(), first.GetNamespace()) } if by != nil { - return fmt.Sprintf("This resource is in-use by %d usage(s), including the %T %s by resource %s/%s.", len(u), first.Unwrap(), id, by.Kind, by.ResourceRef.Name) + name := "<unknown>" + if by.ResourceRef != nil && by.ResourceRef.Name != "" { + name = by.ResourceRef.Name + } + return fmt.Sprintf("This resource is in-use by %d usage(s), including the %T %s by resource %s/%s.", len(u), uw, id, by.Kind, name) } if r := ptr.Deref(first.GetReason(), ""); r != "" { - return fmt.Sprintf("This resource is in-use by %d usage(s), including the %T %s with reason: %q.", len(u), first.Unwrap(), id, r) + return fmt.Sprintf("This resource is in-use by %d usage(s), including the %T %s with reason: %q.", len(u), uw, id, r) } // Either spec.by or spec.reason should be set, which we enforce with a CEL // rule. This is just a fallback. - return fmt.Sprintf("This resource is in-use by %d usage(s), including the %T %s.", len(u), first.Unwrap(), id) + return fmt.Sprintf("This resource is in-use by %d usage(s), including the %T %s.", len(u), uw, id) }As per coding guidelines, admission/controller messages should remain actionable and resilient.
internal/controller/pkg/resolver/reconciler.go (1)
308-316: Potential issue:dep.Identifier()is called before validating the type assertion.Line 310 calls
dep.Identifier()but theokcheck for the type assertion doesn't happen until line 311. Ifimplied[0]is not a*controller.DependencyNode,depwould beniland callingdep.Identifier()would panic.Could you consider reordering to check
okbefore usingdep?🐛 Proposed fix
// If we are missing a node, we want to create it. The resolver never // modifies the Lock. We only create the first implied node as we will // be requeued when it adds itself to the Lock, at which point we will // check for missing nodes again. dep, ok := implied[0].(*controller.DependencyNode) - - depID := dep.Identifier() if !ok { - log.Debug(errInvalidDependency, "error", errors.Errorf(errFmtMissingDependency, depID)) - status.MarkConditions(v1beta1.ResolutionFailed(errors.Errorf(errFmtMissingDependency, depID))) + log.Debug(errInvalidDependency, "error", errors.New("implied node is not a dependency")) + status.MarkConditions(v1beta1.ResolutionFailed(errors.New(errInvalidDependency))) return reconcile.Result{}, errors.Wrap(r.client.Status().Update(ctx, lock), errCannotUpdateStatus) } + + depID := dep.Identifier()
🧹 Nitpick comments (3)
internal/protection/usage.go (1)
95-108: Nice:Unwrap()makes the wrapper pattern explicit and keeps client calls honest.Small suggestion: consider documenting the contract a bit more strongly (e.g., “must return the underlying API object that reflects any mutations made through this interface, and must be non-nil”) to prevent subtle wrapper bugs later.
internal/controller/protection/usage/selector.go (1)
52-66: Good fix: update the underlying API object (u.Unwrap()) instead of the wrapper.One thing to sanity-check: do the wrapper setters (
SetUserOf,SetUsedBy, etc.) mutate the same object returned byUnwrap()? If not, this would silently drop resolved selector writes.Optional: if you expect both selectors to commonly resolve in one pass, you could coalesce to a single
Updateat the end to save an API call.Also applies to: 72-82
internal/protection/wrappers.go (1)
29-258: Consider reducing duplication across wrapper types.The three wrapper implementations share significant structural similarity. While the current approach is clear and explicit, you might consider whether a generic approach or embedding shared behavior could reduce maintenance burden in the future.
That said, the explicit implementations are easy to understand and the differences in converter usage are clear. This is more of a thought for future iterations than a blocking concern. What are your thoughts on this trade-off?
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
apis/apiextensions/v1beta1/zz_generated.deepcopy.gois excluded by!**/zz_generated*.goand included by**/*.goapis/protection/v1beta1/zz_generated.deepcopy.gois excluded by!**/zz_generated*.goand included by**/*.gointernal/protection/zz_generated.conversion.gois excluded by!**/zz_generated*.goand included by**/*.gointernal/protection/zz_generated.conversion_legacy.gois excluded by!**/zz_generated*.goand included by**/*.go
📒 Files selected for processing (26)
apis/apiextensions/v1beta1/usage_interface.goapis/apiextensions/v1beta1/usage_interface_test.goapis/pkg/v1beta1/lock.goapis/protection/v1beta1/clusterusage_interface.goapis/protection/v1beta1/usage_interface.goapis/protection/v1beta1/usage_interface_test.gogenerate.gointernal/controller/pkg/controller/dag.gointernal/controller/pkg/resolver/reconciler.gointernal/controller/pkg/resolver/reconciler_test.gointernal/controller/pkg/revision/dependency.gointernal/controller/pkg/revision/dependency_test.gointernal/controller/pkg/revision/fuzz_test.gointernal/controller/protection/usage/reconciler.gointernal/controller/protection/usage/reconciler_test.gointernal/controller/protection/usage/selector.gointernal/controller/protection/usage/selector_test.gointernal/protection/conversion.gointernal/protection/conversion_legacy.gointernal/protection/usage.gointernal/protection/usage/finder.gointernal/protection/usage/finder_test.gointernal/protection/wrappers.gointernal/protection/wrappers_test.gointernal/webhook/protection/usage/handler.gointernal/webhook/protection/usage/handler_test.go
💤 Files with no reviewable changes (6)
- apis/protection/v1beta1/usage_interface_test.go
- apis/protection/v1beta1/usage_interface.go
- apis/apiextensions/v1beta1/usage_interface_test.go
- apis/pkg/v1beta1/lock.go
- apis/protection/v1beta1/clusterusage_interface.go
- apis/apiextensions/v1beta1/usage_interface.go
🧰 Additional context used
📓 Path-based instructions (3)
**/*.go
⚙️ CodeRabbit configuration file
**/*.go: Enforce Crossplane-specific patterns: Use crossplane-runtime/pkg/errors
for wrapping. Check variable naming (short for local scope, descriptive
for wider scope). Ensure 'return early' pattern. Verify error scoping
(declare in conditionals when possible). For nolint directives, require
specific linter names and explanations. CRITICAL: Ensure all error
messages are meaningful to end users, not just developers - avoid
technical jargon, include context about what the user was trying to do,
and suggest next steps when possible.
Files:
internal/controller/protection/usage/selector.gointernal/protection/usage/finder_test.gointernal/webhook/protection/usage/handler.gointernal/controller/pkg/resolver/reconciler_test.gointernal/protection/usage.gointernal/controller/protection/usage/reconciler_test.gointernal/protection/conversion.gointernal/controller/pkg/controller/dag.gointernal/controller/protection/usage/reconciler.gointernal/controller/pkg/revision/dependency.gointernal/webhook/protection/usage/handler_test.gointernal/controller/pkg/resolver/reconciler.gointernal/controller/pkg/revision/dependency_test.gointernal/protection/wrappers.gointernal/controller/protection/usage/selector_test.gointernal/protection/conversion_legacy.gogenerate.gointernal/controller/pkg/revision/fuzz_test.gointernal/protection/usage/finder.gointernal/protection/wrappers_test.go
**/internal/controller/**
⚙️ CodeRabbit configuration file
**/internal/controller/**: Review controller logic for proper reconciliation patterns, error
handling, and resource management. Pay special attention to conditions
and events: Conditions must be actionable for users (not developers),
stable/deterministic, with proper Type/Reason/Message format. Events
only when something actually happens, with specific details about what
changed. No transient errors in conditions/events. All error messages
must be meaningful to end users - include context about what
resource/operation failed and why.
Files:
internal/controller/protection/usage/selector.gointernal/controller/pkg/resolver/reconciler_test.gointernal/controller/protection/usage/reconciler_test.gointernal/controller/pkg/controller/dag.gointernal/controller/protection/usage/reconciler.gointernal/controller/pkg/revision/dependency.gointernal/controller/pkg/resolver/reconciler.gointernal/controller/pkg/revision/dependency_test.gointernal/controller/protection/usage/selector_test.gointernal/controller/pkg/revision/fuzz_test.go
**/*_test.go
⚙️ CodeRabbit configuration file
**/*_test.go: Enforce table-driven test structure: PascalCase test names (no
underscores), args/want pattern, use cmp.Diff with
cmpopts.EquateErrors() for error testing. Check for proper test case
naming and reason fields. Ensure no third-party test frameworks (no
Ginkgo, Gomega, Testify).
Files:
internal/protection/usage/finder_test.gointernal/controller/pkg/resolver/reconciler_test.gointernal/controller/protection/usage/reconciler_test.gointernal/webhook/protection/usage/handler_test.gointernal/controller/pkg/revision/dependency_test.gointernal/controller/protection/usage/selector_test.gointernal/controller/pkg/revision/fuzz_test.gointernal/protection/wrappers_test.go
🧠 Learnings (9)
📚 Learning: 2025-11-19T19:05:29.846Z
Learnt from: negz
Repo: crossplane/crossplane PR: 6934
File: internal/controller/apiextensions/managed/reconciler.go:115-115
Timestamp: 2025-11-19T19:05:29.846Z
Learning: In Crossplane controllers, status update errors in error paths are typically ignored silently using `_ = r.client.Status().Update(...)` without logging. This is the standard pattern and should not be flagged as an issue.
Applied to files:
internal/controller/protection/usage/selector.go
📚 Learning: 2025-12-02T14:41:54.485Z
Learnt from: CR
Repo: crossplane/crossplane PR: 0
File: contributing/README.md:0-0
Timestamp: 2025-12-02T14:41:54.485Z
Learning: Applies to contributing/**/*_test.go : Use https://github.com/google/go-cmp/ for comparisons in tests to keep test output consistent
Applied to files:
internal/protection/usage/finder_test.go
📚 Learning: 2025-12-02T14:41:54.485Z
Learnt from: CR
Repo: crossplane/crossplane PR: 0
File: contributing/README.md:0-0
Timestamp: 2025-12-02T14:41:54.485Z
Learning: Applies to contributing/**/*_test.go : Crossplane does not use third-party test libraries (e.g., Ginkgo, Gomega, Testify) for unit tests; use only the standard Go testing library
Applied to files:
internal/controller/pkg/resolver/reconciler_test.gointernal/controller/protection/usage/selector_test.go
📚 Learning: 2025-12-02T14:41:54.485Z
Learnt from: CR
Repo: crossplane/crossplane PR: 0
File: contributing/README.md:0-0
Timestamp: 2025-12-02T14:41:54.485Z
Learning: Applies to contributing/**/*_test.go : Use table-driven tests instead of test frameworks like Ginkgo; structure tests with 'cases' map containing test cases with 'reason', 'args', and 'want' fields
Applied to files:
internal/webhook/protection/usage/handler_test.go
📚 Learning: 2025-12-02T14:41:54.485Z
Learnt from: CR
Repo: crossplane/crossplane PR: 0
File: contributing/README.md:0-0
Timestamp: 2025-12-02T14:41:54.485Z
Learning: Applies to contributing/**/*.go : Use crossplane-runtime/pkg/errors to wrap errors with context rather than using error constants
Applied to files:
internal/controller/protection/usage/selector_test.go
📚 Learning: 2025-10-10T19:15:32.438Z
Learnt from: negz
Repo: crossplane/crossplane PR: 6844
File: cluster/crds/pkg.crossplane.io_deploymentruntimeconfigs.yaml:843-845
Timestamp: 2025-10-10T19:15:32.438Z
Learning: CRDs under cluster/crds/*.yaml are generated from upstream Kubernetes API types via controller-gen (see controller-gen.kubebuilder.io/version annotation). Descriptive text in these files mirrors upstream and should not be edited locally; fixes must be made upstream and pulled in via k8s.io module updates.
Applied to files:
generate.go
📚 Learning: 2025-12-02T14:41:54.485Z
Learnt from: CR
Repo: crossplane/crossplane PR: 0
File: contributing/README.md:0-0
Timestamp: 2025-12-02T14:41:54.485Z
Learning: Applies to contributing/**/*.go : Follow Crossplane's Observability Developer Guide
Applied to files:
generate.go
📚 Learning: 2025-09-09T20:15:06.308Z
Learnt from: negz
Repo: crossplane/crossplane PR: 6791
File: .coderabbit.yaml:79-103
Timestamp: 2025-09-09T20:15:06.308Z
Learning: The Crossplane project uses `zz_generated` as their standard naming convention for generated files, rather than broader patterns like `*.gen.go` or `*generated*.go`. The current path filters in `.coderabbit.yaml` with `!**/zz_generated*.go` correctly exclude their generated code.
Applied to files:
generate.go
📚 Learning: 2025-12-02T14:41:54.485Z
Learnt from: CR
Repo: crossplane/crossplane PR: 0
File: contributing/README.md:0-0
Timestamp: 2025-12-02T14:41:54.485Z
Learning: Applies to contributing/**/*.go : Preempt common Go code review comments and test review comments as documented in the Go project's guidelines
Applied to files:
generate.go
🧬 Code graph analysis (14)
internal/webhook/protection/usage/handler.go (3)
internal/protection/usage.go (1)
ResourceRef(30-38)apis/protection/v1beta1/usage_types.go (1)
ResourceRef(27-30)apis/apiextensions/v1beta1/usage_types.go (1)
ResourceRef(26-29)
internal/controller/pkg/resolver/reconciler_test.go (2)
internal/controller/pkg/controller/dag.go (1)
DependencyNode(30-32)apis/pkg/v1beta1/lock.go (3)
Dependency(89-113)ConfigurationPackageType(30-30)ProviderPackageType(31-31)
internal/protection/usage.go (1)
internal/protection/usage/finder.go (1)
Object(107-111)
internal/controller/protection/usage/reconciler_test.go (4)
internal/protection/usage.go (1)
Usage(96-108)apis/protection/v1beta1/usage_types.go (1)
Usage(162-168)apis/apiextensions/v1beta1/usage_types.go (1)
Usage(99-106)internal/protection/wrappers.go (1)
UsageWrapper(30-32)
internal/protection/conversion.go (2)
internal/protection/usage.go (3)
ResourceRef(30-38)ResourceSelector(41-53)Resource(56-69)apis/protection/v1beta1/usage_types.go (1)
NamespacedResource(100-118)
internal/webhook/protection/usage/handler_test.go (2)
internal/protection/wrappers.go (1)
UsageWrapper(30-32)apis/protection/v1beta1/usage_types.go (6)
Usage(162-168)UsageSpec(123-140)NamespacedResource(100-118)ResourceRef(27-30)NamespacedResourceRef(68-75)Resource(45-63)
internal/controller/pkg/resolver/reconciler.go (3)
internal/controller/pkg/controller/dag.go (2)
PackagesToNodes(80-87)DependencyNode(30-32)apis/pkg/v1beta1/lock.go (1)
Dependency(89-113)apis/pkg/meta/v1/meta.go (1)
Dependency(44-80)
internal/controller/pkg/revision/dependency_test.go (1)
internal/controller/pkg/controller/dag.go (2)
DependencyNode(30-32)PackageNode(50-52)
internal/protection/wrappers.go (3)
internal/protection/usage.go (2)
Usage(96-108)Resource(56-69)internal/protection/usage/finder.go (1)
Object(107-111)apis/protection/v1beta1/clusterusage_types.go (1)
ClusterUsage(40-46)
internal/controller/protection/usage/selector_test.go (2)
internal/protection/usage.go (4)
Usage(96-108)ResourceRef(30-38)ResourceSelector(41-53)Resource(56-69)internal/protection/wrappers.go (1)
UsageWrapper(30-32)
internal/protection/conversion_legacy.go (1)
internal/protection/usage.go (3)
ResourceRef(30-38)ResourceSelector(41-53)Resource(56-69)
internal/controller/pkg/revision/fuzz_test.go (1)
internal/controller/pkg/controller/dag.go (1)
PackageNode(50-52)
internal/protection/usage/finder.go (3)
internal/protection/wrappers.go (3)
UsageWrapper(30-32)ClusterUsageWrapper(107-109)LegacyUsageWrapper(184-186)internal/protection/usage.go (1)
Usage(96-108)apis/protection/v1beta1/clusterusage_types.go (1)
ClusterUsage(40-46)
internal/protection/wrappers_test.go (2)
internal/protection/usage.go (1)
Usage(96-108)internal/protection/wrappers.go (3)
UsageWrapper(30-32)ClusterUsageWrapper(107-109)LegacyUsageWrapper(184-186)
🔍 Remote MCP
I'll search for additional context about this PR and the related changes to help with the review.Let me search for more specific information about the PR objectives and the architectural rationale.
Summary of Additional Context for PR #7002 Review
PR Objectives and Scope
This PR prepares for making the apis package a separate Go module by removing imports of internal packages from apis, since the Go convention recommends placing such packages into a directory named internal to prevent other modules from depending on packages not intended for external use, and internal packages are free to refactor without breaking external users.
The PR accomplishes two major refactorings:
- DAG Logic Relocation: Moves DAG (Directed Acyclic Graph) implementations from the public API package to internal/controller/pkg
- Usage Interface Implementations: Moves usage interface implementations into internal/protection via wrapper types
Architectural Context: Internal Packages in Go
Go projects may have exported symbols intended for use only by other packages within the project but not part of the public API. Internal packages provide a mechanism for controlling visibility to such parts that would otherwise be considered part of the public versioned API.
With Go modules and semantic versioning, everything exported by a project in v1 becomes a public API requiring compatibility guarantees. Therefore, it's imperative to export only the minimal API surface essential for users, and all other code needed for implementation should live in internal.
Wrapper Pattern Relevance
The PR uses wrapper types (UsageWrapper, ClusterUsageWrapper, LegacyUsageWrapper, DependencyNode, PackageNode) to adapt API types while maintaining internal implementation details. The Adapter Pattern enables interaction between incompatible interfaces by creating a wrapper object that can translate requests between both interfaces.
While dependency wrapping is a powerful technique for decoupling and making code more modular and maintainable, common pitfalls include over-wrapping, which can lead to complexity and performance issues. Wrapping a dependency involves wrapping a function or struct with another that provides additional functionality without touching the original implementation.
Dependency Resolution in Crossplane Context
Provider and Configuration packages have fields to specify dependencies on other packages, and Crossplane should support resolving and installing dependencies for packages. The DAG implementation in this PR supports enabling Crossplane to automatically upgrade a package's dependency version to the minimum valid version that satisfies all constraints through an alpha feature requiring the --enable-dependency-version-upgrades flag.
[::web_search::]
🔇 Additional comments (47)
internal/webhook/protection/usage/handler_test.go (3)
199-222: LGTM! Wrapper pattern correctly applied.The test case properly constructs
protection.UsageWrapperwrapping thev1beta1.Usagetype, withByand other fields correctly nested underSpec. This aligns with the PR's goal of moving usage interface implementations to internal packages while maintaining API separation.
264-280: LGTM! Reason field properly positioned.The wrapper construction correctly places the
Reasonfield underSpec, matching thev1beta1.UsageSpecstructure. The test expectation also correctly reflects the wrapped usage type in the error message.
322-337: LGTM! Minimal wrapper case handled correctly.The test case properly demonstrates the wrapper pattern even when
ByandReasonare omitted, ensuring the basic usage blocking scenario still works with the new internal wrapper types.generate.go (1)
47-53: Double-check controller-gen/goverter output now that generation targets./internal/protection.This aligns well with “make apis module-standalone”, but can you confirm
go generatedoesn’t accidentally emit (or duplicate) CRDs from the non-versionedinternal/protectionpackage, and that any generated files land where you expect? Thanks for keeping the toolchain updates in one place.Also applies to: 61-66
internal/controller/pkg/revision/fuzz_test.go (1)
33-40: Makes sense: fuzz now exercisescontroller.PackageNodeinstead of rawv1beta1.LockPackage.As long as
*controller.PackageNodesatisfies thedag.Nodeusage in this test path (andGenerateStructpopulates the embeddedLockPackageas expected), this looks like the right alignment with the refactor.Also applies to: 62-76
internal/protection/usage/finder_test.go (1)
158-166: LGTM! Test expectations correctly updated to match wrapper pattern.The test now expects
protection.UsageWrapper,protection.ClusterUsageWrapper, andprotection.LegacyUsageWrappertypes, which aligns with the changes infinder.go. Thanks for ensuring the test expectations stay in sync with the implementation!internal/protection/wrappers_test.go (1)
17-23: LGTM! Compile-time interface assertions look good.Nice use of the blank identifier pattern to verify that all three wrapper types (
UsageWrapper,ClusterUsageWrapper,LegacyUsageWrapper) implement theUsageinterface at compile time. This provides good safety guarantees for the wrapper pattern.internal/controller/protection/usage/reconciler_test.go (2)
67-67: LGTM! Factory function pattern provides good test isolation.Changing from a direct
protection.Usageto a factory functionfunc() protection.Usageensures each test case gets a fresh instance. This is a solid pattern for preventing unintended state sharing between tests. Thanks for this thoughtful change!
86-86: Consistent factory function usage across all test cases.I see the pattern
func() protection.Usage { return &protection.UsageWrapper{} }is applied uniformly across all test cases. This consistency makes the test file easy to maintain.internal/controller/pkg/resolver/reconciler_test.go (3)
46-46: LGTM! Import added for the new controller package.The import of
internal/controller/pkg/controlleris necessary to use the newDependencyNodeandPackageNodewrapper types.
1230-1238: Test cases correctly updated forTestFindDigestToUpdate.The
TestFindDigestToUpdatetest cases now usecontroller.DependencyNodewrapper, which is consistent with the rest of the file. Thanks for maintaining this consistency across all related tests!
348-352: Consistent wrapper pattern for DAG nodes confirmed.The transition from raw
v1beta1.Dependencyto&controller.DependencyNode{Dependency: v1beta1.Dependency{...}}is applied uniformly across all test cases. I've confirmed thatcontroller.DependencyNodeimplements thedag.Nodeinterface through a compile-time assertion at line 25 of dag.go (_ dag.Node = &DependencyNode{}) and the required method implementations (Neighbors()andAddNeighbors()). This aligns well with the PR's goal of moving DAG logic out of the apis package.internal/protection/usage/finder.go (3)
122-124: LGTM! Safe value copy into wrapper struct.The pattern
&protection.UsageWrapper{Usage: u}correctly copies the value ofuinto the wrapper struct, avoiding the classic loop variable capture issue. This is safe because you're copying by value, not taking the address of the loop variable.
131-132: ClusterUsage wrapper follows the same safe pattern.
140-141: LegacyUsage wrapper completes the set.All three usage types are now consistently wrapped before being returned. This aligns nicely with the PR's goal of moving interface implementations into the internal package.
internal/controller/protection/usage/selector_test.go (4)
33-33: LGTM! Import added for protection package.
42-48: Interface type in test args provides flexibility.Changing the test args and want types from concrete
*v1beta1.Usageto theprotection.Usageinterface is a good choice. This allows the tests to work with any wrapper type that implements the interface.
59-120: Test case "AlreadyResolved" correctly uses wrapper pattern.The wrapper pattern
&protection.UsageWrapper{Usage: v1beta1.Usage{...}}is applied consistently to both the input and expected output. This ensures the test validates the selector resolver works correctly with wrapped usage types.
468-548: "BothSelectorsResolved" test case validates the complete flow.This comprehensive test case verifies that both "of" and "by" selectors are resolved correctly when working with wrapped usage types. The nested structure with
OwnerReferencesandMatchControllerRefis preserved, which is great for ensuring the wrapper pattern doesn't break complex scenarios.internal/controller/pkg/revision/dependency.go (5)
40-40: LGTM! Import for controller package with DAG node types.
138-138: DAG initialization now uses controller package helper.The switch from
v1beta1.ToNodestocontroller.PackagesToNodesmoves the node conversion logic into the internal controller package, which is the right place for it. This is a key part of separating the public API from internal implementation concerns.
196-196: Package node wrapped before adding to DAG.The
selfvalue is now wrapped in&controller.PackageNode{LockPackage: self}before being added to the DAG. This maintains the separation between the API type and the DAG node implementation.
208-208: Dependencies wrapped as DependencyNode for missing tracking.
248-256: Struct embedding confirmed—all field and method access is valid.Yes,
controller.PackageNodeembedsv1beta1.LockPackagedirectly (via anonymous field atinternal/controller/pkg/controller/dag.go:50), so both theVersionfield andIdentifier()method are accessible without any additional wrapping. The code correctly leverages this embedding pattern.internal/controller/pkg/revision/dependency_test.go (3)
37-37: LGTM! Import added for the new controller package.The new import aligns with the PR's goal of moving DAG logic to the internal controller package. The wrapper types (
DependencyNode,PackageNode) are now correctly referenced from this package.
285-301: Wrapper types correctly applied in mock DAG initialization.The test now uses
&controller.DependencyNode{Dependency: v1beta1.Dependency{...}}instead of rawv1beta1.Dependency. This matches the new internal DAG node representation.Could you confirm whether there are any edge cases where the mock should return
PackageNodeinstead ofDependencyNodefor theMockInitcallback? The current usage appears correct sinceInitreturns missing/implied dependencies.
386-404: MockGetNode correctly returns PackageNode wrappers.The
MockGetNodefunction now returns&controller.PackageNode{LockPackage: v1beta1.LockPackage{...}}, which is consistent with the new DAG design whereGetNoderetrieves installed packages (not dependencies).This aligns well with the distinction between
DependencyNode(for missing/implied deps) andPackageNode(for installed packages).internal/controller/pkg/resolver/reconciler.go (3)
277-277: Good refactoring to use the new PackagesToNodes converter.The DAG initialization now correctly uses
controller.PackagesToNodes(lock.Packages...)instead of the previousv1beta1.ToNodes(...). This keeps the node conversion logic within the internal controller package as intended.
333-333: Correctly passing the embedded Dependency to NewPackageList.The change from
NewPackageList(dep)toNewPackageList(&dep.Dependency)correctly extracts the embeddedv1beta1.Dependencyfrom theDependencyNodewrapper. This maintains type compatibility with the function signature.
379-379: Consistent use of&dep.Dependencyfor downstream calls.Both
findDependencyVersionToInstallandNewPackagenow receive&dep.Dependency, which is the correct approach for extracting the underlying v1beta1 type from the wrapper. Thanks for maintaining consistency here!Also applies to: 397-397
internal/controller/pkg/controller/dag.go (4)
24-27: Good practice: compile-time interface conformance checks.The
var _ dag.Node = &DependencyNode{}pattern ensures that both wrapper types satisfy thedag.Nodeinterface at compile time rather than runtime. This is a great defensive practice!
29-47: DependencyNode implementation looks correct.The design makes sense:
Neighbors()returnsnilbecause a dependency node doesn't yet know its own dependencies until it's installed.AddNeighbors()propagates the dependency's constraints to the provided nodes.One question: I notice
AddNeighborsalways returnsnil. Is there a scenario where it should return an error? If not, would it be worth adding a brief comment explaining why the error return is always nil?
49-77: PackageNode implementation correctly handles dependency relationships.The implementation properly:
- Returns
DependencyNode-wrapped dependencies fromNeighbors()- Matches dependencies by identifier in
AddNeighbors()before propagating constraintsThe early
breakon line 71 is a nice optimization to avoid unnecessary iterations once a match is found.
79-87: PackagesToNodes helper is clean and efficient.Pre-allocating the slice with
make([]dag.Node, len(pkgs))and using direct index assignment is more efficient than appending. This is well-implemented!internal/protection/conversion_legacy.go (2)
17-27: Legacy converter interface correctly configured for apiextensions v1beta1.The interface and goverter annotations are well-structured:
- Import correctly references the legacy
apiextensions/v1beta1package- Output file path
zz_generated.conversion_legacy.godistinguishes it from the non-legacy converter- Interface name
LegacyResourceConverterclearly indicates its purposeThis cleanly supports backward compatibility with the deprecated Usage type.
29-35: Conversion methods correctly bridge legacy and internal types.The bidirectional conversion (
ToInternal/FromInternal) enables the wrapper pattern used inLegacyUsageWrapper. Thegoverter:ignore Namespacedirectives appropriately handle the structural differences between the API types and internal representation.internal/controller/protection/usage/reconciler.go (4)
105-110: Clean factory pattern for Usage instantiation.The factory function
func() protection.Usage { return &protection.UsageWrapper{} }elegantly allows the reconciler to work with the abstractprotection.Usageinterface while creating the correct concrete type. This pattern supports the three different usage types (Usage, ClusterUsage, LegacyUsage) without code duplication.
218-233: Well-designed NewReconciler signature update.The signature change from accepting a concrete Usage to accepting a factory function
func() protection.Usageis a good design choice. This allows:
- Fresh instances per reconciliation
- Type-agnostic reconciler logic
- Easy testing with mock factories
The
newUsagefield is appropriately named to indicate it's a constructor function.
277-278: Correct usage of factory and Unwrap pattern.Creating the usage via
r.newUsage()and then usingu.Unwrap()for theclient.Getcall is the right approach. The Kubernetes client needs the concrete API type, while the reconciler logic works with the internal interface.
415-415: Consistent Unwrap() usage for client operations.All client operations (
RemoveFinalizer,AddFinalizer,Update,Status().Update) correctly useu.Unwrap()to pass the underlying API object. This consistency is important for correctness.Thank you for ensuring this pattern is applied uniformly throughout the reconciliation logic!
Also applies to: 432-432, 451-451, 519-519, 541-541
internal/protection/conversion.go (2)
17-21: Package and imports correctly configured for internal protection.The package declaration and import path align with the PR's goal of moving conversion logic into the internal protection package. The aliased import
v1beta1 "github.com/crossplane/crossplane/v2/apis/protection/v1beta1"is clear and follows Go conventions.
29-49: Two converters appropriately handle different resource scoping.The separation between
ResourceConverter(for cluster-scopedResource) andNamespacedResourceConverter(forNamespacedResource) is a clean design that matches the Usage API structure:
ClusterUsage.Spec.OfusesResource→ handled byResourceConverterUsage.Spec.OfusesNamespacedResource→ handled byNamespacedResourceConverterThe goverter directive on line 45 correctly targets the internal package for code generation.
internal/protection/wrappers.go (5)
1-1: Build tag correctly excludes file from goverter.The
//go:build !govertertag prevents this file from being processed during goverter code generation, which makes sense since this file uses the generated converters rather than defining them.
62-72: Good nil-safety in SetUsedBy.The nil check before dereferencing
rprevents potential panics when clearing theByfield. This defensive pattern is applied consistently across all three wrappers.
106-149: ClusterUsageWrapper correctly uses ResourceConverter for both fields.Unlike
UsageWrapper,ClusterUsageWrapperusesGeneratedResourceConverterfor bothOfandByfields. This makes sense becauseClusterUsageis cluster-scoped and itsSpec.Ofis aResource(notNamespacedResource).The implementation is consistent and correct.
183-226: LegacyUsageWrapper correctly uses the legacy converter.The
GeneratedLegacyResourceConverteris used throughout, which properly handles conversion between the deprecatedapiextensions/v1beta1.Usagetype and the internal representation.This maintains backward compatibility while keeping the internal interface consistent.
38-60: Converter usage is correct as designed.Great catch asking for clarification! Your assessment is spot-on:
Usage.Spec.OfisNamespacedResource→ correctly usesGeneratedNamespacedResourceConverterUsage.Spec.ByisResource→ correctly usesGeneratedResourceConverterThe design mirrors the type structure well, and the implementation is sound. Thanks for being thorough!
negz
left a comment
negz
left a comment
There was a problem hiding this comment.
Thanks!
I like the DAG approach. Usages are starting to feel a bit too clever, but I don't have any better ideas.
| dag := r.newDag() | ||
|
|
||
| implied, err := dag.Init(v1beta1.ToNodes(lock.Packages...)) | ||
| implied, err := dag.Init(controller.PackagesToNodes(lock.Packages...)) |
There was a problem hiding this comment.
Why package controller? controller.PackagesToNodes is a little unintuitive to me.
There was a problem hiding this comment.
Could the wrapper types live in the existing dag package? 🤔
There was a problem hiding this comment.
I didn't put them in dag since currently it's totally generic. But, this is the only place we use the dag package, so it's kind of a premature abstraction. I'll try moving the wrappers there.
| args: args{ | ||
| mgr: &fake.Manager{}, | ||
| u: &v1beta1.Usage{}, | ||
| u: func() protection.Usage { return &protection.UsageWrapper{} }, |
There was a problem hiding this comment.
I'd prefer a name that made it more obvious why there was wrapping going on / what was wrapped, but I can't think of anything. protection.InternalUsage or InternalUsageWrapper?
There was a problem hiding this comment.
Updated to InternalUsage. I like it better than UsageWrapper, but it still doesn't seem super obvious. I also don't have a much better idea.
I'd really like to make the wrappers not implement resource.Object and resource.Conditioned, so that it's not possible to accidentally pass them anywhere that expects a real API type (like client.Update). This would also keep the Usage interface smaller, which is aesthetically nice. I may play with this some more, since it feels brittle to me right now.
There was a problem hiding this comment.
This was actually easier than I thought. I've updated again to simplify the Usage interface and force users (i.e., the reconciler) to unwrap whenever they need an API object. This means moving the Conditioned implementation back into apis, which is probably the right place for it.
I like this a lot better now overall. The internal Usage interface is just an abstraction for the parts of the API types that are actually different - field naming and details of how they represent resource refs. The reconciler uses that abstraction, but unwraps to get the API object whenever it's dealing with k8s concepts.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/controller/pkg/resolver/reconciler.go (2)
308-316: Critical: Type assertion result not checked before use.There's a critical issue with the type assertion on line 308:
dep, ok := implied[0].(*internaldag.DependencyNode) depID := dep.Identifier() // Line 310 - dep used before ok is checked! if !ok { // Error handling }If the type assertion fails,
depwill benil, and callingdep.Identifier()on line 310 will cause a nil pointer panic before the!okcheck on line 311 can handle the error.🐛 Proposed fix
- dep, ok := implied[0].(*internaldag.DependencyNode) - - depID := dep.Identifier() - if !ok { + dep, ok := implied[0].(*internaldag.DependencyNode) + if !ok { + log.Debug(errInvalidDependency, "error", errors.Errorf(errFmtMissingDependency, implied[0].Identifier())) + status.MarkConditions(v1beta1.ResolutionFailed(errors.Errorf(errFmtMissingDependency, implied[0].Identifier()))) + + return reconcile.Result{}, errors.Wrap(r.client.Status().Update(ctx, lock), errCannotUpdateStatus) + } + + depID := dep.Identifier()Alternatively, if all implied nodes should be
DependencyNodeat this point, document why and consider using a panic-safe assertion.
277-397: Well-structured internal abstraction with one critical fix needed.The reconciler changes demonstrate a clean separation between internal DAG concerns and public API types. The wrapper/unwrapper pattern is applied consistently, and the public-facing functions continue to work with
v1beta1types as expected.However, the critical type assertion issue on line 308 must be fixed before merging (see separate comment).
🧹 Nitpick comments (1)
internal/protection/wrappers.go (1)
39-42: Consider extracting converters as package-level variables (optional).Each method creates a new converter instance (e.g.,
conv := GeneratedNamespacedResourceConverter{}). If these converters are stateless (which they appear to be based on their generated nature), you could optionally declare them as package-level variables to avoid repeated instantiation.That said, since these are likely empty structs and the methods aren't on hot paths, the current approach is perfectly fine. Just mentioning as a potential micro-optimization if you prefer that style.
♻️ Optional refactor example
// Package-level converter instances var ( namespacedResourceConverter = GeneratedNamespacedResourceConverter{} resourceConverter = GeneratedResourceConverter{} legacyResourceConverter = GeneratedLegacyResourceConverter{} ) // Then in methods: func (u *InternalUsage) GetUserOf() Resource { return namespacedResourceConverter.ToInternal(u.Spec.Of) }Also applies to: 56-57, 69-70
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
internal/controller/pkg/resolver/reconciler.gointernal/controller/pkg/resolver/reconciler_test.gointernal/controller/pkg/revision/dependency.gointernal/controller/pkg/revision/dependency_test.gointernal/controller/pkg/revision/fuzz_test.gointernal/controller/protection/usage/reconciler.gointernal/controller/protection/usage/reconciler_test.gointernal/controller/protection/usage/selector_test.gointernal/dag/pkg.gointernal/protection/usage/finder.gointernal/protection/usage/finder_test.gointernal/protection/wrappers.gointernal/protection/wrappers_test.gointernal/webhook/protection/usage/handler_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/webhook/protection/usage/handler_test.go
- internal/controller/pkg/revision/fuzz_test.go
🧰 Additional context used
📓 Path-based instructions (3)
**/*.go
⚙️ CodeRabbit configuration file
**/*.go: Enforce Crossplane-specific patterns: Use crossplane-runtime/pkg/errors
for wrapping. Check variable naming (short for local scope, descriptive
for wider scope). Ensure 'return early' pattern. Verify error scoping
(declare in conditionals when possible). For nolint directives, require
specific linter names and explanations. CRITICAL: Ensure all error
messages are meaningful to end users, not just developers - avoid
technical jargon, include context about what the user was trying to do,
and suggest next steps when possible.
Files:
internal/controller/protection/usage/reconciler_test.gointernal/protection/usage/finder.gointernal/dag/pkg.gointernal/controller/protection/usage/selector_test.gointernal/controller/pkg/resolver/reconciler.gointernal/controller/pkg/resolver/reconciler_test.gointernal/protection/wrappers.gointernal/controller/protection/usage/reconciler.gointernal/protection/usage/finder_test.gointernal/controller/pkg/revision/dependency.gointernal/controller/pkg/revision/dependency_test.gointernal/protection/wrappers_test.go
**/*_test.go
⚙️ CodeRabbit configuration file
**/*_test.go: Enforce table-driven test structure: PascalCase test names (no
underscores), args/want pattern, use cmp.Diff with
cmpopts.EquateErrors() for error testing. Check for proper test case
naming and reason fields. Ensure no third-party test frameworks (no
Ginkgo, Gomega, Testify).
Files:
internal/controller/protection/usage/reconciler_test.gointernal/controller/protection/usage/selector_test.gointernal/controller/pkg/resolver/reconciler_test.gointernal/protection/usage/finder_test.gointernal/controller/pkg/revision/dependency_test.gointernal/protection/wrappers_test.go
**/internal/controller/**
⚙️ CodeRabbit configuration file
**/internal/controller/**: Review controller logic for proper reconciliation patterns, error
handling, and resource management. Pay special attention to conditions
and events: Conditions must be actionable for users (not developers),
stable/deterministic, with proper Type/Reason/Message format. Events
only when something actually happens, with specific details about what
changed. No transient errors in conditions/events. All error messages
must be meaningful to end users - include context about what
resource/operation failed and why.
Files:
internal/controller/protection/usage/reconciler_test.gointernal/controller/protection/usage/selector_test.gointernal/controller/pkg/resolver/reconciler.gointernal/controller/pkg/resolver/reconciler_test.gointernal/controller/protection/usage/reconciler.gointernal/controller/pkg/revision/dependency.gointernal/controller/pkg/revision/dependency_test.go
🧠 Learnings (3)
📚 Learning: 2025-12-02T14:41:54.485Z
Learnt from: CR
Repo: crossplane/crossplane PR: 0
File: contributing/README.md:0-0
Timestamp: 2025-12-02T14:41:54.485Z
Learning: Applies to contributing/**/*_test.go : Crossplane does not use third-party test libraries (e.g., Ginkgo, Gomega, Testify) for unit tests; use only the standard Go testing library
Applied to files:
internal/controller/protection/usage/selector_test.go
📚 Learning: 2025-12-02T14:41:54.485Z
Learnt from: CR
Repo: crossplane/crossplane PR: 0
File: contributing/README.md:0-0
Timestamp: 2025-12-02T14:41:54.485Z
Learning: Applies to contributing/**/*.go : Use crossplane-runtime/pkg/errors to wrap errors with context rather than using error constants
Applied to files:
internal/controller/protection/usage/selector_test.go
📚 Learning: 2025-12-02T14:41:54.485Z
Learnt from: CR
Repo: crossplane/crossplane PR: 0
File: contributing/README.md:0-0
Timestamp: 2025-12-02T14:41:54.485Z
Learning: Applies to contributing/**/*_test.go : Use https://github.com/google/go-cmp/ for comparisons in tests to keep test output consistent
Applied to files:
internal/controller/pkg/revision/dependency_test.go
🧬 Code graph analysis (11)
internal/controller/protection/usage/reconciler_test.go (4)
internal/protection/usage.go (1)
Usage(96-108)apis/protection/v1beta1/usage_types.go (1)
Usage(162-168)apis/apiextensions/v1beta1/usage_types.go (1)
Usage(99-106)internal/protection/wrappers.go (1)
InternalUsage(30-32)
internal/protection/usage/finder.go (3)
internal/protection/wrappers.go (3)
InternalUsage(30-32)InternalClusterUsage(107-109)InternalLegacyUsage(184-186)internal/protection/usage.go (1)
Usage(96-108)apis/protection/v1beta1/clusterusage_types.go (1)
ClusterUsage(40-46)
internal/dag/pkg.go (2)
internal/dag/dag.go (1)
Node(25-38)apis/pkg/v1beta1/lock.go (1)
LockPackage(36-66)
internal/controller/protection/usage/selector_test.go (1)
internal/protection/usage.go (4)
Usage(96-108)ResourceRef(30-38)ResourceSelector(41-53)Resource(56-69)
internal/controller/pkg/resolver/reconciler.go (3)
internal/dag/pkg.go (2)
PackagesToNodes(79-86)DependencyNode(29-31)apis/pkg/v1beta1/lock.go (1)
Dependency(89-113)apis/pkg/meta/v1/meta.go (1)
Dependency(44-80)
internal/protection/wrappers.go (3)
internal/protection/usage.go (2)
Usage(96-108)Resource(56-69)internal/protection/usage/finder.go (1)
Object(107-111)apis/protection/v1beta1/clusterusage_types.go (1)
ClusterUsage(40-46)
internal/controller/protection/usage/reconciler.go (2)
internal/protection/usage.go (1)
Usage(96-108)internal/protection/wrappers.go (3)
InternalUsage(30-32)InternalClusterUsage(107-109)InternalLegacyUsage(184-186)
internal/protection/usage/finder_test.go (2)
internal/protection/wrappers.go (3)
InternalUsage(30-32)InternalClusterUsage(107-109)InternalLegacyUsage(184-186)internal/protection/usage.go (1)
Usage(96-108)
internal/controller/pkg/revision/dependency.go (1)
internal/dag/pkg.go (3)
PackagesToNodes(79-86)PackageNode(49-51)DependencyNode(29-31)
internal/controller/pkg/revision/dependency_test.go (1)
internal/dag/pkg.go (2)
DependencyNode(29-31)PackageNode(49-51)
internal/protection/wrappers_test.go (2)
internal/protection/usage.go (1)
Usage(96-108)internal/protection/wrappers.go (3)
InternalUsage(30-32)InternalClusterUsage(107-109)InternalLegacyUsage(184-186)
🔇 Additional comments (31)
internal/controller/protection/usage/reconciler_test.go (1)
67-67: LGTM! Factory function pattern consistently applied across all test cases.The change from a concrete
protection.Usageto a factory functionfunc() protection.Usageis well-executed. All 21 test cases consistently return&protection.InternalUsage{}, which aligns with the reconciler's updated signature that accepts a factory function for creating usage instances.This pattern effectively decouples the tests from the concrete API type while maintaining test clarity.
Also applies to: 86-86, 103-103, 125-125, 151-151, 180-180, 210-210, 246-246, 285-285, 324-324, 374-374, 434-434, 484-484, 519-519, 554-554, 598-598, 642-642, 681-681, 723-723, 758-758, 806-806, 859-859
internal/protection/wrappers_test.go (1)
17-23: LGTM! Clean interface compliance checks.The interface compliance assertions for all three wrapper types (
InternalUsage,InternalClusterUsage,InternalLegacyUsage) follow Go best practices. These compile-time checks ensure the wrapper types correctly implement theUsageinterface.internal/protection/usage/finder.go (3)
123-123: LGTM! Safe value wrapping.The wrapping of
v1beta1.Usageitems inInternalUsageis correctly implemented. The struct literal&protection.InternalUsage{Usage: u}copies the loop variable by value, avoiding any loop variable capture issues.
132-132: LGTM! Consistent wrapping pattern.The
InternalClusterUsagewrapper follows the same safe pattern as the Usage wrapper above.
141-141: LGTM! Complete wrapper implementation.The
InternalLegacyUsagewrapper completes the consistent pattern across all three usage types. The deprecated usage type is still properly supported through the wrapper approach.internal/controller/pkg/revision/dependency_test.go (3)
284-293: LGTM! DAG node wrappers properly introduced.The
MockInitfunction now returnsdag.DependencyNodewrappers that embedv1beta1.Dependency. This correctly implements the pattern of moving DAG concerns into internal wrapper types.
298-300: LGTM! Consistent MockTraceNode updates.All
MockTraceNodeimplementations consistently return maps withdag.DependencyNodevalues instead of rawv1beta1.Dependencytypes. This maintains consistency with the wrapper-based approach across multiple test scenarios.Also applies to: 380-382, 491-494
387-401: LGTM! PackageNode wrappers correctly implemented.The
MockGetNodeimplementations returndag.PackageNodewrappers that embedv1beta1.LockPackage. This completes the DAG wrapper pattern by handling both dependency nodes and package nodes consistently.The version information and source fields are properly preserved in the wrapped structure.
Also applies to: 499-520
internal/protection/usage/finder_test.go (1)
158-166: LGTM! Test expectations correctly updated.The test expectations now correctly anticipate wrapped usage types (
InternalUsage,InternalClusterUsage,InternalLegacyUsage) that match the updatedFindUsageOfimplementation. The embedded usage objects are properly populated, and the test follows Crossplane's table-driven test pattern withcmp.Difffor comparisons.This maintains test coverage while adapting to the wrapper-based approach.
internal/dag/pkg.go (2)
65-76: Consider handling unmatched nodes in AddNeighbors.In
PackageNode.AddNeighbors(), nodes that don't match any dependency inl.Dependencieswill silently have no constraints added. Is this intentional? If a node is passed that should match but doesn't, this could lead to subtle bugs where constraints aren't properly propagated.Consider either:
- Documenting this behavior explicitly in a comment
- Returning an error when a node doesn't match any dependency
- Adding a flag to track if any match occurred
What was the design intent here - is it expected that some nodes might not match, or should all nodes passed to this method have a corresponding dependency?
78-86: LGTM! Clean conversion utility.The
PackagesToNodesfunction is straightforward and correctly wraps eachLockPackagein aPackageNode. The variadic parameter design allows flexible usage.internal/controller/pkg/revision/dependency.go (3)
137-137: Correct migration to internal DAG package.The change from
v1beta1.ToNodes()todag.PackagesToNodes()correctly migrates DAG logic out of the public API package into internal, aligning with the PR objectives.
195-207: Proper wrapping of API types with internal node types.The wrapping of
selfasdag.PackageNodeanddepasdag.DependencyNodecorrectly creates the internal DAG representations while maintaining the embedded API types. This approach keeps the API types clean while adding DAG functionality internally.
247-250: Safe type assertion with proper error handling.The type assertion to
*dag.PackageNodecorrectly uses the two-value form and returns a meaningful error if the assertion fails. The error messageerrDependencyNotLockPackageis appropriate for this context.internal/controller/protection/usage/selector_test.go (2)
42-48: Consistent migration to internal usage interface.The test type signatures correctly migrate from concrete
*v1beta1.Usageto theprotection.Usageinterface, enabling the internal wrapper pattern while maintaining test coverage.
59-120: Excellent consistency in test data migration.All test cases consistently wrap
v1beta1.Usagedata inprotection.InternalUsagewrappers. The systematic migration maintains test coverage while adopting the new internal wrapper pattern. The test logic remains unchanged, which is exactly right for this refactoring.internal/controller/pkg/resolver/reconciler_test.go (2)
347-351: Systematic test migration to new node types.The test cases consistently wrap dependencies in
dag.DependencyNode, with all fields properly nested under theDependencyfield. This systematic update across all test cases demonstrates careful attention to maintaining test coverage while adopting the new internal DAG abstractions. Great work!
1036-1045: Mock behavior correctly updated for new node types.The
MockGetNodereturns correctly wrap dependencies indag.DependencyNode, including proper nesting ofParentConstraintsand other fields. The mock behavior aligns perfectly with the production code changes.internal/controller/pkg/resolver/reconciler.go (3)
277-277: Correct DAG initialization with internal package.The migration to
internaldag.PackagesToNodes()correctly uses the new internal DAG package. The import aliasinternaldagis clear and follows Go conventions for disambiguating package names.
333-333: Correct access to embedded dependency field.The call
NewPackageList(&dep.Dependency)correctly accesses the embeddedv1beta1.Dependencyfield from theDependencyNodewrapper. This pattern maintains clean separation between internal DAG concerns and the public API types.
379-397: Consistent unwrapping pattern for internal wrappers.The calls to
findDependencyVersionToInstallandNewPackageconsistently pass&dep.Dependencyto unwrap the internal DAG node and access the underlying API type. This pattern is applied uniformly throughout the reconciler, maintaining a clean separation of concerns.internal/controller/protection/usage/reconciler.go (6)
105-110: LGTM! Clean factory pattern implementation.The factory function approach nicely decouples the reconciler from concrete types while enabling fresh instance creation per reconciliation. This aligns well with the PR's goal of moving internal implementations out of the apis package.
122-127: LGTM! Consistent usage of wrapper types across all setup functions.All three setup functions (Usage, ClusterUsage, LegacyUsage) follow the same pattern with their respective internal wrapper types. Thank you for the consistent implementation!
Also applies to: 140-145
218-218: LGTM! Factory function wiring is correct.The signature change from direct instance to factory function (
u func() protection.Usage) and the corresponding struct field update properly support the lazy instantiation pattern.Also applies to: 233-233, 257-257
277-278: Verify wrapper/unwrap mutation semantics remain consistent.The flow here is: create wrapper via
r.newUsage(), thenGetintou.Unwrap(). Since the wrapper embeds the API type andUnwrap()returns a pointer to that embedded struct, the fetched data will be accessible through the wrapper. This looks correct.Could you confirm that downstream operations like
meta.AddAnnotations(u, ...)(line 447) andu.GetAnnotations()(line 446) correctly operate on the same underlying ObjectMeta thatu.Unwrap()returns? Based on Go's embedding semantics withInternalUsage struct { v1beta1.Usage }, this should work, but I want to make sure this was intentionally verified.
415-415: LGTM! Finalizer operations correctly use unwrapped objects.The
xpresource.Finalizerinterface expectsresource.Object, so passingu.Unwrap()is the right approach here. The finalizer will modify the underlying API object directly.Also applies to: 432-432
451-451: LGTM! Client operations consistently use unwrapped objects.All
r.client.Update()andr.client.Status().Update()calls now useu.Unwrap()to pass the concrete API type to the Kubernetes client. This is correct since the client needs the actual API object, not the internal wrapper interface.Also applies to: 519-519, 541-541
internal/protection/wrappers.go (4)
1-2: Build constraint looks intentional - thanks for including it.The
//go:build !goverterconstraint excludes this file when building with the goverter tag. Could you briefly confirm this is to avoid conflicts with generated conversion code? Just want to make sure I understand the reasoning correctly.
29-104: LGTM! InternalUsage wrapper is well-implemented.The wrapper correctly:
- Uses
GeneratedNamespacedResourceConverterforOf(since Usage is namespace-scoped and can reference namespaced resources)- Uses
GeneratedResourceConverterforBy- Handles nil
Byfield safely in both getter and setter- Delegates condition methods to the embedded Status
The
Unwrap()returning&u.Usageensures mutations through the wrapper are visible to callers using the unwrapped object.
106-181: LGTM! InternalClusterUsage wrapper follows consistent pattern.Correctly uses
GeneratedResourceConverterfor bothOfandByfields since ClusterUsage is cluster-scoped. The implementation mirrors InternalUsage appropriately.
183-258: LGTM! InternalLegacyUsage wrapper completes the set.Correctly uses
GeneratedLegacyResourceConverterfor the legacy API types. This maintains backward compatibility for the deprecated apiextensions.crossplane.io API group.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @internal/controller/protection/usage/reconciler.go:
- Line 293: The current comparison is between the wrapper `u` and the `orig`
runtime.Object so it always appears changed; change the comparison to compare
the actual API objects (e.g., use `cmp.Equal(uu, orig)` or
`cmp.Equal(u.Unwrap(), orig)`) when deciding to emit the "UsageConfigured" event
and update status; locate where `orig := uu.DeepCopyObject()` is set and replace
the existing `cmp.Equal` call to compare `uu` (or `u.Unwrap()`) with `orig`
instead of `u` so the change detection reflects underlying object mutations.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apis/apiextensions/v1beta1/conditions.goapis/protection/v1beta1/conditions.gointernal/controller/protection/usage/reconciler.gointernal/controller/protection/usage/selector.gointernal/protection/usage.gointernal/protection/wrappers.gointernal/webhook/protection/usage/handler.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/controller/protection/usage/selector.go
- internal/protection/usage.go
🧰 Additional context used
📓 Path-based instructions (5)
{apis,cmd}/**
📄 CodeRabbit inference engine (Custom checks)
{apis,cmd}/**: Public fields/flags in apis/** or cmd/** must not be removed or renamed without a 'breaking-change' label
New required public fields/flags in apis/** or cmd/** must not be added without a 'breaking-change' label
Behavior removals in apis/** or cmd/** must be labeled with 'breaking-change'
Files:
apis/protection/v1beta1/conditions.goapis/apiextensions/v1beta1/conditions.go
apis/**
📄 CodeRabbit inference engine (Custom checks)
New experimental features affecting apis/** must have feature flag implementations
Files:
apis/protection/v1beta1/conditions.goapis/apiextensions/v1beta1/conditions.go
**/*.go
⚙️ CodeRabbit configuration file
**/*.go: Enforce Crossplane-specific patterns: Use crossplane-runtime/pkg/errors
for wrapping. Check variable naming (short for local scope, descriptive
for wider scope). Ensure 'return early' pattern. Verify error scoping
(declare in conditionals when possible). For nolint directives, require
specific linter names and explanations. CRITICAL: Ensure all error
messages are meaningful to end users, not just developers - avoid
technical jargon, include context about what the user was trying to do,
and suggest next steps when possible.
Files:
apis/protection/v1beta1/conditions.gointernal/webhook/protection/usage/handler.goapis/apiextensions/v1beta1/conditions.gointernal/protection/wrappers.gointernal/controller/protection/usage/reconciler.go
**/apis/**
⚙️ CodeRabbit configuration file
**/apis/**: Focus on API design following Kubernetes API conventions from
https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md.
Check for proper field naming (camelCase), appropriate types, validation
tags, and documentation. Ask about backward compatibility and the impact
on existing users and upgrade paths. Consider if changes need feature
gates or alpha/beta graduation. Ensure error messages in validation are
user-friendly. Pay attention to API consistency, proper use of optional
vs required fields, and following established Kubernetes patterns.
Files:
apis/protection/v1beta1/conditions.goapis/apiextensions/v1beta1/conditions.go
**/internal/controller/**
⚙️ CodeRabbit configuration file
**/internal/controller/**: Review controller logic for proper reconciliation patterns, error
handling, and resource management. Pay special attention to conditions
and events: Conditions must be actionable for users (not developers),
stable/deterministic, with proper Type/Reason/Message format. Events
only when something actually happens, with specific details about what
changed. No transient errors in conditions/events. All error messages
must be meaningful to end users - include context about what
resource/operation failed and why.
Files:
internal/controller/protection/usage/reconciler.go
🧬 Code graph analysis (5)
apis/protection/v1beta1/conditions.go (2)
internal/protection/usage.go (1)
Usage(96-104)apis/protection/v1beta1/clusterusage_types.go (1)
ClusterUsage(40-46)
internal/webhook/protection/usage/handler.go (3)
internal/protection/usage.go (1)
ResourceRef(30-38)apis/protection/v1beta1/usage_types.go (1)
ResourceRef(27-30)apis/apiextensions/v1beta1/usage_types.go (1)
ResourceRef(26-29)
apis/apiextensions/v1beta1/conditions.go (1)
internal/protection/usage.go (1)
Usage(96-104)
internal/protection/wrappers.go (2)
internal/protection/usage.go (2)
Usage(96-104)Resource(56-69)apis/protection/v1beta1/clusterusage_types.go (1)
ClusterUsage(40-46)
internal/controller/protection/usage/reconciler.go (1)
internal/protection/usage.go (2)
Usage(96-104)ResourceRef(30-38)
🔍 Remote MCP
I'll search for relevant context about this PR's objectives and the related architecture changes to help with your review.The search results returned information about Apache Airflow's DAG dependencies rather than Crossplane's. Let me search more specifically for Crossplane architecture and Go module information.Based on the search results, I found relevant information about the PR's context. Let me provide you with a concise summary of the relevant findings:
Summary of Relevant Context for PR Review
Usage API Context
Usage defines a usage relationship for Managed Resources or Composites. Usages are a beta feature and are enabled by default. The Usage feature supports protecting a resource from accidental deletion and deletion ordering by ensuring that a resource isn't deleted before the deletion of its dependent resources.
The PR is moving internal implementations of the Usage interface (for Usage, ClusterUsage, and legacy Usage types) into wrapper types located in internal/protection, rather than keeping them as public methods in the apis package.
Wrapper Type Approach
The refactoring creates three new internal wrapper types:
InternalUsagewrappingv1beta1.UsageInternalClusterUsagewrappingv1beta1.ClusterUsageInternalLegacyUsagewrappinglegacy.Usage
Each wrapper implements the internal protection.Usage interface with methods like GetUserOf(), SetUserOf(), GetUsedBy(), SetUsedBy(), GetReason(), SetReason(), GetReplayDeletion(), SetReplayDeletion(), and condition handling methods—all delegating to the wrapped API object via an Unwrap() method.
Key Architectural Change
The PR eliminates direct interface implementations from public API types, treating interface implementation as an internal concern. This allows the apis package to become a standalone Go module (referenced as issue #6998) by removing all internal package imports from public API definitions.
DAG Restructuring
For package management, the PR also moves DAG (Directed Acyclic Graph) logic out of the public API by introducing DependencyNode and PackageNode wrapper types in internal/dag, so that only controllers—not API consumers—need to understand DAG concerns.
🔇 Additional comments (9)
internal/webhook/protection/usage/handler.go (1)
163-183: LGTM! Clean integration with the wrapper pattern.The
inUseMessagefunction correctly unwraps the usage object and uses the underlying API object for all formatting operations. This change aligns well with the new internal wrapper architecture.apis/apiextensions/v1beta1/conditions.go (1)
23-31: LGTM! Proper delegation to Status for condition management.These methods follow Kubernetes API conventions by providing condition accessors at the top-level type that delegate to the Status field. This enables the wrapper's
Unwrap()method to return a validconditions.ObjectWithConditions.apis/protection/v1beta1/conditions.go (1)
23-41: LGTM! Consistent condition accessor pattern.Both
UsageandClusterUsagetypes follow the same delegation pattern seen in the legacy API, properly exposing condition management through their Status fields.internal/controller/protection/usage/reconciler.go (2)
105-107: LGTM! Factory pattern correctly implemented.The three setup functions properly pass factory functions that return the appropriate wrapper types (
InternalUsage,InternalClusterUsage,InternalLegacyUsage). This lazy instantiation pattern aligns well with the new internal wrapper architecture.Also applies to: 122-124, 140-142
277-279: LGTM! Unwrapping pattern correctly applied.The reconcile method properly instantiates a new usage via the factory function and immediately unwraps it to obtain the underlying API object for Kubernetes operations. This is the correct approach for working with the wrapper types.
internal/protection/wrappers.go (4)
28-93: LGTM! InternalUsage wrapper correctly implemented.The wrapper properly:
- Embeds the
v1beta1.Usagetype- Implements
Unwrap()returning the embedded object- Uses the appropriate
GeneratedNamespacedResourceConverterforOfconversions (since Usage is namespaced)- Handles nil cases correctly in
SetUsedBy- Provides simple passthrough for
ReasonandReplayDeletionfields
95-160: LGTM! InternalClusterUsage wrapper follows the same pattern.The implementation is consistent with
InternalUsage, usingGeneratedResourceConverter(instead of the namespaced variant) which is appropriate for cluster-scoped resources.
162-227: LGTM! InternalLegacyUsage wrapper maintains consistency.The wrapper correctly uses
GeneratedLegacyResourceConverterfor backward compatibility with the legacy API, maintaining the same structure and nil-safety patterns as the other wrappers.
1-1: Build tag appropriately excludes file during code generation.The
//go:build !govertertag correctly prevents this file from being included during goverter code generation, which is appropriate since these are manual wrapper implementations that use the generated converters.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @internal/controller/protection/usage/reconciler.go:
- Around line 216-218: Update the comment for NewReconciler to describe that the
second parameter is a factory function rather than an empty type: explain that u
is a factory function that returns a new protection.Usage instance on each call,
and fix the typo "satsifes" to "satisfies"; ensure the comment clearly states
the factory pattern (e.g., "u is a function that creates a new
protection.Usage") and keeps the rest of the description in sync with the
function signature.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/controller/protection/usage/reconciler.go
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
⚙️ CodeRabbit configuration file
**/*.go: Enforce Crossplane-specific patterns: Use crossplane-runtime/pkg/errors
for wrapping. Check variable naming (short for local scope, descriptive
for wider scope). Ensure 'return early' pattern. Verify error scoping
(declare in conditionals when possible). For nolint directives, require
specific linter names and explanations. CRITICAL: Ensure all error
messages are meaningful to end users, not just developers - avoid
technical jargon, include context about what the user was trying to do,
and suggest next steps when possible.
Files:
internal/controller/protection/usage/reconciler.go
**/internal/controller/**
⚙️ CodeRabbit configuration file
**/internal/controller/**: Review controller logic for proper reconciliation patterns, error
handling, and resource management. Pay special attention to conditions
and events: Conditions must be actionable for users (not developers),
stable/deterministic, with proper Type/Reason/Message format. Events
only when something actually happens, with specific details about what
changed. No transient errors in conditions/events. All error messages
must be meaningful to end users - include context about what
resource/operation failed and why.
Files:
internal/controller/protection/usage/reconciler.go
🧠 Learnings (1)
📚 Learning: 2025-12-02T14:41:54.485Z
Learnt from: CR
Repo: crossplane/crossplane PR: 0
File: contributing/README.md:0-0
Timestamp: 2025-12-02T14:41:54.485Z
Learning: Applies to contributing/**/*.go : Do not show transient issues (e.g., apiserver conflict errors) in condition messages; instead, silently requeue the reconciliation
Applied to files:
internal/controller/protection/usage/reconciler.go
🧬 Code graph analysis (1)
internal/controller/protection/usage/reconciler.go (2)
internal/protection/usage.go (1)
Usage(96-104)internal/protection/wrappers.go (3)
InternalUsage(29-31)InternalClusterUsage(96-98)InternalLegacyUsage(163-165)
🔇 Additional comments (3)
internal/controller/protection/usage/reconciler.go (3)
105-107: LGTM! Factory pattern correctly implemented.The Setup functions now properly pass factory functions that return internal wrapper types, which aligns with the PR's goal of moving usage interface implementations into the internal package. The pattern is consistent across all three usage types.
Also applies to: 122-124, 140-142
277-279: Excellent use of the factory and unwrap pattern.The factory function creates a fresh usage instance for each reconciliation, and the unwrapping is done consistently at the start. This cleanly separates the interface-level operations (on
u) from the Kubernetes object operations (onuu).
293-542: Consistent unwrapping throughout the reconciliation flow.All Kubernetes operations (Get, Update, event recording, finalizers, status updates, namespace/owner references) consistently use the unwrapped object
uu, while interface methods are appropriately called on the wrapped objectu. This demonstrates careful attention to the wrapper abstraction.
We're going to make the apis directory its own go module so that we can depend on it from crossplane-runtime, and so that other importers don't need to depend on all of crossplane. To do that, apis can't depend on anything from internal, which means we need to implement some DAG-related methods outside of apis. This is a nice change anyway, since the DAG is an internal Crossplane concern that shouldn't really be exposed through the API. Implement wrapper types in the pkg controller that implement the necessary DAG methods, and update consuming code to use them. Signed-off-by: Adam Wolfe Gordon <awg@upbound.io>
The usage interface exists to let us implement a single reconciler for multiple usage types. Implementing the interface is an internal concern, so doesn't really belong in apis, and introduces a dependency on internal from apis, preventing us from making apis a separate repository or go module. Move the interface implementations into `internal/protection` by introducing wrapper types for each of the usage API types. Signed-off-by: Adam Wolfe Gordon <awg@upbound.io>
|
Force pushed a rebase to resolve conflicts and integrate fixup commits. |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/controller/pkg/resolver/reconciler.go (1)
285-293: Consider reordering:depIDis accessed before theokcheck.I noticed that
dep.Identifier()is called on line 287 before checking if the type assertion succeeded on line 288. If the type assertion fails,depwould benil, leading to a panic.Could you clarify whether this ordering was intentional? It looks like this pattern may have existed before your changes, but I wanted to flag it in case you'd like to address it while you're in this area. Moving
depID := dep.Identifier()after theif !okblock would be the safer approach.
🧹 Nitpick comments (1)
internal/protection/wrappers.go (1)
39-41: Consider extracting converter instantiation to reduce allocations.Quick question - I noticed that each Get/Set method instantiates a new converter struct. Since these converters appear to be stateless, would it make sense to use package-level singleton instances? This would reduce allocations, especially in hot paths. That said, if these methods aren't called frequently, the current approach is perfectly fine and arguably more readable.
♻️ Optional: Use package-level converter instances
// Package-level converter instances (if converters are stateless) var ( namespacedResourceConverter = GeneratedNamespacedResourceConverter{} resourceConverter = GeneratedResourceConverter{} legacyResourceConverter = GeneratedLegacyResourceConverter{} ) // Then in methods: func (u *InternalUsage) GetUserOf() Resource { return namespacedResourceConverter.ToInternal(u.Spec.Of) }Also applies to: 55-56, 106-107, 122-123, 173-174, 189-190
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
apis/apiextensions/v1beta1/zz_generated.deepcopy.gois excluded by!**/zz_generated*.goand included by**/*.goapis/protection/v1beta1/zz_generated.deepcopy.gois excluded by!**/zz_generated*.goand included by**/*.gointernal/protection/zz_generated.conversion.gois excluded by!**/zz_generated*.goand included by**/*.gointernal/protection/zz_generated.conversion_legacy.gois excluded by!**/zz_generated*.goand included by**/*.go
📒 Files selected for processing (26)
apis/apiextensions/v1beta1/conditions.goapis/apiextensions/v1beta1/usage_interface.goapis/pkg/v1beta1/lock.goapis/protection/v1beta1/clusterusage_interface.goapis/protection/v1beta1/conditions.goapis/protection/v1beta1/usage_interface.gogenerate.gointernal/controller/pkg/resolver/reconciler.gointernal/controller/pkg/resolver/reconciler_test.gointernal/controller/pkg/revision/dependency.gointernal/controller/pkg/revision/dependency_test.gointernal/controller/pkg/revision/fuzz_test.gointernal/controller/protection/usage/reconciler.gointernal/controller/protection/usage/reconciler_test.gointernal/controller/protection/usage/selector.gointernal/controller/protection/usage/selector_test.gointernal/dag/pkg.gointernal/protection/conversion.gointernal/protection/conversion_legacy.gointernal/protection/usage.gointernal/protection/usage/finder.gointernal/protection/usage/finder_test.gointernal/protection/wrappers.gointernal/protection/wrappers_test.gointernal/webhook/protection/usage/handler.gointernal/webhook/protection/usage/handler_test.go
💤 Files with no reviewable changes (4)
- apis/apiextensions/v1beta1/usage_interface.go
- apis/pkg/v1beta1/lock.go
- apis/protection/v1beta1/clusterusage_interface.go
- apis/protection/v1beta1/usage_interface.go
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/controller/protection/usage/reconciler_test.go
- internal/protection/conversion_legacy.go
- apis/protection/v1beta1/conditions.go
- apis/apiextensions/v1beta1/conditions.go
🧰 Additional context used
📓 Path-based instructions (3)
**/*.go
⚙️ CodeRabbit configuration file
**/*.go: Enforce Crossplane-specific patterns: Use crossplane-runtime/pkg/errors
for wrapping. Check variable naming (short for local scope, descriptive
for wider scope). Ensure 'return early' pattern. Verify error scoping
(declare in conditionals when possible). For nolint directives, require
specific linter names and explanations. CRITICAL: Ensure all error
messages are meaningful to end users, not just developers - avoid
technical jargon, include context about what the user was trying to do,
and suggest next steps when possible.
Files:
internal/controller/pkg/resolver/reconciler.gointernal/controller/protection/usage/selector.gointernal/controller/pkg/revision/dependency.gointernal/protection/usage.gointernal/protection/usage/finder_test.gointernal/controller/protection/usage/selector_test.gointernal/protection/conversion.gointernal/protection/wrappers.gointernal/controller/pkg/revision/dependency_test.gointernal/controller/protection/usage/reconciler.gointernal/controller/pkg/revision/fuzz_test.gointernal/controller/pkg/resolver/reconciler_test.gointernal/webhook/protection/usage/handler_test.gointernal/webhook/protection/usage/handler.gointernal/protection/usage/finder.gogenerate.gointernal/protection/wrappers_test.gointernal/dag/pkg.go
**/internal/controller/**
⚙️ CodeRabbit configuration file
**/internal/controller/**: Review controller logic for proper reconciliation patterns, error
handling, and resource management. Pay special attention to conditions
and events: Conditions must be actionable for users (not developers),
stable/deterministic, with proper Type/Reason/Message format. Events
only when something actually happens, with specific details about what
changed. No transient errors in conditions/events. All error messages
must be meaningful to end users - include context about what
resource/operation failed and why.
Files:
internal/controller/pkg/resolver/reconciler.gointernal/controller/protection/usage/selector.gointernal/controller/pkg/revision/dependency.gointernal/controller/protection/usage/selector_test.gointernal/controller/pkg/revision/dependency_test.gointernal/controller/protection/usage/reconciler.gointernal/controller/pkg/revision/fuzz_test.gointernal/controller/pkg/resolver/reconciler_test.go
**/*_test.go
⚙️ CodeRabbit configuration file
**/*_test.go: Enforce table-driven test structure: PascalCase test names (no
underscores), args/want pattern, use cmp.Diff with
cmpopts.EquateErrors() for error testing. Check for proper test case
naming and reason fields. Ensure no third-party test frameworks (no
Ginkgo, Gomega, Testify).
Files:
internal/protection/usage/finder_test.gointernal/controller/protection/usage/selector_test.gointernal/controller/pkg/revision/dependency_test.gointernal/controller/pkg/revision/fuzz_test.gointernal/controller/pkg/resolver/reconciler_test.gointernal/webhook/protection/usage/handler_test.gointernal/protection/wrappers_test.go
🧠 Learnings (10)
📚 Learning: 2025-12-02T14:41:54.485Z
Learnt from: CR
Repo: crossplane/crossplane PR: 0
File: contributing/README.md:0-0
Timestamp: 2025-12-02T14:41:54.485Z
Learning: Applies to contributing/**/*_test.go : Crossplane does not use third-party test libraries (e.g., Ginkgo, Gomega, Testify) for unit tests; use only the standard Go testing library
Applied to files:
internal/controller/protection/usage/selector_test.go
📚 Learning: 2025-12-02T14:41:54.485Z
Learnt from: CR
Repo: crossplane/crossplane PR: 0
File: contributing/README.md:0-0
Timestamp: 2025-12-02T14:41:54.485Z
Learning: Applies to contributing/**/*.go : Use crossplane-runtime/pkg/errors to wrap errors with context rather than using error constants
Applied to files:
internal/controller/protection/usage/selector_test.go
📚 Learning: 2025-12-02T14:41:54.485Z
Learnt from: CR
Repo: crossplane/crossplane PR: 0
File: contributing/README.md:0-0
Timestamp: 2025-12-02T14:41:54.485Z
Learning: Applies to contributing/**/*_test.go : Use https://github.com/google/go-cmp/ for comparisons in tests to keep test output consistent
Applied to files:
internal/controller/pkg/revision/dependency_test.go
📚 Learning: 2025-12-02T14:41:54.485Z
Learnt from: CR
Repo: crossplane/crossplane PR: 0
File: contributing/README.md:0-0
Timestamp: 2025-12-02T14:41:54.485Z
Learning: Applies to contributing/**/*.go : Do not show transient issues (e.g., apiserver conflict errors) in condition messages; instead, silently requeue the reconciliation
Applied to files:
internal/controller/protection/usage/reconciler.go
📚 Learning: 2025-12-02T14:41:54.485Z
Learnt from: CR
Repo: crossplane/crossplane PR: 0
File: contributing/README.md:0-0
Timestamp: 2025-12-02T14:41:54.485Z
Learning: Applies to contributing/**/*.go : Do not emit events for transient issues like apiserver conflict errors; instead, silently requeue the reconciliation
Applied to files:
internal/controller/protection/usage/reconciler.go
📚 Learning: 2025-12-02T14:41:54.485Z
Learnt from: CR
Repo: crossplane/crossplane PR: 0
File: contributing/README.md:0-0
Timestamp: 2025-12-02T14:41:54.485Z
Learning: Applies to contributing/**/*_test.go : Use table-driven tests instead of test frameworks like Ginkgo; structure tests with 'cases' map containing test cases with 'reason', 'args', and 'want' fields
Applied to files:
internal/webhook/protection/usage/handler_test.go
📚 Learning: 2025-12-02T14:41:54.485Z
Learnt from: CR
Repo: crossplane/crossplane PR: 0
File: contributing/README.md:0-0
Timestamp: 2025-12-02T14:41:54.485Z
Learning: Applies to contributing/**/*.go : Preempt common Go code review comments and test review comments as documented in the Go project's guidelines
Applied to files:
generate.go
📚 Learning: 2025-10-10T19:15:32.438Z
Learnt from: negz
Repo: crossplane/crossplane PR: 6844
File: cluster/crds/pkg.crossplane.io_deploymentruntimeconfigs.yaml:843-845
Timestamp: 2025-10-10T19:15:32.438Z
Learning: CRDs under cluster/crds/*.yaml are generated from upstream Kubernetes API types via controller-gen (see controller-gen.kubebuilder.io/version annotation). Descriptive text in these files mirrors upstream and should not be edited locally; fixes must be made upstream and pulled in via k8s.io module updates.
Applied to files:
generate.go
📚 Learning: 2025-12-02T14:41:54.485Z
Learnt from: CR
Repo: crossplane/crossplane PR: 0
File: contributing/README.md:0-0
Timestamp: 2025-12-02T14:41:54.485Z
Learning: Applies to contributing/**/*.go : Follow Crossplane's Observability Developer Guide
Applied to files:
generate.go
📚 Learning: 2025-09-09T20:15:06.308Z
Learnt from: negz
Repo: crossplane/crossplane PR: 6791
File: .coderabbit.yaml:79-103
Timestamp: 2025-09-09T20:15:06.308Z
Learning: The Crossplane project uses `zz_generated` as their standard naming convention for generated files, rather than broader patterns like `*.gen.go` or `*generated*.go`. The current path filters in `.coderabbit.yaml` with `!**/zz_generated*.go` correctly exclude their generated code.
Applied to files:
generate.go
🧬 Code graph analysis (13)
internal/controller/pkg/resolver/reconciler.go (3)
internal/dag/pkg.go (2)
PackagesToNodes(79-86)DependencyNode(29-31)apis/pkg/v1beta1/lock.go (1)
Dependency(89-113)apis/pkg/meta/v1/meta.go (1)
Dependency(44-80)
internal/controller/pkg/revision/dependency.go (2)
internal/dag/pkg.go (3)
PackagesToNodes(79-86)PackageNode(49-51)DependencyNode(29-31)apis/pkg/v1beta1/lock.go (2)
LockPackage(36-66)Dependency(89-113)
internal/protection/usage/finder_test.go (4)
internal/protection/wrappers.go (3)
InternalUsage(29-31)InternalClusterUsage(96-98)InternalLegacyUsage(163-165)internal/protection/usage.go (1)
Usage(96-104)apis/pkg/v1beta1/deployment_runtime_config_types.go (1)
ObjectMeta(26-44)apis/protection/v1beta1/clusterusage_types.go (1)
ClusterUsage(40-46)
internal/controller/protection/usage/selector_test.go (2)
internal/protection/usage.go (4)
Usage(96-104)ResourceRef(30-38)ResourceSelector(41-53)Resource(56-69)internal/protection/wrappers.go (1)
InternalUsage(29-31)
internal/protection/conversion.go (2)
internal/protection/usage.go (3)
ResourceRef(30-38)ResourceSelector(41-53)Resource(56-69)apis/protection/v1beta1/usage_types.go (1)
NamespacedResource(100-118)
internal/protection/wrappers.go (2)
internal/protection/usage.go (2)
Usage(96-104)Resource(56-69)apis/protection/v1beta1/clusterusage_types.go (1)
ClusterUsage(40-46)
internal/controller/pkg/revision/dependency_test.go (4)
internal/dag/pkg.go (2)
DependencyNode(29-31)PackageNode(49-51)apis/pkg/v1beta1/lock.go (2)
Dependency(89-113)LockPackage(36-66)apis/pkg/meta/v1/meta.go (1)
Dependency(44-80)internal/dag/dag.go (1)
Node(25-38)
internal/controller/protection/usage/reconciler.go (2)
internal/protection/usage.go (1)
ResourceRef(30-38)apis/protection/v1beta1/usage_types.go (1)
ResourceRef(27-30)
internal/controller/pkg/revision/fuzz_test.go (1)
internal/dag/pkg.go (1)
PackageNode(49-51)
internal/controller/pkg/resolver/reconciler_test.go (4)
internal/dag/pkg.go (1)
DependencyNode(29-31)apis/pkg/v1beta1/lock.go (1)
Dependency(89-113)apis/pkg/meta/v1/meta.go (1)
Dependency(44-80)internal/dag/dag.go (1)
Node(25-38)
internal/webhook/protection/usage/handler.go (3)
internal/protection/usage.go (1)
ResourceRef(30-38)apis/protection/v1beta1/usage_types.go (1)
ResourceRef(27-30)apis/apiextensions/v1beta1/usage_types.go (1)
ResourceRef(26-29)
internal/protection/wrappers_test.go (2)
internal/protection/usage.go (1)
Usage(96-104)internal/protection/wrappers.go (3)
InternalUsage(29-31)InternalClusterUsage(96-98)InternalLegacyUsage(163-165)
internal/dag/pkg.go (2)
internal/dag/dag.go (1)
Node(25-38)apis/pkg/v1beta1/lock.go (2)
Dependency(89-113)LockPackage(36-66)
🔍 Remote MCP
I'll search for relevant context about this PR's goals and related changes to help with an effective review.Let me search for more specific information about the internal DAG interfaces and wrapper architecture referenced in this PR.Based on my research, I have gathered sufficient context to provide a comprehensive summary for PR review. Let me now compile the findings.
Summary of Additional Context Found
PR Architecture & Design Goals
This PR achieves a significant architectural milestone toward making the apis package into a separate Go module. The changes implement a clean separation between public API definitions and internal implementation concerns:
Key Refactoring Pattern:
- Removed public interface implementations from
apis/package directories - Moved interface implementations into
internal/packages - Created wrapper types (
InternalUsage,InternalClusterUsage,InternalLegacyUsage,DependencyNode,PackageNode) that encapsulate API types and provide implementation details - Introduced lazy factory-based instantiation patterns where reconcilers receive function closures instead of direct instances
Two Major Changes
1. DAG (Directed Acyclic Graph) Logic Migration:
- Previously: DAG node conversion logic (
ToNodes(),Neighbors(),AddNeighbors()) exposed in publicapis/pkg/v1beta1/lock.go - Now: Wrapped in new internal types (
internal/dag/DependencyNode,internal/dag/PackageNode) with a factory function (PackagesToNodes) - Impact: Controllers now own DAG construction logic rather than exposing it through the public API
- This aligns with the principle that package resolution and graph construction are implementation details
2. Usage Interface Implementations Migration:
- Previously: Public interface method implementations directly on API types (e.g.,
GetUserOf(),SetUserOf(),GetCondition()in public API files) - Now: Wrapper types in
internal/protection/wrappers.goimplement the interface withUnwrap()method for accessing the underlying Kubernetes object - Conversion logic: Uses generated converter types (
GeneratedResourceConverter,GeneratedLegacyResourceConverter,GeneratedNamespacedResourceConverter) - Reconcilers now: Receive factory functions (
func() protection.Usage) that return wrapped instances, which are unwrapped before Kubernetes API calls
Testing Coverage
The PR includes comprehensive test updates:
- Unit tests for wrapper types in
internal/protection/wrappers_test.go - Test reconciler mocks updated to use new wrapper types
- DAG node tests updated to validate the new
DependencyNodeandPackageNodewrappers - Selection/resolver tests refactored to work with the wrapper abstraction
Backward Compatibility & Breaking Changes
This is a breaking change for external consumers:
- Public methods removed from
apis/protection/v1beta1/{usage,clusterusage}_interface.go - Public methods removed from
apis/pkg/v1beta1/lock.go - Replacement condition methods added to public API types in
conditions.go(delegating to Status) - DAG conversion methods no longer available from the public API
The PR appears suitable for v2.x of Crossplane (which can accommodate breaking changes), aligning with a deliberate architectural decision to keep API and controller implementation concerns separate.
Code Generation & Conversion
The PR updates code generation directives in generate.go to point to internal/protection instead of apis/protection/v1beta1, meaning future code generation targets the internal package rather than the public API.
[::web_search::]
🔇 Additional comments (98)
internal/controller/pkg/revision/fuzz_test.go (1)
69-69: LGTM!Thank you for updating the fuzz test to align with the new DAG wrapper types. Since
dag.PackageNodeembedsv1beta1.LockPackage, the fuzzer'sGenerateStructcall will correctly populate the embedded fields, and the mock will return the expected type for the refactored reconciler code.generate.go (2)
52-52: Thanks for adding deepcopy generation for internal/protection.This ensures the new internal wrapper types (
InternalUsage,InternalClusterUsage, etc.) get proper deepcopy methods generated, which is necessary since they're now the types implementing theUsageinterface.
65-65: Goverter target correctly updated to internal/protection.The conversion code generation now targets the internal package where the
ResourceConverterandNamespacedResourceConverterinterfaces are defined. This aligns with the broader refactoring to keep implementation concerns internal.internal/protection/conversion.go (2)
29-38: Converter interfaces properly bridge v1beta1 and internal types.The bidirectional conversion between the public API types and internal representations looks good. I noticed the
// goverter:ignore Namespacedirectives onToInternalResourceRefandToInternalResourceSelector.Could you help me understand the reasoning here? Looking at the type definitions, the internal
ResourceRefhas aNamespace *stringfield whilev1beta1.ResourceRefappears to only haveName. Is the namespace being populated through a different path, or is this intentional for cluster-scoped resources? Just want to make sure I understand the conversion semantics correctly.
40-49: NamespacedResourceConverter updated consistently.Thank you for updating the output package directive and conversion methods to align with the internal/protection package. The interface correctly converts
v1beta1.NamespacedResourceto the internalResourcetype, which makes sense given the structural similarities between the types.internal/dag/pkg.go (5)
23-26: Nice use of compile-time interface checks.These assertions ensure
DependencyNodeandPackageNodecorrectly implement theNodeinterface at build time. Thank you for including these!
53-61: Neighbors() correctly wraps dependencies.The implementation properly creates
DependencyNodewrappers for each dependency in the package. This aligns with the DAG traversal pattern where neighbors are discovered during resolution.
63-76: AddNeighbors propagates parent constraints correctly.The logic matches dependencies by identifier and propagates constraints to the provided nodes. This maintains the constraint tracking needed for version resolution in the DAG.
One small observation: the inner loop breaks after finding a match (line 70), which is correct assuming identifiers are unique within a package's dependencies.
78-86: Clean helper function for bulk conversion.
PackagesToNodesprovides a convenient way to convert lock packages to DAG nodes for initialization. This nicely encapsulates the wrapper creation that reconcilers need.
39-46: Clarify the counterintuitive semantics ofAddNeighborsforDependencyNode.The method propagates the current node's constraints to provided nodes rather than adding those nodes as neighbors to the current node. This is backwards from test implementations (
simpleNode,SimpleFuzzNode) which store nodes in the current node's neighbors map, and contradicts what the method name suggests.Additionally,
DependencyNode.Neighbors()returnsniland doesn't track neighbors at all. Could you clarify whether this semantic difference from the test implementations is intentional? If so, consider adding a comment explaining why constraint propagation is sufficient forDependencyNodebut not for other node types. Also note that the call pattern indag.goandupgrading_dag.gocallsAddNeighborsfollowed by a separateAddParentConstraints, which suggests the method might be incomplete for establishing the full edge relationship.internal/protection/usage.go (1)
96-103: Unwrap pattern cleanly decouples internal interface from public API types.The design here is excellent—replacing embedded interfaces with an
Unwrap()method that returnsconditions.ObjectWithConditionsis a clean way to maintain abstraction. The public API types (v1beta1.Usage,v1beta1.ClusterUsage, and legacy types) all provide the necessary capabilities through their embeddedmetav1.ObjectMeta(for object metadata and identity) andxpv1.ConditionedStatus(for condition management). The wrapper implementations confirm the pattern is working as intended, and the actual usage in the webhook handler and reconciler validates that reconcilers have everything they need.internal/controller/pkg/resolver/reconciler_test.go (24)
348-352: LGTM - Consistent wrapper pattern applied.The transition from raw
v1beta1.Dependencytodag.DependencyNodewrapper is correctly applied here. The embedded struct pattern allows direct field access while providing the wrapper abstraction needed for the DAG interface.
395-400: Wrapper pattern consistently applied for error test case.Thanks for maintaining consistency across all the MockDag initializations - this makes the test suite much easier to follow.
445-450: Consistent with earlier changes.
495-500: Wrapper usage correct for version constraint test.
546-552: Type field correctly included in wrapper.The
Typefield is properly set within the embeddedDependencystruct, maintaining the same semantics as before.
598-604: Digest constraint test case properly updated.
650-656: Successful creation test case correctly wrapped.
715-721: Tag selection test maintains correct wrapper structure.
767-773: Rewrite test case correctly updated.
823-829: Digest-based dependency creation test properly wrapped.
879-900: MockGetNode correctly returns wrapped DependencyNode with ParentConstraints.I appreciate that you've ensured the
ParentConstraintsfield is correctly nested within theDependencystruct inside the wrapper. This maintains the same semantics while adapting to the new architecture.
947-969: Rewrite with GetNode test properly structured.
1011-1034: Digest update test with duplicate digests in ParentConstraints.The test correctly verifies behavior when the same digest appears multiple times in
ParentConstraints.
1076-1084: TestFindDigestToUpdate correctly uses wrapper type.The test input is properly updated to use
dag.DependencyNodewrapper.
1093-1101: Different digests error case properly wrapped.
1110-1115: All versions test case correctly structured.
1125-1133: Mixed constraint types error case properly wrapped.
1178-1186: TestReconcilerFindDependencyVersionToUpgrade uses wrapper correctly.
1197-1205: Mixed parent constraints error test properly structured.
1216-1223: Version upgrade test case correctly wrapped.
1240-1247: No valid version error test properly updated.
1264-1271: Downgrade not enabled error test correctly wrapped.
1288-1295: Upgrade to smallest valid version test properly structured.
1313-1320: Downgrade to largest valid test case correctly wrapped.All test cases in this file have been consistently updated to use the
dag.DependencyNodewrapper type. The changes are mechanical and correct.internal/protection/wrappers.go (9)
1-2: Build tag correctly excludes goverter.The
//go:build !govertertag ensures this file is excluded when goverter code generation runs, which makes sense since these are handwritten wrapper implementations.
28-35: InternalUsage wrapper correctly implemented.The wrapper embeds
v1beta1.Usageand theUnwrap()method correctly returns a pointer to the embedded struct, satisfying theconditions.ObjectWithConditionsinterface.
37-47: GetUserOf/SetUserOf use correct converter for namespaced resources.
InternalUsage.Spec.Ofis aNamespacedResource, so usingGeneratedNamespacedResourceConverteris correct here.
49-71: GetUsedBy/SetUsedBy properly handle nil with correct converter.The nil checks in both methods are appropriate. The
Byfield usesGeneratedResourceConverterwhich aligns withv1beta1.Resource(cluster-scoped reference).
73-93: Reason and ReplayDeletion accessors are straightforward pass-throughs.These simple getter/setter methods correctly delegate to the underlying spec fields.
95-114: InternalClusterUsage uses GeneratedResourceConverter for both Of and By.This is correct since
ClusterUsage.Spec.Ofis aResource(not namespaced), unlike the namespacedUsage.
116-160: InternalClusterUsage remaining methods follow same pattern.The GetUsedBy/SetUsedBy nil handling and Reason/ReplayDeletion accessors are consistent with InternalUsage.
162-181: InternalLegacyUsage correctly uses GeneratedLegacyResourceConverter.This maintains compatibility with the legacy
apiextensions/v1beta1.Usagetype while implementing the same interface.
183-227: InternalLegacyUsage implementation complete and consistent.All three wrapper types follow the same structural pattern, which aids maintainability. The converter selection is appropriate for each type's field definitions.
internal/controller/protection/usage/selector.go (2)
57-65: Correct use of Unwrap() for API operations.The change to pass
u.Unwrap()to bothresolveSelectorandclient.Updateis correct. The wrapper (protection.Usage) provides the business logic interface, whileUnwrap()exposes the underlying Kubernetes object needed for:
resolveSelector- which needsmetav1.Objectto access namespace informationclient.Update- which needsclient.Objectfor the Kubernetes API callThis cleanly separates the internal interface from the API types.
73-81: Consistent pattern for "by" resource resolution.The same unwrap pattern is correctly applied to the "by" (using) resource resolution path. The symmetry with the "of" path above makes the code easy to follow.
internal/protection/wrappers_test.go (1)
17-23: Clean compile-time interface verification.This is the idiomatic Go pattern for ensuring types implement an interface at compile time. By asserting all three internal wrapper types implement
Usage, you'll catch any interface drift immediately during compilation rather than at runtime.internal/webhook/protection/usage/handler_test.go (4)
199-223: Test correctly updated to use InternalUsage wrapper.The nested structure
protection.InternalUsage{ Usage: v1beta1.Usage{...} }is correct. The expected response message on line 248 references*v1beta1.Usagewhich aligns with whatUnwrap()returns - this is the type that will be formatted in error messages.
264-281: Reason field test case properly wrapped.The
Reason: &protectedfield is correctly placed withinv1beta1.UsageSpecinside the wrapper. The expected message on line 306 correctly shows the reason string.
322-338: Minimal usage (no By/Reason) test case correctly structured.This test verifies behavior when neither
BynorReasonis specified on the usage. The wrapper is correctly applied.
243-249: Confirmed: type string formatting is correct.Your analysis is spot-on. The handler at line 165 calls
uu := first.Unwrap(), then usesfmt.Sprintf(..., %T, uu, ...)(lines 174, 178, 182) to format the message. SinceUnwrap()returns a pointer tov1beta1.Usage, the%Tformat verb correctly produces*v1beta1.Usagein the message. The test expectations at lines 243–249, 301–307, and 358–364 accurately match this behavior. Thanks for double-checking this!internal/webhook/protection/usage/handler.go (1)
165-182: LGTM! Nice improvement for user-facing messages.Thank you for this change! Using
Unwrap()to get the underlying object for message formatting is a solid approach. The%Tplaceholder will now show the actual API type (e.g.,*v1beta1.Usage) rather than the internal wrapper type, which gives users more meaningful context in error messages.Could you confirm that this is the intended behavior - showing the concrete API type in user-facing messages?
internal/protection/usage/finder_test.go (1)
157-167: LGTM! Test expectations correctly updated for wrapper types.The test expectations now properly reflect the new wrapper-based architecture. The initialization pattern
&protection.InternalUsage{Usage: v1beta1.Usage{...}}correctly constructs the expected wrapper instances with embedded API types.internal/protection/usage/finder.go (3)
122-124: LGTM! This also addresses the loop variable aliasing concern.The wrapper approach
&protection.InternalUsage{Usage: u}copies the loop variable's value into the struct, which is safer than taking the address of a loop variable directly. This pattern is consistent across all three usage types.
131-133: LGTM!Consistent wrapper pattern for ClusterUsage.
140-142: LGTM!Consistent wrapper pattern for legacy Usage, with appropriate
staticchecksuppression preserved.internal/controller/protection/usage/selector_test.go (11)
33-34: LGTM!Import added for the protection package to access wrapper types.
41-48: LGTM! Good use of the interface type.Changing from concrete
*v1beta1.Usagetoprotection.Usageinterface type forargs.uandwant.uproperly abstracts the test to work with the wrapper-based architecture. This aligns with the broader refactoring goals.
59-120: LGTM!Test case "AlreadyResolved" properly updated to use the
InternalUsagewrapper for both input and expected output.
126-164: LGTM!Test case "AlreadyResolvedNoUsing" properly updated with wrapper types.
174-188: LGTM!Error case "CannotResolveUsedListError" updated consistently.
202-223: LGTM!Error case "CannotResolveUsingListError" updated consistently.
257-271: LGTM!Error case "CannotUpdateAfterResolvingUsed" updated consistently.
305-326: LGTM!Error case "CannotUpdateAfterResolvingUsing" updated consistently.
340-354: LGTM!Error case "CannotResolveNoMatchingResources" updated consistently.
389-404: LGTM!Error case "CannotResolveNoMatchingResourcesWithControllerRef" updated consistently.
468-548: LGTM!Test case "BothSelectorsResolved" - the most comprehensive test case - is properly updated with wrapper types for both input and expected output, including owner references and status fields.
internal/controller/protection/usage/reconciler.go (18)
105-110: LGTM! Factory pattern is a clean approach.Using
func() protection.Usage { return &protection.InternalUsage{} }allows the reconciler to create fresh instances without knowing the concrete type. This nicely decouples the reconciler from the specific wrapper types.
122-127: LGTM!Consistent factory pattern for ClusterUsage setup.
140-145: LGTM!Consistent factory pattern for legacy Usage setup.
216-218: LGTM! Clear documentation and signature.The updated signature
func NewReconciler(mgr manager.Manager, u func() protection.Usage, f Finder, opts ...ReconcilerOption) *Reconcilerclearly communicates the factory pattern expectation.
233-233: LGTM!Field assignment
newUsage: ucorrectly captures the factory function.
257-257: LGTM!Struct field declaration matches the factory function type.
277-282: LGTM! Clear separation between wrapper and unwrapped objects.The pattern of:
u := r.newUsage()- create wrapper via factoryuu := u.Unwrap()- get underlying K8s objectr.client.Get(ctx, req.NamespacedName, uu)- use unwrapped for K8s operationsThis is intuitive. The wrapper (
u) provides the abstracted interface methods, while the unwrapped object (uu) is what gets passed to the Kubernetes client.
293-293: LGTM!
orig := uu.DeepCopyObject()correctly captures the initial state of the unwrapped object for later comparison.
295-301: LGTM!The selector resolution still operates on the wrapper
u(which implements the interface), while events are recorded againstuu(the actual K8s object).
303-312: LGTM!Status conditions and resource references correctly use
uufor namespace lookups.
314-351: LGTM!Deletion handling path properly uses
uufor metadata checks (labels, namespace) and event recording.
360-393: LGTM!The "used resource" cleanup path correctly operates on
uufor events.
396-427: LGTM!Replay deletion and finalizer removal correctly use
uu. The async goroutine at line 401 appropriately uses theusedresource (not the usage) for deletion replay.
432-444: LGTM!Finalizer addition uses
uucorrectly.
446-464: LGTM!Details annotation logic correctly accesses and mutates
uu.
466-493: LGTM!"Used resource" handling with in-use label correctly references
uu.GetUID()for ownership check.
496-532: LGTM!"Using resource" handling correctly gets namespace from
uuand adds owner reference touu.
540-543: LGTM! Status update path is correct.The comparison
!cmp.Equal(uu, orig)correctly compares the potentially-modified unwrapped object against its original state. The status update and event recording both correctly useuu.internal/controller/pkg/revision/dependency.go (4)
137-140: LGTM on the DAG initialization migration.The switch from
v1beta1.ToNodestodag.PackagesToNodescleanly moves the node conversion logic into the internal package, which aligns nicely with the PR's goal of eliminating internal package imports from the APIs. Thanks for keeping the call site so clean!
195-195: LGTM on the PackageNode wrapping.Wrapping
selfin&dag.PackageNode{LockPackage: self}is the right approach here sincePackageNodeembedsLockPackageand implements thedag.Nodeinterface. This keeps the DAG interface implementation internal while maintaining access to all the underlying fields.
207-207: LGTM on the DependencyNode wrapping for missing dependencies.Wrapping
depin&dag.DependencyNode{Dependency: dep}correctly satisfies the[]dag.Nodetype for themissingslice, and the embeddedDependencyensures theIdentifier()andGetConstraints()methods will work as expected downstream.
247-250: LGTM on the type assertion update.The type assertion to
*dag.PackageNodeis correct, and sincePackageNodeembedsv1beta1.LockPackage, the subsequent accesses tolp.Versionandlp.Identifier()will work transparently through Go's embedding mechanism. Nice work keeping backward compatibility in the logic flow!internal/controller/pkg/resolver/reconciler.go (4)
254-262: LGTM on the DAG initialization.Consistent with the changes in
dependency.go, this correctly migrates tointernaldag.PackagesToNodes. The error handling and condition marking remain intact.
310-310: LGTM on accessing the embedded Dependency.Using
&dep.Dependencyto extract a pointer to the embeddedv1beta1.DependencyfromDependencyNodeis the correct approach here. This maintains compatibility withNewPackageList's signature while working with the new wrapper type.
356-356: LGTM on the embedded Dependency access for version resolution.Consistent pattern with line 310 - correctly passing
&dep.DependencytofindDependencyVersionToInstall.
374-374: LGTM on the embedded Dependency access for package construction.Same pattern applied consistently for
NewPackage. The changes throughout this file maintain a clean separation between the internal wrapper type and the API type.internal/controller/pkg/revision/dependency_test.go (5)
282-294: LGTM on the MockInit update.The test correctly wraps dependencies in
&dag.DependencyNode{Dependency: v1beta1.Dependency{...}}to match the production code's expectations. The pattern is clear and consistent.
296-302: LGTM on the MockTraceNode update.Using empty
&dag.DependencyNode{}as placeholder values in the trace map is appropriate here since the test is checking for node presence rather than specific content. Clean and minimal.
378-403: LGTM on the MockGetNode and MockTraceNode updates for invalid dependencies test.The mocks correctly return
*dag.PackageNodewrappers, which aligns with the production code's type assertion at line 247 independency.go. TheSourceandVersionfields are properly populated for the version compatibility checks.
489-525: LGTM on the MockTraceNode and MockGetNode updates for valid dependencies test.The mocks are correctly updated to use the wrapper types, and I appreciate that the function package type (
function-not-here-1) is also covered in this test case. Good coverage!
595-600: LGTM on the MockTraceNode update for source mismatch test.Consistent wrapper usage pattern. The test coverage for the source mismatch scenario is well maintained with these updates.
3 participants
Description of your changes
As the first step toward making
apisa separate go module (#6998), restructure a couple of areas so that we don't need to import any internal packages in apis (since packages can't import internal packages from other modules). There are two changes:pkg: Move DAG logic out of the apis package
Implement the DAG interfaces for package types by adding wrappers in
internal/controller/pkg, since the controllers are the only part of the code that constructs a DAG of packages. The DAG is an internal Crossplane concern that shouldn't really be exposed through the API anyway.usage: Move usage interface implementations into internal
The usage interface exists to let us implement a single reconciler for multiple usage types. Implementing the interface is an internal concern, so doesn't really belong in apis. Move the interface implementations into
internal/protectionby introducing wrapper types for each of the usage API types.I have:
earthly +reviewableto ensure this PR is ready for review.- [ ] Added or updated e2e tests.- [ ] Linked a PR or a docs tracking issue to document this change.- [ ] Addedbackport release-x.ylabels to auto-backport this PR.- [ ] Followed the API promotion workflow if this PR introduces, removes, or promotes an API.