Add Disconnect call in Reconcile #296
Conversation
vaspahomov
force-pushed
the
feature/disconnect-in-reconcile
branch
from
6df5539
to
a1ef79f
Compare
There was a problem hiding this comment.
Thanks for raising this! Could you add a little more information about your use case - what SDK have you found that needs an explicit disconnect? I think I've run into a similar case in the past, but I can't remember what it was.
In general this approach looks good to me - could you please also update the PR description with information about how you've tested this, and add a unit test for the case in which the deferred Disconnect fails.
vaspahomov
force-pushed
the
feature/disconnect-in-reconcile
branch
from
a1ef79f
to
5296080
Compare
I've found what yandex-cloud/go-sdk needs disconnect. |
|
Thanks for review! I've committed fixes |
Signed-off-by: vaspahomov <vas2142553@gmail.com>
vaspahomov
force-pushed
the
feature/disconnect-in-reconcile
branch
from
5296080
to
8af1fca
Compare
Signed-off-by: vaspahomov <vas2142553@gmail.com>
vaspahomov
force-pushed
the
feature/disconnect-in-reconcile
branch
from
8af1fca
to
a5ff67d
Compare
| if disconnectErr := r.external.Disconnect(ctx); disconnectErr != nil { | ||
| log.Debug("Cannot disconnect from provider", "error", disconnectErr) | ||
| record.Event(managed, event.Warning(reasonCannotDisconnect, disconnectErr)) | ||
| managed.SetConditions(xpv1.ReconcileError(errors.Wrap(disconnectErr, errReconcileDisconnect))) |
There was a problem hiding this comment.
I think this SetCondition call might cause flapping/overwritten conditions in cases where the deferred function is called because we're returning from another error case where we would also update the same condition. I'll fix this up in a future PR though.
Description of your changes
I've add Disconnect call in end of Reconcile.
Some sdks required Close call after use. But I still want to create sdk in Connect call rather then create sdk in each Observe, Create, Update, Delete. And it's not possible to dispose sdk in the end of Reconcile.
I have:
make reviewable testto ensure this PR is ready for review.