Skip to content

fix: order values in MultiResolutionResponse#802

Merged
jbw976 merged 1 commit intocrossplane:mainfrom
guilhem:ResolveMultipleOrder
Jun 9, 2025
Merged

fix: order values in MultiResolutionResponse#802
jbw976 merged 1 commit intocrossplane:mainfrom
guilhem:ResolveMultipleOrder

Conversation

@guilhem
Copy link
Copy Markdown
Contributor

@guilhem guilhem commented Jan 15, 2025

Description of your changes

Sort outputs from ResolveMultiple.

Fixes #803

I have:

Need help with this checklist? See the cheat sheet.

@guilhem guilhem force-pushed the ResolveMultipleOrder branch 6 times, most recently from ed7979f to 7611fb7 Compare January 15, 2025 16:36
@guilhem guilhem marked this pull request as ready for review January 15, 2025 16:38
@guilhem guilhem requested a review from a team as a code owner January 15, 2025 16:38
@guilhem guilhem requested a review from negz January 15, 2025 16:38
@negz
Copy link
Copy Markdown
Member

negz commented Jun 7, 2025

Sorry for the super late review. This LGTM but looks like there's now a merge conflict.

@guilhem guilhem force-pushed the ResolveMultipleOrder branch from f284457 to 5d7b981 Compare June 7, 2025 14:31
Signed-off-by: Guilhem Lettron <glettron@akamai.com>
@guilhem guilhem force-pushed the ResolveMultipleOrder branch from 5d7b981 to d48e582 Compare June 7, 2025 14:31
Copy link
Copy Markdown
Member

@jbw976 jbw976 left a comment

Choose a reason for hiding this comment

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

Let's get this merged before my #843, so you don't have to deal with any more merge conflicts 😇

want: want{
rsp: MultiResolutionResponse{
ResolvedValues: []string{value2, value},
ResolvedReferences: []xpv1.Reference{{Name: value2}, {Name: value}},
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

interesting that we don't seem to have had a test case already that returned multiple values - i guess if we did it would have been failing intermittently because we weren't using a stable sort order until this PR 🤓

@jbw976
Copy link
Copy Markdown
Member

jbw976 commented Jun 9, 2025

I think the check-diff step failed because this branch doesn't have #846, as shown in the compare. Will merge to main with this failure since it's fixed there.

@jbw976 jbw976 merged commit 28c1851 into crossplane:main Jun 9, 2025
9 of 10 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.

Reciliation storm when using listType=set

3 participants