Remove client get from repo reconciler

[liamfallon]

No description provided.

[liamfallon]

The giteaclient.Get() function is used in the Token reconciler as well so I'll raise a separate PR to remove the Get() from that reconciler and then from the giteaclient module itself.

[liamfallon]

/assign @johnbelamaric @vjayaramrh @efiacor

[efiacor]

I think the logic here needs to be refactored. I am guessing the flow was done to allow for async setup of tokens/secrets and or repos but it seems a bit off.
@henderiw do we need to assign a new var here even though we have the gitea client in the context already from SetupWithManager?

[liamfallon]

If SetupWithManager() works correctly then r.giteaclient should never be nil, it is set in the giteaclient.GetClient() function (and giteaclient.Start() which it calls).

Therefore if SetupWithManager() is always called, the entire if statement from lines 107-113 is redundant and can be removed.

@vjayaramrh from a giteaclient point of view, what do you think?

[vjayaramrh]

The r.giteaClient is of type giteaclient.GiteaClient (refer https://github.com/nephio-project/nephio/blob/main/controllers/pkg/reconcilers/repository/reconciler.go#L87 which in turn refers to https://github.com/nephio-project/nephio/blob/main/controllers/pkg/giteaclient/giteaclient.go#L34). This would be non-nil after the call to SetupWithManager().
The r.giteaClient.Get() returns object of type *gitea.Client (refer https://github.com/nephio-project/nephio/blob/main/controllers/pkg/giteaclient/giteaclient.go#L76 ) and can be nil until it is setup. So, the r.giteaClient.Get() is needed per my opinion.

Note: The r.giteaClient.Get() returns non-nil only when the code at https://github.com/nephio-project/nephio/blob/main/controllers/pkg/giteaclient/giteaclient.go#L130 is successfully executed.

[liamfallon]

But won't SetupWithManager() always be called before the reconcile method is called?

[efiacor]

So in that case should the failure from.Start() not be handled in the .GetClient() similar to how the context and client nil is checked?
If the actual gitea client fails to init, then surely the whole flow should fail?
Also, the naming of the wrapper doesn't help the reader at all but that's a separate discussion.

[vjayaramrh]

@liamfallon I do believe that SetupWithManager() will be called before reconcile method is called. The r.giteaClient of type giteaclient.GiteaClient is definitely non-nil at https://github.com/nephio-project/nephio/blob/main/controllers/pkg/reconcilers/repository/reconciler.go#L63 after SetupWithManager() is invoked, however, it does not mean that the value for the field giteaClient of type *gitea.Client is filled up at https://github.com/nephio-project/nephio/blob/main/controllers/pkg/giteaclient/giteaclient.go#L76. The value for the field is only set successfully at https://github.com/nephio-project/nephio/blob/main/controllers/pkg/giteaclient/giteaclient.go#L130.

[vjayaramrh]

@efiacor The Start() does not return any errors or other return values, refer https://github.com/nephio-project/nephio/blob/main/controllers/pkg/giteaclient/giteaclient.go#L80. The Start continues to be running as part of the invocation at https://github.com/nephio-project/nephio/blob/main/controllers/pkg/giteaclient/giteaclient.go#L63

[efiacor]

Ok. So .Start() will run until ctx.Done() is reached?
But ultimately, do we need direct access to the gitea client? Should we not just wrap the parts of the interface that we need?
Something like this preferably to assist with testing.

[henderiw]

the code can be optimised. 1 I passed the client to delete repo which should not be done.
in this case the check should be moved to delete repo. Now the check is purely a protection, so if it was not initialised you get a panic. So I rather protect against it even though it should never happen. So I prefer to keep the check and maybe add a comment to say /should never happen since it is initialized in setupManager.

[liamfallon]

I added a function to the giteaclient wrapper to check if the the gitea client is initialized so the check in the reconciler remains. I think that should be OK.

[henderiw]

Move the tests to the giteaclient folder as we are mocking the gitea client, not the reconciler.

[efiacor]

/approve

[henderiw]

/lgtm
/approve

[nephio-prow]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: efiacor, henderiw

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [henderiw]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[johnbelamaric]

/lgtm