Skip to content

Conversation

@erhancagirici
Copy link
Contributor

Description of your changes

During reference resolution for multiple resources, do not sort refs and resolved values while doing resolution only for refs (no selector resolution involved).Sort only when resolving a selector.

Fixes crossplane/crossplane#6919

Context:
Previously, #802 introduced sorting the resolved references during-multi resolution to address #803.

In #803, AFAIU the issue was related to resolving multiple values with selector. Here at selector resolution code the k8s list call does not have a stable ordering of returned resources. So, each selector resolution attempt, might potentially shuffle the order for both resolved references and values.

Once selector is resolved, say spec.forProvider.fooIDsSelector, it populates/mutates both spec.forProvider.fooIDsRefs and spec.forProvider.fooIDs.
Unless specified otherwise via the resolve policy spec.forProvider.fooIDsSelector.policy.resolve, by default, subsequent resolutions enters this code block, uses the already-resolved refs at spec.forProvider.fooIDsRefs and returns accordingly, i.e. won't try to "list" and re-evaluate selector .

Therefore, in #802, sorting the resolved refs and values when resolving the selector is OK (here).

However, when resolving the existing refs only, sorting here seems to be unnecessarily mutating the spec.forProvider.fooIDsRefs and causing issues.
When no selector is involved/omitted and refs are explicitly set, spec.forProvider.fooIDsRefs should not be re-ordered.

  • This overrides the user intent, when the element order is significant for spec.forProvider.fooIDs
  • Also, it results in a superfluous drift in gitops scenarios where spec.forProvider.fooIDsRefs are set by tools like ArgoCD etc.

I have:

  • Read and followed Crossplane's [contribution process].
  • Run earthly +reviewable to ensure this PR is ready for review.
  • Added or updated unit tests.
  • Linked a PR or a [docs tracking issue] to [document this change].
  • Added backport release-x.y labels to auto-backport this PR.

@coderabbitai
Copy link

coderabbitai bot commented Dec 23, 2025

📝 Walkthrough

Walkthrough

Modify ResolveMultiple in pkg/reference/ implementations to preserve input order when resolving explicit References by collecting resolved values into slices; selector-based listing still aggregates into maps and sorts by keys. Tests add scaffolding and new cases to validate ordering behaviors.

Changes

Cohort / File(s) Summary
Core resolution logic
pkg/reference/reference.go, pkg/reference/namespaced_reference.go
ResolveMultiple now fills a fixed-size resolvedVals slice when req.References is provided (preserving input order) instead of building/sorting a map. The no-reference/selector branch continues to build a map from List results and sorts by keys before constructing the response.
Tests: reference resolution
pkg/reference/reference_test.go, pkg/reference/namespaced_reference_test.go
Added test helpers (prepareTestExamples*) and package-level test data; renamed "OrderOutput" → "SelectorOrderOutput"; added cases "NoSelectorOnlyRefs" and "AlwaysResolveSelector_NewValuesOrdered"; enhanced tests to assert ordering and List/Get interaction semantics.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Thanks for the clear changes — if you want, I can generate a focused review checklist or highlight specific lines to inspect.


Important

Pre-merge checks failed

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

❌ Failed checks (1 error, 1 warning)
Check name Status Explanation Resolution
Breaking Changes ❌ Error The PR modifies the output order of ResolveMultiple, an exported function, changing from sorted results to input-order preservation when no selector is involved. Apply 'breaking-change' label or document why this behavioral change doesn't require it; clarify whether this fixes issue #6919 as intended.
Title check ⚠️ Warning The title exceeds the 72-character limit at 79 characters, though it clearly describes the main change: preventing ref sorting during multi-resolution when no selector is involved. Shorten the title to 72 characters or fewer while preserving the core meaning, e.g., 'fix: do not sort refs when resolving existing refs only'.
✅ Passed checks (3 passed)
Check name Status Explanation
Description check ✅ Passed The description comprehensively explains the change, rationale, and context, directly addressing the issue #6919 regarding unwanted reordering of list fields in GitOps workflows.
Linked Issues check ✅ Passed The code changes successfully address issue #6919 by removing sorting when resolving existing refs only and preserving sorting for selector-based resolution, preventing unwanted reordering of user-specified reference lists.
Out of Scope Changes check ✅ Passed All changes are within scope: modifications to reference resolution logic in both base and namespaced implementations, plus corresponding comprehensive test updates to validate the new sorting behavior.

🧹 Recent nitpick comments
pkg/reference/namespaced_reference.go (1)

288-297: Nice use of generics for the sort helper.

The sortGenericMapByKeys function cleanly leverages Go generics. I notice reference.go has a similar non-generic sortMapByKeys function. Would it make sense to consolidate by having sortMapByKeys also use this generic version, or are there reasons to keep them separate (e.g., avoiding changes to the non-namespaced path)? Just curious about your thoughts—not blocking.


📜 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 be53912 and 824c454.

📒 Files selected for processing (4)
  • pkg/reference/namespaced_reference.go
  • pkg/reference/namespaced_reference_test.go
  • pkg/reference/reference.go
  • pkg/reference/reference_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
  • pkg/reference/namespaced_reference_test.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:

  • pkg/reference/reference.go
  • pkg/reference/namespaced_reference.go
  • pkg/reference/reference_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:

  • pkg/reference/reference_test.go
🧠 Learnings (4)
📚 Learning: 2025-10-16T17:11:48.360Z
Learnt from: CR
Repo: crossplane/crossplane-runtime PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2025-10-16T17:11:48.360Z
Learning: Applies to **/!(*_test).go : Avoid behavior changes to exported Go APIs that could break existing users unless labeled 'breaking-change'

Applied to files:

  • pkg/reference/reference_test.go
📚 Learning: 2025-10-16T17:11:48.360Z
Learnt from: CR
Repo: crossplane/crossplane-runtime PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2025-10-16T17:11:48.360Z
Learning: Applies to **/!(*_test).go : Do not rename exported Go APIs without the 'breaking-change' label

Applied to files:

  • pkg/reference/reference_test.go
📚 Learning: 2025-10-16T17:11:48.360Z
Learnt from: CR
Repo: crossplane/crossplane-runtime PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2025-10-16T17:11:48.360Z
Learning: Applies to **/!(*_test).go : Do not change function/method signatures of exported Go APIs without the 'breaking-change' label

Applied to files:

  • pkg/reference/reference_test.go
📚 Learning: 2025-10-16T17:11:48.360Z
Learnt from: CR
Repo: crossplane/crossplane-runtime PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2025-10-16T17:11:48.360Z
Learning: Applies to **/!(*_test).go : Do not remove exported Go APIs (functions, types, methods, or fields) without the 'breaking-change' label

Applied to files:

  • pkg/reference/reference_test.go
🧬 Code graph analysis (1)
pkg/reference/reference.go (2)
pkg/resource/interfaces.go (1)
  • Managed (200-204)
apis/common/v1/resource.go (1)
  • Reference (77-77)
🔍 Remote MCP

Based on my search results, I've found relevant context about the historical background and design decisions for this PR. Here's the additional context that's relevant to reviewing this pull request:

Key Background Information

PR #802 Context

PR #802 introduced "fix: order values in MultiResolutionResponse", which is the change this PR is reverting for the explicit references case. This original change added sorting to handle nondeterministic ordering from selector-based list calls, but it had the unintended side effect of reordering user-provided reference lists.

Reference Resolution Architecture

ResolveMultiple resolves the supplied MultiResolutionRequest. The function has two distinct code paths:

  1. References-present path: When explicit references are provided (e.g., user-supplied names in spec.forProvider.compositeRolesRefs), the system directly uses the Get operation to look up individual resources by name.

  2. Selector-based path: When a selector is used instead of explicit references, the system uses the List operation, which can return resources in unstable/random order.

Reference Resolution Policies

Resolve specifies when this reference should be resolved. The default is 'IfNotPresent', which will attempt to resolve the reference only when the corresponding field is not present. Use 'Always' to resolve the reference on every reconcile.

Relationship to GitOps Workflows

The reordering issue directly impacts GitOps workflows because Settings in forProvider are always enforced by Crossplane. Crossplane reverts any changes to a forProvider field in the external resource. When Crossplane reorders fields that users have deliberately ordered in their Git manifests, it triggers continuous reconciliation cycles.

Review Considerations

  • Code change scope: The PR limits sorting to only the selector-based path (valueMap aggregation), leaving the explicit references path to preserve input order with direct slice assignment.
  • Test coverage: The PR adds comprehensive test cases including "NoSelectorOnlyRefs" to ensure refs are not reordered when selectors are omitted, and "AlwaysResolveSelector_NewValuesOrdered" to validate selector behavior still orders correctly.
  • Backward compatibility: The change maintains sorting behavior for selector-based resolution (where ordering is nondeterministic), ensuring existing users relying on sorted selector results are not affected.

[::web_search::]

🔇 Additional comments (9)
pkg/reference/namespaced_reference.go (2)

236-259: LGTM! Order preservation for explicit refs path looks correct.

The change from map-based aggregation to indexed slice assignment (resolvedVals[i] = ...) correctly preserves the user-specified order of references. This directly addresses the GitOps sync loop issue by avoiding mutation of the spec.forProvider.<...>Refs field order.

Quick question to confirm my understanding: by returning req.References directly (line 256), we ensure the original reference slice is preserved as-is—is there any concern about aliasing if the caller later modifies req.References? Given this is typically a response struct that's consumed and not mutated, I think it's fine, but wanted to check your intent here.


272-286: LGTM! Selector-based path correctly maintains sorted output.

The map-based aggregation with sorting is appropriate here since client.List can return resources in unstable order. Sorting ensures deterministic output for selector-based resolution.

I see the valueMap declaration was moved down to be closer to its usage—thanks for addressing that nit!

pkg/reference/reference.go (2)

388-406: LGTM! Consistent with the namespaced variant.

The explicit refs resolution path correctly uses indexed slice assignment to preserve user-specified order. The implementation mirrors the changes in namespaced_reference.go, which is great for maintainability.


408-428: LGTM! Selector path maintains deterministic ordering.

The map-based approach with sorting for the selector path is appropriate and consistent with the namespaced implementation.

pkg/reference/reference_test.go (5)

45-63: Good test scaffolding for ordering verification.

The prepareTestExamples helper cleanly generates consistent test data with proper external names and controller references. Using package-level initialization via var testValues, testRefs, testControlled = prepareTestExamples(10) is pragmatic for test setup.

One minor observation: meta.AddControllerReference returns an error that's being ignored (line 57). While unlikely to fail in tests, you might consider checking it for completeness:

if err := meta.AddControllerReference(controlled, meta.AsController(&xpv1.TypedReference{UID: testControllerUID})); err != nil {
    panic(err) // fail fast in test setup
}

Not critical since this is test scaffolding, but could help catch unexpected issues.


876-902: Great defensive test enhancement.

Adding MockList: test.NewMockListFn(errors.New("unexpected call to List when resolving Refs only")) is a smart way to verify that the refs-only path doesn't accidentally invoke List. This catches potential regressions where the code might incorrectly fall through to the selector path.


903-932: LGTM! Clear rename to distinguish test scope.

Renaming to SelectorOrderOutput clarifies that this test specifically validates ordering behavior for selector-based resolution. The updated reason at line 904 reinforces this distinction nicely.


933-963: Excellent test case—this is the heart of the fix!

This test directly validates the PR's objective: ensuring refs maintain their user-specified order when no selector is involved. The shuffled input order ([2, 3, 0, 1]) effectively demonstrates that the output preserves this exact order rather than sorting alphabetically.

The MockList returning an error is a nice touch to guarantee the code path stays in the refs-only branch.


964-1005: Good coverage for the Always policy + selector combination.

This test validates that when Selector.Policy.Resolve is Always, the selector-based resolution runs (ignoring existing refs) and produces sorted output. The expected values [testValues[1], testValues[2], testValues[4]] confirm the sorting behavior for selector results.

One question for my understanding: I see the test provides CurrentValues and References in the request (lines 976-977), but since the selector has Resolve: Always, these should be cleared in IsNoOp() and ignored. The test's MockGet is wired up but I don't think it gets called since the selector path uses List. Could you confirm this is intentional—perhaps kept for documentation purposes to show what gets overridden?

✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.


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

Caution

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

⚠️ Outside diff range comments (1)
pkg/reference/reference_test.go (1)

8-8: Fix typo in license URL.

The URL has a typo: "htcp://" should be "http://".

🔎 Proposed fix
-    htcp://www.apache.org/licenses/LICENSE-2.0
+    http://www.apache.org/licenses/LICENSE-2.0
📜 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 f5c1eee and e5fb399.

📒 Files selected for processing (4)
  • pkg/reference/namespaced_reference.go
  • pkg/reference/namespaced_reference_test.go
  • pkg/reference/reference.go
  • pkg/reference/reference_test.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:

  • pkg/reference/namespaced_reference.go
  • pkg/reference/namespaced_reference_test.go
  • pkg/reference/reference.go
  • pkg/reference/reference_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:

  • pkg/reference/namespaced_reference_test.go
  • pkg/reference/reference_test.go
🧠 Learnings (4)
📚 Learning: 2025-10-16T17:11:48.360Z
Learnt from: CR
Repo: crossplane/crossplane-runtime PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2025-10-16T17:11:48.360Z
Learning: Applies to **/!(*_test).go : Avoid behavior changes to exported Go APIs that could break existing users unless labeled 'breaking-change'

Applied to files:

  • pkg/reference/namespaced_reference_test.go
  • pkg/reference/reference_test.go
📚 Learning: 2025-10-16T17:11:48.360Z
Learnt from: CR
Repo: crossplane/crossplane-runtime PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2025-10-16T17:11:48.360Z
Learning: Applies to **/!(*_test).go : Do not rename exported Go APIs without the 'breaking-change' label

Applied to files:

  • pkg/reference/namespaced_reference_test.go
  • pkg/reference/reference_test.go
📚 Learning: 2025-10-16T17:11:48.360Z
Learnt from: CR
Repo: crossplane/crossplane-runtime PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2025-10-16T17:11:48.360Z
Learning: Applies to **/!(*_test).go : Do not change function/method signatures of exported Go APIs without the 'breaking-change' label

Applied to files:

  • pkg/reference/namespaced_reference_test.go
  • pkg/reference/reference_test.go
📚 Learning: 2025-10-16T17:11:48.360Z
Learnt from: CR
Repo: crossplane/crossplane-runtime PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2025-10-16T17:11:48.360Z
Learning: Applies to **/!(*_test).go : Do not remove exported Go APIs (functions, types, methods, or fields) without the 'breaking-change' label

Applied to files:

  • pkg/reference/namespaced_reference_test.go
  • pkg/reference/reference_test.go
🧬 Code graph analysis (2)
pkg/reference/namespaced_reference_test.go (5)
apis/common/v1/resource.go (5)
  • NamespacedReference (80-80)
  • TypedReference (85-85)
  • Selector (88-88)
  • NamespacedSelector (91-91)
  • Policy (74-74)
pkg/meta/meta.go (1)
  • SetExternalName (279-281)
pkg/test/fake.go (1)
  • NewMockListFn (105-115)
pkg/reference/namespaced_reference.go (2)
  • MultiNamespacedResolutionRequest (94-101)
  • MultiNamespacedResolutionResponse (139-142)
pkg/reference/reference.go (2)
  • To (182-185)
  • ExternalName (193-197)
pkg/reference/reference.go (1)
pkg/resource/interfaces.go (1)
  • Managed (200-204)
🔍 Remote MCP

The Context7 API key validation failed, but the web search provided useful information. Let me search for more specific details about the referenced PR #802 and the selector-based resolution behavior.Excellent! I found the key information about PR #802. Let me search for more details about the specific ordering issue and the problem statement.Now I have sufficient information to provide a comprehensive summary. Let me compile the findings.

Additional Context for PR #898 Review

Background: Prior Sorting Change (PR #802)

PR #802 introduced "fix: order values in MultiResolutionResponse", which added sorting logic to reference resolution results to address nondeterministic ordering from selector-based list calls.

Problem This PR Addresses

Managed resources can reference other managed resources by name reference or selector, with selector being the most flexible matching method using matchLabels. The issue is that when Crossplane lists environment configs (or other resources) matching given labels through selectors, the ordering returned by the List call can be nondeterministic, potentially resulting in a random one being picked as the first one.

However, the sorting logic from PR #802 had an unintended side effect: it reorders user-specified references (like compositeRolesRefs in the linked issue #6919), which conflicts with GitOps tools like ArgoCD that track resource state by comparing desired vs. actual configurations.

Resolution Strategy in PR #898

The PR narrows the sorting behavior introduced in PR #802:

  1. Sorting applies only when using a selector - When a selector is present and its policy is set to "Always", the References are cleared and resolved through a List operation, where sorting is appropriate to handle nondeterministic ordering.

  2. No sorting for existing references only - When resolving pre-set references without a selector, if CurrentValues are already present and the policy is not "Always", the resolver returns early without re-resolving, preserving user-defined order.

  3. Direct aggregation preserving input order - The MultiResolutionRequest contains CurrentValues and References, with the MultiResolutionResponse returning ResolvedValues and ResolvedReferences. The fix directly aggregates resolved values in input order instead of using map-based sorting.

Test Coverage

The PR adds test cases to verify:

  • "NoSelectorOnlyRefs": References are resolved directly without reordering when selector is omitted
  • "AlwaysResolveSelector_NewValuesOrdered": Ordering behavior when selector policy is Always and new matches appear
  • Updated "SelectorOrderOutput" test: Validates ordering only applies when resolvers use selector policies

[::web_search::]

🔇 Additional comments (12)
pkg/reference/namespaced_reference.go (1)

240-258: LGTM! Order preservation logic is correct.

The change effectively preserves the input order of references by directly assigning extracted values into resolvedVals[i] instead of using map-based aggregation. This ensures user-specified reference order is maintained when resolving existing refs (no selector involved), which fixes the GitOps drift issue described in #6919.

The selector-based path (lines 274-286) still applies sorting via sortGenericMapByKeys, which correctly addresses the nondeterministic ordering from List calls introduced in PR #802.

pkg/reference/reference.go (1)

392-405: LGTM! Consistent implementation across resolvers.

The change mirrors the logic in namespaced_reference.go, preserving input order when resolving existing references by using direct slice indexing. This ensures consistency across both the namespaced and non-namespaced resolver implementations.

The selector-based resolution path (lines 416-428) continues to apply sorting, which is the correct behavior to handle nondeterministic List results.

pkg/reference/namespaced_reference_test.go (5)

23-41: Test scaffolding looks good, minor suggestion for readability.

The helper function prepareTestExamplesNamespaced effectively generates test data with consistent relationships between values, references, and controlled objects. The package-level initialization on line 41 is a reasonable approach for shared test data.

One small suggestion: consider adding a comment above line 41 explaining that these are pre-populated test fixtures to make the test file more approachable for future readers.


891-891: Excellent verification approach.

Injecting a MockList error on line 891 (and similarly on line 948) is a great way to verify that the List operation is not invoked when resolving references directly. This catches potential regressions where the code might inadvertently call List even when only references are provided.


915-944: Good rename and clarification.

Renaming "OrderOutput" to "SelectorOrderOutput" and updating the reason string to explicitly state "when resolving a selector" makes the test's intent much clearer. This helps distinguish between selector-based ordering (which should sort) and reference-based ordering (which should not).


945-975: Excellent coverage for the refs-only scenario.

This test case directly validates the core fix: when a selector is omitted and only references are provided, the resolved values should maintain the input order (indices 2, 3, 0, 1) rather than being sorted. The MockList error on line 948 ensures we're truly testing the refs-only path.


976-1017: Great test for the Always policy edge case.

This test covers an important scenario: when the selector policy is set to Always, the resolver should re-list resources and apply sorting, even when existing refs/values are present. The unordered List results on lines 998-1004 combined with the expectation of ordered output on lines 1013-1014 clearly demonstrate that sorting is applied in this case.

Thank you for the thorough test coverage!

pkg/reference/reference_test.go (5)

39-63: Test scaffolding is well-structured.

The constants and helper function provide a clean way to generate consistent test data. The pattern mirrors the namespaced version, which is good for consistency across the codebase.


879-902: Excellent verification with MockList error.

Similar to the namespaced tests, the MockList error injection on line 879 ensures that List is not called when resolving references directly. This provides strong confidence that the implementation follows the intended code path.


903-932: Clear test naming and intent.

The rename from "OrderOutput" to "SelectorOrderOutput" with the updated reason clearly communicates that sorting applies when using selectors. This helps distinguish the two ordering behaviors introduced by this PR.


933-963: Strong test coverage for order preservation.

This test validates the core behavior change: when resolving references without a selector, the input order (indices 2, 3, 0, 1) is preserved in the output. The MockList error ensures we're testing the correct code path.


964-1005: Comprehensive test for Always policy scenario.

This test effectively validates that when the selector policy is Always, the resolver performs a List operation and applies sorting to the results, even when existing refs/values are present. The unordered List items (lines 986-992) contrasted with ordered output expectations (lines 1001-1002) demonstrates the sorting behavior.

Great work on the test coverage for this PR!

Copy link
Member

@ezgidemirel ezgidemirel left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @erhancagirici! There’s some duplicated code between the namespaced and non-namespaced resolvers for resolving refs and preserving order. Not a blocker, but it might be nice to factor this into a helper.

…resolution

Signed-off-by: Erhan Cagirici <erhan@upbound.io>
@phisco phisco merged commit 8cf59f2 into crossplane:main Jan 20, 2026
11 checks passed
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.

Crossplane v2.1 reorders list fields alphabetically, causing infinite sync loop with ArgoCD

3 participants