Use interface for kclient

[feloy]

/kind cleanup

What does this PR do / why we need it:

• Uses an interface instead of concrete kclient.Client to make easier to test functions using this package
• Added mock generated with mockgen
• Updated mocks for existing mocsk (url, storage, locaConfigProvider)
• Added a script to regenerate the mocks
• Added a unit test to demo the use of kclient mock

PR acceptance criteria:

• Unit test

• Integration test

• Documentation

• I have read the test guidelines

How to test changes / Special notes to the reviewer:

[dharmit]

@feloy I haven't reviewed the PR. My comment is based only on pkg/kclient/interface.go.

Do you think having smaller interfaces would be better than having one large interface? I am suggesting along the lines of creating an interface for each of the .go files in pkg/kclient. So the methods under // deploymnet.go could live in its own interface, same for // events.go, // ingress.go, so on and so forth.

The reason I'm thinking this way is it makes testing easier because we can focus on implementing only a set of methods instead of a large number. Maybe using gomock we can automate the generation of mocks, but I remember reading on some official document that it's preferrable to have many smaller interfaces than one large interface. And in the end we can always make one big interface which takes all these interfaces if we really want a big interface, no?

What are your thoughts?

[feloy]

@dharmit I agree that interfaces should be small.
That was my first intent to break into smaller interfaces, one for each file (deployment, pod, service, etc), but that will increase the number of fileds in the genericclioptions.genericCxt, and probably the number of parameters for some functions needing methods from more than one interface.

Even with a large interface, Gomock will make easy to mock some methods only.

[dharmit]

@feloy thinking out loud here. If we're going to do this, would it provide benefit or be painful with those increased fields and parameters?

• If it's painful, sure, we can avoid it.
• If we don't know whether it will provide benefit, but it won't be painful either, I'd prefer smaller interfaces.

I admit that my comment is based more on the articles/docs I have read and not based on actual work done along these lines. :)

[feloy]

@dharmit
If we split the interfaces, we will get something similar to the ClientSet of the client-go library, containing a set of interfaces, one for each file in the kclient directory.

We could build a clientset similar to the client-go one: https://pkg.go.dev/k8s.io/client-go/kubernetes#Clientset

(let me have a try...)

[feloy]

/test v4.8-integration-e2e

• Failure [23.924 seconds]
odo devfile status command tests
/go/src/github.com/openshift/odo/tests/integration/devfile/cmd_devfile_status_test.go:19
  Verify devfile status works
  /go/src/github.com/openshift/odo/tests/integration/devfile/cmd_devfile_status_test.go:42
    Verify that odo component status correctly reports supervisord status [It]
skipped 192 lines unfold_more
[Fail] odo devfile status command tests Verify devfile status works [It] Verify that odo component status correctly reports supervisord status 
/go/src/github.com/openshift/odo/tests/helper/helper_cmd_wrapper.go:99

[feloy]

/test v4.8-integration-e2e

[Fail] odo devfile push command tests when creating a nodejs component [BeforeEach] when setting git config and running odo push checks that odo push works with a devfile 
/go/src/github.com/openshift/odo/tests/helper/helper_cmd_wrapper.go:99

[openshift-ci]

@feloy: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/psi-k8s-ibmc-integration-e2e	7bfc800	link	false	/test psi-k8s-ibmc-integration-e2e

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

[dharmit]

/approve
/lgtm

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dharmit

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 [dharmit]

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

[dharmit]

@feloy thanks for getting this done! Just one question. We discussed on some call to name the interface as Interface so that it could be imported/used as kclient.Interface. It's ClientInterface at the moment. Just trying to understand if you forgot to change it or you would rather prefer it to be ClientInterface.

[feloy]

@feloy thanks for getting this done! Just one question. We discussed on some call to name the interface as Interface so that it could be imported/used as kclient.Interface. It's ClientInterface at the moment. Just trying to understand if you forgot to change it or you would rather prefer it to be ClientInterface.

I didn't change for the moment, I prefer to change it in a next iteration of refactoring