-
Notifications
You must be signed in to change notification settings - Fork 96
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
Disconnect() on per reconcile loop scoped external client #745
Conversation
@negz @denniskniep PTAL. |
@ravilr I may not have a complete understanding but I'm curious why this affects I'm happy to be wrong, I just want to make sure we're not missing anything. Thanks |
Hm - maybe the |
yes, on every reconcile loop, ExternalConnecter's Connect() produces a new ExternalClient, and we want to be able to disconnect that ExternalClient at the end of that reconcile, if in case such an ExternalClient needs calling a close method to release resources it is holding (for example, a GRPC client requiring calling connection close). Also, linking the crossplane slack channel thread in #core-development: https://crossplane.slack.com/archives/CEF5N8X08/p1720210518036179, for more context. |
This LGTM, thanks @ravilr!
It'd be nice to avoid this but I can't think of any simple way to do so. Could you update the comment on the new |
1de3acf
to
7315060
Compare
Updated go doc comments to add :
|
Signed-off-by: ravilr <raviprasad_lr@yahoo.com>
7315060
to
84ee48a
Compare
@ravilr LGTM |
Description of your changes
Fixes #697
This deprecates the
WithExternalConnectDisconnecter(..)
Reconciler Option, to replace the Disconnect(..) on ExternalConnecter interface added in #296 with the newly added Disconnect(..) on the ExternalClient interface.Any provider implementation has to implement the Disconnect(..) method on its external client type to satisfy the ExternalClient interface. An ExternalClient not requiring to explicitly disconnect to cleanup it resources, can provide a no-op implementation just returning nil.
I have:
earthly +reviewable
to ensure this PR is ready for review.[ ] Linked a PR or a docs tracking issue to document this change.[ ] Addedbackport release-x.y
labels to auto-backport this PR.Need help with this checklist? See the cheat sheet.