Proposal: Refactoring of KnParameters to an interface

[rhuss]

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:

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.

[rhuss]

ballot is over, let's tackle that refactoring. Thanks !

/assign rhuss

[github-actions]

This issue is stale because it has been open for 90 days with no
activity. It will automatically close after 30 more days of
inactivity. Reopen the issue with /reopen. Mark the issue as
fresh by adding the comment /remove-lifecycle stale.

[rhuss]

/remove-lifecycle-stale

[rhuss]

/remove-lifecycle stale

[github-actions]

This issue is stale because it has been open for 90 days with no
activity. It will automatically close after 30 more days of
inactivity. Reopen the issue with /reopen. Mark the issue as
fresh by adding the comment /remove-lifecycle stale.

[rhuss]

/remove-lifecycle stale

[rhuss]

This is refactoring should be considered to be implemented along with the extraction of the shared code #1039

[github-actions]

This issue is stale because it has been open for 90 days with no
activity. It will automatically close after 30 more days of
inactivity. Reopen the issue with /reopen. Mark the issue as
fresh by adding the comment /remove-lifecycle stale.

[rhuss]

/remove-lifecycle stale

@dsimansk Not sure if we can do it within the current scope of extracting shared code or whether we should consider this story for a second step refactoring.

[github-actions]

This issue is stale because it has been open for 90 days with no
activity. It will automatically close after 30 more days of
inactivity. Reopen the issue with /reopen. Mark the issue as
fresh by adding the comment /remove-lifecycle stale.

[rhuss]

/remove-lifecycle stale