libgit2: refactor feature managed transport usage #779
Conversation
…heckout pkg Adds a new field `Managed` to `CheckoutOpts` to let checkout impls figure out when to use managed transport. Adds a new field to the reconciler which stores the state of managed transport registration and uses the same to return a stalling error when managed transports are not registered but the feature is enabled. Signed-off-by: Sanskar Jaiswal <jaiswalsanskar078@gmail.com>
Signed-off-by: Sanskar Jaiswal <jaiswalsanskar078@gmail.com>
| } | ||
| conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error()) | ||
| return nil, e | ||
| } |
There was a problem hiding this comment.
Seeing the complexity this new approach (removing Enabled() from libgit2/managed/ package and storing the result of initialization in ManagedTransportRegistered) adds, and considering that we'll not need all these anymore soon, maybe we don't need to change any of it.
In this implementation, what was short and simple before using Enabled() which indicated if managed transport is requested and is successfully initialized, is now divided into two separate variables and we now need to add more checks to ensure that the reconciler doesn't enter into an undefined state.
Also considering that in the current implementation, if managed transport initialization fails, Enabled() wouldn't be true, maybe we should keep Enabled() and improve it a little.
source-controller/pkg/git/libgit2/managed/init.go
Lines 78 to 79 in f6a389c
|
We are probably better off focusing on decommissioning unmanaged transport rather than making more changes to the current setup. Specially if we are removing that feature in the next minor or so. |
|
Closing because of #779 (comment) and #779 (comment) |
This PR adds a new field
ManagedtoCheckoutOptsto let checkout implsfigure out when to use managed transport. Adds a new field to the
reconciler which stores the state of managed transport registration and
uses the same to return a stalling error when managed transports are not
registered but the feature is enabled.
Follow up to #727 and branched off from #746