Use core APIs from the Crossplane apis module#940
Use core APIs from the Crossplane apis module#940adamwg wants to merge 1 commit intocrossplane:mainfrom
Conversation
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>
📝 WalkthroughWalkthroughThis 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
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 failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (2 passed)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
There was a problem hiding this comment.
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 | 🟡 MinorMinor 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 | 🟡 MinorTypo in license header URL.
There's a small typo in the Apache License URL:
htcp://should behttp://. 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 | 🟠 MajorConfirm the breaking-change label was applied to this PR, or clarify the migration approach.
This commit intentionally migrates three exported functions—
TypedReferenceTo,AsOwner, andAsController—to usexpv2.TypedReferencefrom 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 | 🟡 MinorMake 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 registeredsounds like an internal wiring problem, but this path is reachable from user-configured sources likeInjectedIdentity. 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 | 🟡 MinorUse
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 thesnapshotfield 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
KeepMapValuestest coverage is solid, testing bothfalseandtruescenarios with clear expected outcomes. One thought - would it be valuable to add a test case that combines bothKeepMapValues: trueandAppendSlice: trueto 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 genericsortGenericMapByKeysfunction.I noticed there's a generic version
sortGenericMapByKeys[T any]innamespaced_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
Trackis the migrated label and provider-config reference wiring, but these cases only assert the returned error. Capturing theclient.Objectpassed toApplyFnin 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 theAllpolicy 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. IfAllis 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
⛔ Files ignored due to path filters (7)
apis/common/v1/zz_generated.deepcopy.gois excluded by!**/zz_generated*.goand included by**/*.goapis/common/v2/zz_generated.deepcopy.gois excluded by!**/zz_generated*.goand included by**/*.goapis/common/zz_generated.deepcopy.gois excluded by!**/zz_generated*.goand included by**/*.gogo.modis excluded by none and included by nonego.sumis excluded by!**/*.sumand included by nonegomod2nix.tomlis excluded by none and included by nonepkg/resource/fake/mocks.gois excluded by!**/fake/**and included by**/*.go
📒 Files selected for processing (50)
apis/changelogs/proto/v1alpha1/changelog.protoapis/common/condition.goapis/common/condition_test.goapis/common/doc.goapis/common/merge.goapis/common/merge_test.goapis/common/observation.goapis/common/policies.goapis/common/resource.goapis/common/v1/condition.goapis/common/v1/doc.goapis/common/v1/merge.goapis/common/v1/observation.goapis/common/v1/policies.goapis/common/v1/resource.goapis/common/v2/doc.goapis/common/v2/resource.goapis/proto/v1alpha1/ess.protopkg/conditions/manager.gopkg/conditions/manager_test.gopkg/fieldpath/merge.gopkg/fieldpath/merge_test.gopkg/meta/meta.gopkg/meta/meta_test.gopkg/reconciler/managed/api_test.gopkg/reconciler/managed/metrics.gopkg/reconciler/managed/policies.gopkg/reconciler/managed/reconciler.gopkg/reconciler/managed/reconciler_legacy_test.gopkg/reconciler/managed/reconciler_modern_test.gopkg/reconciler/providerconfig/reconciler.gopkg/reference/namespaced_reference.gopkg/reference/namespaced_reference_test.gopkg/reference/reference.gopkg/reference/reference_test.gopkg/resource/enqueue_handlers_test.gopkg/resource/interfaces.gopkg/resource/predicates_test.gopkg/resource/providerconfig.gopkg/resource/providerconfig_test.gopkg/resource/resource.gopkg/resource/resource_test.gopkg/resource/unstructured/claim/claim.gopkg/resource/unstructured/claim/claim_test.gopkg/resource/unstructured/composed/composed.gopkg/resource/unstructured/composed/composed_test.gopkg/resource/unstructured/composite/composite.gopkg/resource/unstructured/composite/composite_test.gopkg/statemetrics/mr_state_metrics.gopkg/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
Description of your changes
This PR brings back the changes from crossplane/crossplane#7190. We now depend on the Crossplane
apismodule for core APIs, and it does not depend on crossplane-runtime.I have:
./nix.sh flake checkto ensure this PR is ready for review.- [ ] Added or updated unit tests.- [ ] Linked a PR or a docs tracking issue to document this change.- [ ] Addedbackport release-x.ylabels to auto-backport this PR.