Skip to content

Use core APIs from the Crossplane apis module#940

Open
adamwg wants to merge 1 commit intocrossplane:mainfrom
adamwg:awg/api-updates
Open

Use core APIs from the Crossplane apis module#940
adamwg wants to merge 1 commit intocrossplane:mainfrom
adamwg:awg/api-updates

Conversation

@adamwg
Copy link
Member

@adamwg adamwg commented Mar 6, 2026

Description of your changes

This PR brings back the changes from crossplane/crossplane#7190. We now depend on the Crossplane apis module for core APIs, and it does not depend on crossplane-runtime.

I have:

This commit brings back the changes from crossplane/crossplane#7190. We now
depend on the Crossplane `apis` module for core APIs, and it does not depend on
crossplane-runtime.

Signed-off-by: Adam Wolfe Gordon <awg@upbound.io>
@adamwg adamwg requested a review from a team as a code owner March 6, 2026 22:37
@adamwg adamwg requested a review from bobh66 March 6, 2026 22:37
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 6, 2026

📝 Walkthrough

Walkthrough

This PR consolidates Crossplane API types by removing deprecated v1/v2 re-export wrappers from the runtime package and migrating all internal code to directly use v2 core APIs. Additionally, it introduces a new changelog service definition with message types for recording resource operations. The changes represent a systematic transition from local API copies to upstream v2 API dependencies.

Changes

Cohort / File(s) Summary
Changelog Proto Definitions
apis/changelogs/proto/v1alpha1/changelog.proto, apis/proto/v1alpha1/ess.proto
Adds new proto service and message types for changelog recording (ChangeLogRequest, ChangeLogEntry with operation tracking, timestamp, and resource metadata). Reformats license headers and adjusts whitespace in proto files.
Common API Removal
apis/common/condition.go, apis/common/condition_test.go, apis/common/merge.go, apis/common/merge_test.go, apis/common/observation.go, apis/common/policies.go, apis/common/resource.go, apis/common/doc.go
Deletes core common package types including Condition/ConditionedStatus, MergeOptions, ObservedStatus, policy enums (ManagementPolicies, DeletionPolicy, UpdatePolicy), and resource reference types (Reference, TypedReference, SecretReference, ProviderConfigReference). Removes 897 lines of deprecated APIs.
v1 API Package Cleanup
apis/common/v1/condition.go, apis/common/v1/doc.go, apis/common/v1/merge.go, apis/common/v1/observation.go, apis/common/v1/policies.go, apis/common/v1/resource.go
Removes v1 package re-exports and aliases that proxied to common package types. Eliminates 441 lines of wrapper declarations and convenience constructors.
v2 API Package Cleanup
apis/common/v2/doc.go, apis/common/v2/resource.go
Removes v2 package definitions (ManagedResourceSpec, TypedProviderConfigUsage alias) totaling 70 lines.
Fieldpath Merge Refactor
pkg/fieldpath/merge.go, pkg/fieldpath/merge_test.go
Extracts MergeOptions type locally in fieldpath package, removing dependency on external xpv1.MergeOptions. Introduces new mergoConfiguration() and isAppendSlice() methods. Updates MergeValue and merge function signatures to accept local MergeOptions struct.
Condition Manager Migration
pkg/conditions/manager.go, pkg/conditions/manager_test.go
Updates ConditionSet interface and observedGenerationPropagationConditionSet to use xpv2.Condition instead of xpv1.Condition. Updates test fixtures and condition types accordingly.
Reference Resolution v1→v2
pkg/reference/reference.go, pkg/reference/reference_test.go, pkg/reference/namespaced_reference.go, pkg/reference/namespaced_reference_test.go
Migrates ResolutionRequest/ResolutionResponse and MultiResolutionRequest/Response struct fields from xpv1 types (Reference, Selector, Policy) to xpv2 equivalents. Updates resolver logic and test expectations to construct and propagate v2 references.
Metadata & Provider Config v1→v2
pkg/meta/meta.go, pkg/meta/meta_test.go, pkg/reconciler/providerconfig/reconciler.go
Updates TypedReferenceTo, AsOwner, AsController functions and ProviderConfig reconciler to use xpv2.TypedReference and v2 label keys (LabelKeyProviderName, LabelKeyProviderKind). Updates condition types to xpv2.
Managed Reconciler Policies v1→v2
pkg/reconciler/managed/policies.go, pkg/reconciler/managed/reconciler.go
Migrates ManagementPoliciesResolver and LegacyManagementPoliciesResolver struct fields, function signatures, and policy decision logic from xpv1.ManagementAction/DeletionPolicy to xpv2 equivalents. Updates all policy validation and action comparison methods.
Managed Reconciler Tests v1→v2
pkg/reconciler/managed/api_test.go, pkg/reconciler/managed/metrics.go, pkg/reconciler/managed/reconciler_legacy_test.go, pkg/reconciler/managed/reconciler_modern_test.go
Updates test setup and assertions across managed resource reconciler tests to use xpv2 types for policies (ManagementPolicies, DeletionPolicy), conditions (TypeReady, TypeSynced), and secret references. Large-scale type migration affecting test fixtures and expected outcomes.
Resource Interface & Accessor v1→v2
pkg/resource/interfaces.go, pkg/resource/enqueue_handlers_test.go, pkg/resource/providerconfig.go, pkg/resource/providerconfig_test.go, pkg/resource/resource.go, pkg/resource/resource_test.go
Updates Conditioned, LocalConnectionSecretWriterTo, ConnectionSecretWriterTo, Manageable, ProviderConfigReferencer interfaces and credential extraction functions (ExtractEnv, ExtractFs, ExtractSecret, CommonCredentialExtractor) to use xpv2 types. Updates IsConditionTrue to accept xpv2.Condition. Adjusts test setup accordingly.
Unstructured Claim & Composite v1→v2
pkg/resource/unstructured/claim/claim.go, pkg/resource/unstructured/claim/claim_test.go, pkg/resource/unstructured/composed/composed.go, pkg/resource/unstructured/composed/composed_test.go, pkg/resource/unstructured/composite/composite.go, pkg/resource/unstructured/composite/composite_test.go
Migrates unstructured resource implementations to use xpv2 types in WithConditions, condition accessors, secret references, update/delete policies, and observed status handling. Updates all method signatures and internal status handling to v2 API surface. Updates test expectations and fixtures.
Metrics & Test Utilities v1→v2
pkg/statemetrics/mr_state_metrics.go, pkg/test/cmp.go, pkg/resource/predicates_test.go
Updates metrics condition type checks (TypeReady, TypeSynced) and test comparators (EquateConditions) from xpv1 to xpv2. Updates test helpers to reference v2 condition constructors.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

This review requires careful attention due to the large-scale migration from v1 to v2 APIs across 60+ files with heterogeneous change patterns (deletions, refactors, type migrations). While individual changes follow a consistent substitution pattern (xpv1→xpv2), the breadth of affected code paths, especially spanning reconcilers, resource abstractions, and reference resolution logic, demands verification that type migrations are complete and consistent throughout the codebase. The removal of shared API packages necessitates confirming all dependencies have been properly updated.


Important

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

❌ Failed checks (1 error)

Check name Status Explanation Resolution
Breaking Changes ❌ Error PR #940 removes entire public packages (apis/common, apis/common/v1, apis/common/v2) and modifies 14+ public function/method signatures without a breaking-change label. Add the 'breaking-change' label to acknowledge these breaking API changes and update the CHANGELOG with migration guidance.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The PR title clearly describes the main change: migrating from crossplane-runtime APIs to Crossplane apis module core APIs. It's concise (45 characters) and directly reflects the primary objective of the changeset.
Description check ✅ Passed The PR description is directly related to the changeset, explaining that it brings back changes to depend on the Crossplane apis module for core APIs instead of crossplane-runtime, which aligns with the migration of API types throughout the files.

Tip

Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs).
Share your feedback on Discord.


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
Contributor

@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: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (5)
apis/changelogs/proto/v1alpha1/changelog.proto (1)

71-73: ⚠️ Potential issue | 🟡 Minor

Minor grammar fix needed in the field comment.

The comment reads "An optional additional details" — could we update this to "Optional additional details" for grammatical correctness?

📝 Suggested fix
-  // An optional additional details that can be provided for further context
+  // Optional additional details that can be provided for further context
   // about the change.
   map<string, string> additional_details = 10;
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@apis/changelogs/proto/v1alpha1/changelog.proto` around lines 71 - 73, The
field comment for the map field additional_details is grammatically incorrect
("An optional additional details"); update the proto comment above the
map<string, string> additional_details = 10; to read "Optional additional
details" so the comment is concise and correct for the additional_details field
in the Changelog proto.
pkg/reference/reference_test.go (1)

8-8: ⚠️ Potential issue | 🟡 Minor

Typo in license header URL.

There's a small typo in the Apache License URL: htcp:// should be http://. This appears to be a pre-existing issue but worth fixing while you're in this file.

📝 Suggested fix
-    htcp://www.apache.org/licenses/LICENSE-2.0
+    http://www.apache.org/licenses/LICENSE-2.0
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@pkg/reference/reference_test.go` at line 8, Fix the typo in the license
header string "htcp://www.apache.org/licenses/LICENSE-2.0" by replacing
"htcp://" with "http://" so the URL reads
"http://www.apache.org/licenses/LICENSE-2.0"; locate the exact string in
reference_test.go and update it accordingly (ensure no other occurrences of
"htcp://" remain).
pkg/meta/meta.go (1)

88-117: ⚠️ Potential issue | 🟠 Major

Confirm the breaking-change label was applied to this PR, or clarify the migration approach.

This commit intentionally migrates three exported functions—TypedReferenceTo, AsOwner, and AsController—to use xpv2.TypedReference from the external Crossplane APIs module rather than their previous type. Since these are public APIs in a library repo, any downstream callers still compiled against the old type surface will fail to compile.

The commit message references crossplane/crossplane#7190 and indicates this is part of a deliberate API migration. However, without a breaking-change label or documented migration path (temporary shims, deprecated aliases, release notes), users will face an unplanned breaking upgrade.

Can you clarify: Was a breaking-change label intended? If so, please apply it. If there's a compatibility layer or migration guide in the upstream crossplane/crossplane PR, link it here to help users navigate this transition.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@pkg/meta/meta.go` around lines 88 - 117, The public API change migrates
TypedReferenceTo, AsOwner, and AsController to use xpv2.TypedReference which is
a breaking change for downstream consumers; either (A) mark this PR/release with
the breaking-change label and update the release notes to reference the
crossplane/crossplane#7190 migration guide, or (B) add a compatibility layer:
provide deprecated type alias or adapter functions that accept the previous
exported type and convert to xpv2.TypedReference (and update TypedReferenceTo,
AsOwner, AsController to use the adapter), plus clear deprecation comments and a
migration note in the changelog pointing to the upstream PR.
pkg/resource/providerconfig.go (1)

106-111: ⚠️ Potential issue | 🟡 Minor

Make the unsupported credentials-source error user-actionable.

Thanks for adding the fallback. Could we phrase this from the user's point of view? no extraction handler registered sounds like an internal wiring problem, but this path is reachable from user-configured sources like InjectedIdentity. A message that says credentials could not be loaded from the selected source, plus what the user should use instead, would be much easier to act on.

As per coding guidelines, "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."

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@pkg/resource/providerconfig.go` around lines 106 - 111, The fallback error
for unsupported credential sources (triggered in the
CredentialsSourceInjectedIdentity case and default branch, which currently
returns errors.Errorf(errNoHandlerForSourceFmt, source)) should be rewritten to
a user-actionable message: update the returned error to explain that credentials
could not be loaded from the selected source, name the provided source value,
and suggest next steps (e.g., configure a supported credentials source such as
environment variables or a provider-specific injected identity implementation).
Replace usages of errNoHandlerForSourceFmt with a clearer string and ensure the
error is returned from the same branch handling
CredentialsSourceInjectedIdentity/default so callers receive the actionable
guidance.
pkg/reconciler/managed/policies.go (1)

154-171: ⚠️ Potential issue | 🟡 Minor

Use sets.List() for deterministic error formatting.

The validation errors use UnsortedList() on a generic set type, which produces non-deterministic ordering. This can cause the same invalid spec to surface different condition text across reconciles, violating the requirement for stable/deterministic conditions.

Since sets.List[T cmp.Ordered]() returns a sorted slice, we should extract it once at the start of the function and reuse it in both error paths:

Suggested fix
 func (m *ManagementPoliciesResolver) Validate() error {
+	policies := sets.List(m.managementPolicies)
+
 	// check if its disabled, but uses a non-default value.
 	if !m.enabled {
 		if !m.managementPolicies.Equal(sets.New[xpv2.ManagementAction](xpv2.ManagementActionAll)) && m.managementPolicies.Len() != 0 {
-			return fmt.Errorf(errFmtManagementPolicyNonDefault, m.managementPolicies.UnsortedList())
+			return fmt.Errorf(errFmtManagementPolicyNonDefault, policies)
 		}
 		// if its just disabled we don't care about supported policies
 		return nil
 	}
 
-	return fmt.Errorf(errFmtManagementPolicyNotSupported, m.managementPolicies.UnsortedList())
+	return fmt.Errorf(errFmtManagementPolicyNotSupported, policies)
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@pkg/reconciler/managed/policies.go` around lines 154 - 171, The Validate
method in ManagementPoliciesResolver uses m.managementPolicies.UnsortedList(),
producing non-deterministic ordering; replace those calls by computing a
deterministic slice once (e.g., policiesList := sets.List(m.managementPolicies))
at the start of Validate and use policiesList in both error returns (the
errFmtManagementPolicyNonDefault and errFmtManagementPolicyNotSupported paths)
so the error text is stable across reconciles; update references to
UnsortedList() to use the single policiesList variable and ensure sets.List is
used on m.managementPolicies (type xpv2.ManagementAction) before any early
returns.
🧹 Nitpick comments (5)
apis/changelogs/proto/v1alpha1/changelog.proto (1)

63-65: Question: Could you share more about the snapshot field design decision?

I noticed this captures a "full snapshot of the resource's state" as a google.protobuf.Struct. This provides great flexibility, but for resources with many fields or nested structures, these entries could become quite large.

Is there guidance on what should be captured here? For instance:

  • Full spec + status, or just spec?
  • Any sensitive data filtering considerations?
  • Expected consumers of this data (debugging, audit trails, compliance)?

Just curious about the intended use case — this helps understand if there are any downstream considerations we should document. Thanks!

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@apis/changelogs/proto/v1alpha1/changelog.proto` around lines 63 - 65, Update
the comment for the google.protobuf.Struct field snapshot in changelog.proto to
document the intended design: specify that snapshot should capture the
resource's full observed state (recommend including spec and status but omit
derived/transient fields), mandate redaction/filtering of sensitive fields
(secrets, PII) before storing, note size/complexity limits or encourage
truncation/summary for deeply nested/large resources, and list expected
consumers (debugging, audit trails, compliance) and retention expectations; also
mention that google.protobuf.Struct is chosen for flexibility and call out
alternatives (typed snapshots or diffs) if structured schema is desired.
pkg/fieldpath/merge_test.go (1)

170-193: Map merge tests look good.

The KeepMapValues test coverage is solid, testing both false and true scenarios with clear expected outcomes. One thought - would it be valuable to add a test case that combines both KeepMapValues: true and AppendSlice: true to document the combined behavior? This could help clarify expectations for maintainers.

That said, the existing coverage is sufficient for the scope of this PR, so this is just a nice-to-have suggestion.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@pkg/fieldpath/merge_test.go` around lines 170 - 193, Add a new test case that
verifies combined behavior when MergeOptions has both KeepMapValues set to true
and AppendSlice set to true: create a test similar to "MergeMapKeep" but set mo:
&MergeOptions{KeepMapValues: &valTrue, AppendSlice: &valTrue}, use object:
fnMapDst(), value: fnMapSrc(), path: pathTest and assert the serialized output
via formatMap(...) to reflect that existing dst map values are preserved while
slice fields within map entries are appended; place it alongside the existing
MergeMap/Keep tests so maintainers can see combined behavior for MergeOptions.
pkg/reference/reference.go (1)

438-447: Consider using the generic sortGenericMapByKeys function.

I noticed there's a generic version sortGenericMapByKeys[T any] in namespaced_reference.go (line 286) that could replace this type-specific implementation. This would reduce code duplication within the package.

What do you think about consolidating these? Happy to help if you'd like to make that change!

♻️ Suggested refactor
-func sortMapByKeys(m map[string]xpv2.Reference) ([]string, []xpv2.Reference) {
-	keys := slices.Sorted(maps.Keys(m))
-
-	values := make([]xpv2.Reference, 0, len(keys))
-	for _, k := range keys {
-		values = append(values, m[k])
-	}
-
-	return keys, values
-}
+// sortMapByKeys can be replaced with sortGenericMapByKeys[xpv2.Reference]

Then update the call site at line 423:

-	sortedKeys, sortedRefs := sortMapByKeys(valueMap)
+	sortedKeys, sortedRefs := sortGenericMapByKeys(valueMap)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@pkg/reference/reference.go` around lines 438 - 447, The small helper
sortMapByKeys is duplicating functionality already implemented by the generic
sortGenericMapByKeys[T any] in namespaced_reference.go; remove sortMapByKeys and
replace its usages with calls to sortGenericMapByKeys[xpv2.Reference](m) (or the
explicit type parameter) so the package uses the single generic implementation
(update any call sites that previously referenced sortMapByKeys to call
sortGenericMapByKeys with the xpv2.Reference type).
pkg/resource/providerconfig_test.go (1)

270-303: Could we capture the applied usage object in these nil-returning cases?

Thanks for updating the fake refs. Most of the production delta in Track is the migrated label and provider-config reference wiring, but these cases only assert the returned error. Capturing the client.Object passed to ApplyFn in one legacy and one modern path would make this migration much harder to regress silently.

Also applies to: 365-397

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@pkg/resource/providerconfig_test.go` around lines 270 - 303, In the
"NopUpdate" test (and the analogous modern-path case later) capture the
client.Object that ApplyFn receives so the test can assert the applied object's
wiring; update the ApplyFn used in the test (the ApplyFn literal passed into
fields.c) to store the passed-in object to a test-scoped variable (e.g.,
appliedObj or appliedUsage) when invoked, then return the same nil-or-errBoom
behavior, and add assertions after the call that verify the captured object's
provider-config reference/labels are as expected; do the same change for the
corresponding modern-path test to ensure one legacy and one modern case record
the applied object for regression safety.
pkg/reconciler/managed/reconciler_legacy_test.go (1)

1866-1879: These cases still hit the All policy path, not the create/update-specific paths their names describe.

Could we clarify the intent here? Both setups duplicate the existing ManagementPolicyAll* coverage above, so after the xpv2 migration these names still imply create/update-specific behavior that the test data doesn't actually exercise. If All is intentional, renaming or removing the duplicate cases would avoid false confidence; otherwise the setup likely needs different policy inputs.

As per coding guidelines, "Check for proper test case naming and reason fields."

Also applies to: 2011-2020

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@pkg/reconciler/managed/reconciler_legacy_test.go` around lines 1866 - 1879,
The test cases in reconciler_legacy_test.go are setting ManagementPolicies to
xpv2.ManagementActionAll (see MockGet -> asLegacyManaged and MockStatusUpdate ->
newLegacyManaged with SetManagementPolicies), so they actually exercise the
"All" policy path rather than create/update-specific behavior implied by the
test names; either rename/remove these duplicate cases to reflect they cover the
All policy, or change the policy inputs in those cases from
xpv2.ManagementActionAll to the appropriate xpv2.ManagementActionCreate or
xpv2.ManagementActionUpdate (and adjust expected behavior/conditions
accordingly) so the tests truly exercise create- or update-specific paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@pkg/fieldpath/merge.go`:
- Around line 129-145: Add a unit test that constructs a MergeOptions with
KeepMapValues=true and AppendSlice=true, calls mergoConfiguration() (method on
MergeOptions) and asserts the returned slice of mergo.Config options contains
mergo.WithAppendSlice and does not contain mergo.WithOverride (i.e., expected
config is [mergo.WithAppendSlice]); ensure the test also verifies behavior by
running a small mergo.Merge of a source into a destination map+slice to confirm
existing map values are preserved and slices are appended when both options are
set.

In `@pkg/reconciler/managed/reconciler.go`:
- Around line 1098-1100: The provider error is being surfaced directly to users
via record.Event(managed, event.Warning(...)) and
status.MarkConditions(xpv2.ReconcileError(...)), which exposes non-deterministic
backend details; instead, keep the original err for operator logs but publish a
stable, user-friendly condition/event message (e.g., use the existing
errReconcileConnect/errCreateIncomplete strings) — replace
event.Warning(managed, err) with event.Warning(managed, errReconcileConnect) (or
the appropriate stable constant) and mark conditions with
xpv2.ReconcileError(errors.New(errReconcileConnect)) while logging the
wrapped/raw error via the controller logger (or record a debug/error log using
managed.GetLogger() or a similar logger) so raw details are retained in logs but
not in user-facing events/conditions.

---

Outside diff comments:
In `@apis/changelogs/proto/v1alpha1/changelog.proto`:
- Around line 71-73: The field comment for the map field additional_details is
grammatically incorrect ("An optional additional details"); update the proto
comment above the map<string, string> additional_details = 10; to read "Optional
additional details" so the comment is concise and correct for the
additional_details field in the Changelog proto.

In `@pkg/meta/meta.go`:
- Around line 88-117: The public API change migrates TypedReferenceTo, AsOwner,
and AsController to use xpv2.TypedReference which is a breaking change for
downstream consumers; either (A) mark this PR/release with the breaking-change
label and update the release notes to reference the crossplane/crossplane#7190
migration guide, or (B) add a compatibility layer: provide deprecated type alias
or adapter functions that accept the previous exported type and convert to
xpv2.TypedReference (and update TypedReferenceTo, AsOwner, AsController to use
the adapter), plus clear deprecation comments and a migration note in the
changelog pointing to the upstream PR.

In `@pkg/reconciler/managed/policies.go`:
- Around line 154-171: The Validate method in ManagementPoliciesResolver uses
m.managementPolicies.UnsortedList(), producing non-deterministic ordering;
replace those calls by computing a deterministic slice once (e.g., policiesList
:= sets.List(m.managementPolicies)) at the start of Validate and use
policiesList in both error returns (the errFmtManagementPolicyNonDefault and
errFmtManagementPolicyNotSupported paths) so the error text is stable across
reconciles; update references to UnsortedList() to use the single policiesList
variable and ensure sets.List is used on m.managementPolicies (type
xpv2.ManagementAction) before any early returns.

In `@pkg/reference/reference_test.go`:
- Line 8: Fix the typo in the license header string
"htcp://www.apache.org/licenses/LICENSE-2.0" by replacing "htcp://" with
"http://" so the URL reads "http://www.apache.org/licenses/LICENSE-2.0"; locate
the exact string in reference_test.go and update it accordingly (ensure no other
occurrences of "htcp://" remain).

In `@pkg/resource/providerconfig.go`:
- Around line 106-111: The fallback error for unsupported credential sources
(triggered in the CredentialsSourceInjectedIdentity case and default branch,
which currently returns errors.Errorf(errNoHandlerForSourceFmt, source)) should
be rewritten to a user-actionable message: update the returned error to explain
that credentials could not be loaded from the selected source, name the provided
source value, and suggest next steps (e.g., configure a supported credentials
source such as environment variables or a provider-specific injected identity
implementation). Replace usages of errNoHandlerForSourceFmt with a clearer
string and ensure the error is returned from the same branch handling
CredentialsSourceInjectedIdentity/default so callers receive the actionable
guidance.

---

Nitpick comments:
In `@apis/changelogs/proto/v1alpha1/changelog.proto`:
- Around line 63-65: Update the comment for the google.protobuf.Struct field
snapshot in changelog.proto to document the intended design: specify that
snapshot should capture the resource's full observed state (recommend including
spec and status but omit derived/transient fields), mandate redaction/filtering
of sensitive fields (secrets, PII) before storing, note size/complexity limits
or encourage truncation/summary for deeply nested/large resources, and list
expected consumers (debugging, audit trails, compliance) and retention
expectations; also mention that google.protobuf.Struct is chosen for flexibility
and call out alternatives (typed snapshots or diffs) if structured schema is
desired.

In `@pkg/fieldpath/merge_test.go`:
- Around line 170-193: Add a new test case that verifies combined behavior when
MergeOptions has both KeepMapValues set to true and AppendSlice set to true:
create a test similar to "MergeMapKeep" but set mo: &MergeOptions{KeepMapValues:
&valTrue, AppendSlice: &valTrue}, use object: fnMapDst(), value: fnMapSrc(),
path: pathTest and assert the serialized output via formatMap(...) to reflect
that existing dst map values are preserved while slice fields within map entries
are appended; place it alongside the existing MergeMap/Keep tests so maintainers
can see combined behavior for MergeOptions.

In `@pkg/reconciler/managed/reconciler_legacy_test.go`:
- Around line 1866-1879: The test cases in reconciler_legacy_test.go are setting
ManagementPolicies to xpv2.ManagementActionAll (see MockGet -> asLegacyManaged
and MockStatusUpdate -> newLegacyManaged with SetManagementPolicies), so they
actually exercise the "All" policy path rather than create/update-specific
behavior implied by the test names; either rename/remove these duplicate cases
to reflect they cover the All policy, or change the policy inputs in those cases
from xpv2.ManagementActionAll to the appropriate xpv2.ManagementActionCreate or
xpv2.ManagementActionUpdate (and adjust expected behavior/conditions
accordingly) so the tests truly exercise create- or update-specific paths.

In `@pkg/reference/reference.go`:
- Around line 438-447: The small helper sortMapByKeys is duplicating
functionality already implemented by the generic sortGenericMapByKeys[T any] in
namespaced_reference.go; remove sortMapByKeys and replace its usages with calls
to sortGenericMapByKeys[xpv2.Reference](m) (or the explicit type parameter) so
the package uses the single generic implementation (update any call sites that
previously referenced sortMapByKeys to call sortGenericMapByKeys with the
xpv2.Reference type).

In `@pkg/resource/providerconfig_test.go`:
- Around line 270-303: In the "NopUpdate" test (and the analogous modern-path
case later) capture the client.Object that ApplyFn receives so the test can
assert the applied object's wiring; update the ApplyFn used in the test (the
ApplyFn literal passed into fields.c) to store the passed-in object to a
test-scoped variable (e.g., appliedObj or appliedUsage) when invoked, then
return the same nil-or-errBoom behavior, and add assertions after the call that
verify the captured object's provider-config reference/labels are as expected;
do the same change for the corresponding modern-path test to ensure one legacy
and one modern case record the applied object for regression safety.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 6da5d934-9bab-47df-82d1-5f37de0672c8

📥 Commits

Reviewing files that changed from the base of the PR and between 8fa945b and baf45a5.

⛔ Files ignored due to path filters (7)
  • apis/common/v1/zz_generated.deepcopy.go is excluded by !**/zz_generated*.go and included by **/*.go
  • apis/common/v2/zz_generated.deepcopy.go is excluded by !**/zz_generated*.go and included by **/*.go
  • apis/common/zz_generated.deepcopy.go is excluded by !**/zz_generated*.go and included by **/*.go
  • go.mod is excluded by none and included by none
  • go.sum is excluded by !**/*.sum and included by none
  • gomod2nix.toml is excluded by none and included by none
  • pkg/resource/fake/mocks.go is excluded by !**/fake/** and included by **/*.go
📒 Files selected for processing (50)
  • apis/changelogs/proto/v1alpha1/changelog.proto
  • apis/common/condition.go
  • apis/common/condition_test.go
  • apis/common/doc.go
  • apis/common/merge.go
  • apis/common/merge_test.go
  • apis/common/observation.go
  • apis/common/policies.go
  • apis/common/resource.go
  • apis/common/v1/condition.go
  • apis/common/v1/doc.go
  • apis/common/v1/merge.go
  • apis/common/v1/observation.go
  • apis/common/v1/policies.go
  • apis/common/v1/resource.go
  • apis/common/v2/doc.go
  • apis/common/v2/resource.go
  • apis/proto/v1alpha1/ess.proto
  • pkg/conditions/manager.go
  • pkg/conditions/manager_test.go
  • pkg/fieldpath/merge.go
  • pkg/fieldpath/merge_test.go
  • pkg/meta/meta.go
  • pkg/meta/meta_test.go
  • pkg/reconciler/managed/api_test.go
  • pkg/reconciler/managed/metrics.go
  • pkg/reconciler/managed/policies.go
  • pkg/reconciler/managed/reconciler.go
  • pkg/reconciler/managed/reconciler_legacy_test.go
  • pkg/reconciler/managed/reconciler_modern_test.go
  • pkg/reconciler/providerconfig/reconciler.go
  • pkg/reference/namespaced_reference.go
  • pkg/reference/namespaced_reference_test.go
  • pkg/reference/reference.go
  • pkg/reference/reference_test.go
  • pkg/resource/enqueue_handlers_test.go
  • pkg/resource/interfaces.go
  • pkg/resource/predicates_test.go
  • pkg/resource/providerconfig.go
  • pkg/resource/providerconfig_test.go
  • pkg/resource/resource.go
  • pkg/resource/resource_test.go
  • pkg/resource/unstructured/claim/claim.go
  • pkg/resource/unstructured/claim/claim_test.go
  • pkg/resource/unstructured/composed/composed.go
  • pkg/resource/unstructured/composed/composed_test.go
  • pkg/resource/unstructured/composite/composite.go
  • pkg/resource/unstructured/composite/composite_test.go
  • pkg/statemetrics/mr_state_metrics.go
  • pkg/test/cmp.go
💤 Files with no reviewable changes (16)
  • apis/common/observation.go
  • apis/common/v1/merge.go
  • apis/common/v1/observation.go
  • apis/common/doc.go
  • apis/common/v1/doc.go
  • apis/common/condition.go
  • apis/common/merge.go
  • apis/common/condition_test.go
  • apis/common/merge_test.go
  • apis/common/v2/resource.go
  • apis/common/policies.go
  • apis/common/v2/doc.go
  • apis/common/v1/policies.go
  • apis/common/v1/resource.go
  • apis/common/v1/condition.go
  • apis/common/resource.go

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