Skip to content

Conversation

@erhancagirici
Copy link
Contributor

@erhancagirici erhancagirici commented Aug 5, 2025

Description of your changes

Adds LegacyTracker and ModernTracker interfaces for capturing new provider config usage pattern.
These would help also in testing.

I have:

@erhancagirici erhancagirici marked this pull request as ready for review October 27, 2025 08:56
@erhancagirici erhancagirici requested a review from a team as a code owner October 27, 2025 08:56
@erhancagirici erhancagirici requested a review from negz October 27, 2025 08:56
@coderabbitai
Copy link

coderabbitai bot commented Oct 27, 2025

📝 Walkthrough

Summary by CodeRabbit

  • New Features
    • Added new resource tracking abstractions to provide enhanced flexibility and extensibility for resource management operations.

Walkthrough

Adds two new tracker interfaces, LegacyTracker and ModernTracker, plus their function-adapter types LegacyTrackerFn and ModernTrackerFn with Track(ctx, mg) methods to allow functional adapters for tracking legacy and modern managed resources.

Changes

Cohort / File(s) Summary
New Tracker Abstractions
pkg/resource/providerconfig.go
Added LegacyTracker and ModernTracker interfaces with `Track(ctx context.Context, mg LegacyManaged

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Small, localized API additions with a consistent adapter pattern.
  • Areas to spot-check: method signatures for correct managed types, import/namespace consistency, and any tests or call sites needing updates.

Thank you for the clear PR — would you like me to scan for call sites that could adopt these adapters or generate small usage examples?

Pre-merge checks

✅ Passed checks (3 passed)
Check name Status Explanation
Title Check ✅ Passed The pull request title "v2: add legacy and modern tracker interfaces for providerconfig usages" directly summarizes the main changes in the changeset, which adds LegacyTracker and ModernTracker interfaces along with their corresponding function types to pkg/resource/providerconfig.go. The title is descriptive and specific about what is being added and where, and at 70 characters, it meets the requirement of staying under 72 characters.
Description Check ✅ Passed The pull request description clearly describes the changes made, stating that it "Adds LegacyTracker and ModernTracker interfaces for capturing new provider config usage pattern" and mentions benefits for testing. This directly relates to the changeset, which introduces these two new interfaces and their corresponding function types in the providerconfig file. The description is relevant and on-topic, making it appropriate for the lenient evaluation criteria of this check.
Breaking Changes ✅ Passed This PR contains no breaking changes. It exclusively adds new public interfaces (LegacyTracker and ModernTracker) along with corresponding function types (LegacyTrackerFn and ModernTrackerFn) that mirror the existing Tracker pattern. All existing exported symbols remain unchanged—no removals, renames, signature changes, or behavioral modifications. Adding new APIs in Go is backwards-compatible and does not break existing code that depends on the current interfaces.

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between bf01e9c and cc51946.

📒 Files selected for processing (1)
  • pkg/resource/providerconfig.go (1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/!(*_test).go

📄 CodeRabbit inference engine (coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt)

**/!(*_test).go: Do not remove exported Go APIs (functions, types, methods, or fields) without the 'breaking-change' label
Do not rename exported Go APIs without the 'breaking-change' label
Do not change function/method signatures of exported Go APIs without the 'breaking-change' label
Avoid behavior changes to exported Go APIs that could break existing users unless labeled 'breaking-change'

Files:

  • pkg/resource/providerconfig.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:

  • pkg/resource/providerconfig.go
🧬 Code graph analysis (1)
pkg/resource/providerconfig.go (2)
pkg/resource/interfaces.go (2)
  • LegacyManaged (219-224)
  • ModernManaged (209-213)
pkg/resource/fake/mocks.go (2)
  • LegacyManaged (398-405)
  • ModernManaged (370-376)
🔍 Remote MCP

I'm encountering authentication issues with the Context7 tools that prevent me from retrieving documentation about the crossplane-runtime library. However, let me search for publicly available information about this PR and the patterns being implemented:Let me search for more specific information about the existing Tracker pattern and the relationship between LegacyManaged and ModernManaged types:## Summary of Relevant Context for PR #862 Review

Based on my research, here is the additional context relevant to reviewing this PR:

Existing Tracker Pattern

The crossplane-runtime already defines a Tracker interface with a Track(ctx context.Context, mg Managed) error method that tracks managed resources. There is also a TrackerFn function type for tracking managed resources. The PR's new interfaces (LegacyTracker, ModernTracker) mirror this established pattern, applying it to legacy and modern managed resource variants.

Legacy vs. Modern Managed Resources Context

Crossplane v2 supports two styles of managed resources: Modern v2 style (recommended) using *.m.crossplane.io domains for namespaced managed resources with better isolation and security, and Legacy v1 style using *.crossplane.io domains for cluster-scoped managed resources maintained for backward compatibility. Cluster-scoped MRs will be legacy and exist only for v1 backward compatibility; however, support for cluster-scoped resources is needed for some time to help folks migrate to v2 APIs.

ProviderConfigUsage Tracking Relationship

Crossplane providers generate a ProviderConfigUsage for each managed resource (MR) they handle, which enables representing the relationship between MR and a ProviderConfig so that the controller can use it as a finalizer when you delete a ProviderConfig. The new tracker interfaces would support tracking different managed resource types through this usage mechanism.

Scope: This addition provides type-safe abstractions for tracking both legacy cluster-scoped and modern namespaced managed resources, following the established functional options pattern in crossplane-runtime for providerconfig usages.

[::web_search::]

🔇 Additional comments (3)
pkg/resource/providerconfig.go (3)

129-155: Overall design rationale question

The new interfaces provide a clean separation between legacy (cluster-scoped v1) and modern (namespaced v2) resource tracking, which aligns well with Crossplane's v2 migration story. The type safety and testability benefits are clear.

One question: Given that the concrete tracker implementations (LegacyProviderConfigUsageTracker and ProviderConfigUsageTracker) already exist with the correct signatures, can you share what specific use case or testing scenario motivated extracting these as explicit interfaces? Understanding the 'why' would help verify the design serves its intended purpose.

For example, are these interfaces primarily for:

  • Enabling mock implementations in provider tests?
  • Supporting alternative tracking strategies?
  • Facilitating controller composition?

143-155: Thank you for the detailed script output! I can now see the full picture. The implementation looks well-structured, but I need to flag an important gap:

Verify that ModernTracker types are intentionally added (not dead code) and check error messaging compliance.

While the design is solid—ProviderConfigUsageTracker correctly implements the ModernTracker interface at line 174—the verification reveals:

  1. No usage found: Ripgrep searches found ModernTracker and ModernTrackerFn only in their definitions (providerconfig.go), with no callers elsewhere in the codebase.

  2. Error messaging: The implementation uses error wrapping (lines 181, 185, 207) with constants like errMissingPCRef, errMissingPCRefKind, and errApplyPCU. Per the coding guidelines, error messages must be user-friendly with context and next steps—please confirm these constants provide that.

  3. Test coverage: No test evidence found for these new types.

Are these new types intended for future use / external API, or should they be added only when actually used? A quick pointer to the PR description or issue would clarify the intent.


129-141: Verify usage context and test coverage before merging.

The search found these interfaces are defined but shows no usage of LegacyTracker or LegacyTrackerFn elsewhere in the codebase, and no test files referencing them. This could indicate either:

  • These are newly added interfaces awaiting integration in follow-up work or other PRs
  • Tests are being added separately outside this review

Before merging, please confirm:

  1. Where (and in which PR/file) these interfaces will be used to satisfy their type-safe abstraction
  2. Whether tests have been added separately (and where they're located)
  3. Whether the existing LegacyProviderConfigUsageTracker.Track (line 228) is already designed to implement this interface

The code itself is sound and follows the established pattern—I want to ensure the broader context justifies their introduction now.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (2)
pkg/resource/providerconfig.go (2)

135-136: Refine the documentation for specificity.

The comment says "tracks managed resources" but should say "tracks legacy managed resources" for consistency with the Track method comment on line 138 and to clearly distinguish this from the generic TrackerFn.

Apply this diff:

-// A LegacyTrackerFn is a function that tracks managed resources.
+// A LegacyTrackerFn is a function that tracks legacy managed resources.
 type LegacyTrackerFn func(ctx context.Context, mg LegacyManaged) error

149-152: Refine the documentation for specificity.

The comments should be more specific:

  • Line 149 says "tracks managed resources" but should say "tracks modern managed resources"
  • Line 152 says "Track the supplied managed resource" but should say "Track the supplied modern managed resource"

This improves consistency with the LegacyTrackerFn documentation and clearly distinguishes this from the generic TrackerFn.

Apply this diff:

-// A ModernTrackerFn is a function that tracks managed resources.
+// A ModernTrackerFn is a function that tracks modern managed resources.
 type ModernTrackerFn func(ctx context.Context, mg ModernManaged) error
 
-// Track the supplied managed resource.
+// Track the supplied modern managed resource.
 func (fn ModernTrackerFn) Track(ctx context.Context, mg ModernManaged) error {
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 979520c and bf01e9c.

📒 Files selected for processing (1)
  • pkg/resource/providerconfig.go (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/!(*_test).go

📄 CodeRabbit inference engine (coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt)

**/!(*_test).go: Do not remove exported Go APIs (functions, types, methods, or fields) without the 'breaking-change' label
Do not rename exported Go APIs without the 'breaking-change' label
Do not change function/method signatures of exported Go APIs without the 'breaking-change' label
Avoid behavior changes to exported Go APIs that could break existing users unless labeled 'breaking-change'

Files:

  • pkg/resource/providerconfig.go
🧬 Code graph analysis (1)
pkg/resource/providerconfig.go (2)
pkg/resource/interfaces.go (2)
  • LegacyManaged (219-224)
  • ModernManaged (209-213)
pkg/resource/fake/mocks.go (2)
  • LegacyManaged (398-405)
  • ModernManaged (370-376)

@erhancagirici erhancagirici force-pushed the update-tracker-interfaces branch from bf01e9c to cc51946 Compare October 27, 2025 09:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant