Disconnect() on per reconcile loop scoped external client

[ravilr]

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:

• 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.

Need help with this checklist? See the cheat sheet.

[ravilr]

@negz @denniskniep PTAL.

[bobh66]

@ravilr I may not have a complete understanding but I'm curious why this affects DIsconnect() but not Connect()? It seems like if the Disconnect() method is applicable to a particular scope then there should also be a similarly scoped Connect() method?

I'm happy to be wrong, I just want to make sure we're not missing anything. Thanks

[bobh66]

@ravilr I may not have a complete understanding but I'm curious why this affects DIsconnect() but not Connect()? It seems like if the Disconnect() method is applicable to a particular scope then there should also be a similarly scoped Connect() method?

Hm - maybe the Connect is implicit in the creation of the ExternalClient.

[ravilr]

Hm - maybe the Connect is implicit in the creation of the ExternalClient.

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.

[negz]

This LGTM, thanks @ravilr!

Any provider implementation has to implement the Disconnect(..) method on its external client type to satisfy the ExternalClient interface.

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 Disconnect method to mention that clients that don't need to explicitly disconnect can just return nil without doing anything (or similar wording). I want to make sure folks don't get confused, thinking they must implement Disconnect.

[ravilr]

Could you update the comment on the new Disconnect method to mention that clients that don't need to explicitly disconnect can just return nil without doing anything (or similar wording). I want to make sure folks don't get confused, thinking they must implement Disconnect.

Updated go doc comments to add :

	// Disconnect from the provider and close the ExternalClient.
	// Called at the end of reconcile loop. An ExternalClient not requiring
	// to explicitly disconnect to cleanup it resources, can provide a no-op
	// implementation which just return nil.

[ravilr]

@bobh66 @negz is this good to be merged ?

[denniskniep]

@ravilr LGTM
Thanks!