knative / knative/client

Proposal: Refactoring of KnParameters to an interface

Open
#888 11 comments 4 reactions 1 assignee Claimed by @rhuss View on GitHub
triage/accepted
Dominant language
Go
Stars
387
Forks
274
Avg merge
1d 6h
Merged PRs (30d)
8

Description

Curently we are using a struct `KnParams` as the _context_ that we pass down into specific commands so that they can access the various services:

```golang
type KnParams struct {
Output io.Writer
KubeCfgPath string
ClientConfig clientcmd.ClientConfig
NewServingClient func(namespace string) (clientservingv1.KnServingClient, error)
NewSourcesClient func(namespace string) (v1alpha2.KnSourcesClient, error)
NewEventingClient func(namespace string) (clienteventingv1beta1.KnEventingClient, error)
NewDynamicClient func(namespace string) (clientdynamic.KnDynamicClient, error)

// General global options
LogHTTP bool

// Set this if you want to nail down the namespace
fixedCurrentNamespace string
}
```

This struct was mainly added in this form for allowing injection of fake clients for unit testing. However, this construct has some drawbacks:

* It leaks testing concerns into the business logic
* Anytime something you need for testing is added here (see `fixedCurrentNamespace` or also the `Output` writer which should be used from the command itself.
* Test initialization is done lazy with a side-effect in `Initialize()` which is hard to understand why that call is needed from business logic PoV
* `KnParameters` is a poor name because it does not reflect its purpose (i.e. the services included are not parameters, but part of the environment/context for which a command is created).

The suggested refactoring is:

* Move to a `KnContext` interface with direct access to the services needed.
* Provide an implementation `KnDefaultContext`, `KnFakeContext` (for fake based testing) and `KnMockContext` (for mock based testing)
* Consider to also move the output streams out & error to the context, too, so that any output goes over that streams. Test implementations `KnFakeContext` and `KnMockContext` the can use a buffer for these streams.
* The context remains the single constructor parameter for a command.

Please vote for this refactoring here with 👍 / 👎 , preferably with some comment to reason about your vote.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.