Skip to content

Conversation

@MisterMX
Copy link
Contributor

@MisterMX MisterMX commented Oct 23, 2024

Description of your changes

Marks the composite as ready or unready if the if .Desired.Composite.Ready field is set explicitly.

Fixes #6020

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.
  • [ ] Added or updated e2e tests.
  • [ ] Linked a PR or a docs tracking issue to document this change.
  • Added backport release-x.y labels to auto-backport this PR.

Need help with this checklist? See the cheat sheet.

@MisterMX MisterMX requested review from a team and negz as code owners October 23, 2024 11:52
@MisterMX MisterMX requested a review from bobh66 October 23, 2024 11:52
// ConditionsOverride allows the composition process to force
// a specific condition status, unlike Conditions which does not allow
// setting system conditions.
ConditionsOverride []xpv1.Condition
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we could use a special TargetdCondition variant. i.e. Add a System field and just use that internally to surface these conditions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried this but it does work as intended since the custom conditions update (which uses the Conditions array) is performed in handleCommonCompositionResult() but the system conditions are set in updateXRConditions().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed to a new type CompositeResource that is analog to ComposedResource and contains a Ready *bool property.

@MisterMX MisterMX force-pushed the feat/function-composite-ready-state branch 3 times, most recently from abcea6b to 3273978 Compare October 31, 2024 14:12
Copy link
Contributor

@phisco phisco left a comment

Choose a reason for hiding this comment

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

overal LGTM, code coverage is complaining, but not too much so we could be fine... Thanks @MisterMX!

@negz I'll leave to you the final call.

@phisco phisco merged commit 8714042 into crossplane:main Oct 31, 2024
@github-actions
Copy link

Successfully created backport PR for release-1.18:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Crossplane should consider the composite ready state in function responses

3 participants