Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(claim): Add SYCNED condition to XRs and XRCs #2968

Merged
merged 1 commit into from
Sep 12, 2022

Conversation

MisterMX
Copy link
Contributor

Description of your changes

Fixes #2850

  • Add Synced condition that can be used to track reconcile errors
    (similar to managed resources in providers)
  • Add an additional printer column to XR CRDs that references the
    Synced condition

I have:

  • Read and followed Crossplane's contribution process.
  • Run make reviewable to ensure this PR is ready for review.
  • Added backport release-x.y labels to auto-backport this PR if necessary.

How has this code been tested

Manually and unit tests

}

return res, errors.Wrap(r.client.Status().Update(ctx, cm), errUpdateClaimStatus)
}
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer to update the reconciler to set ReconcileError as part of the existing SetConditions calls, rather than moving the bulk of the logic into a wrapped reconcile method. The former approach is more in line with how we approach this in the managed reconciler, for example:

https://github.com/crossplane/crossplane-runtime/blob/988c9ba9c255d7cade32e2c0135ce47b083eaf95/pkg/reconciler/managed/reconciler.go#L684

Copy link
Contributor Author

@MisterMX MisterMX Jul 5, 2022

Choose a reason for hiding this comment

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

I rewrote the changes so the status updates follow the old pattern, although I don't think its the best decision and creates a lot of redundant code.

@negz can you have another look at it?

Fixes crossplane#2850

* Add `Synced` condition that can be used to track reconcile errors
(similar to managed resources in providers)
* Add an additional printer column to XR CRDs that references the
`Synced` condition

Signed-off-by: Maximilian Blatt <maximilian.blatt-extern@deutschebahn.com>
(external expert on behalf of DB Netz AG)
@MisterMX MisterMX requested a review from negz July 5, 2022 11:34
@MisterMX MisterMX mentioned this pull request Aug 22, 2022
6 tasks
@MisterMX MisterMX requested review from a team, nullable-eth and bobh66 and removed request for a team September 1, 2022 16:16
@negz negz closed this Sep 12, 2022
@negz negz reopened this Sep 12, 2022
@negz
Copy link
Member

negz commented Sep 12, 2022

Apparently I can't merge this PR - I think because it hasn't been approved by any CODEOWNERS (which would in this case be a member of the reviewers group or muvaf).

Copy link
Member

@hasheddan hasheddan left a comment

Choose a reason for hiding this comment

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

Stamping to unblock merge based on @negz approval.

@negz negz merged commit 7f08b2e into crossplane:master Sep 12, 2022
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.

Add SYNCED status column to XRC and XR
4 participants