src: exported client constructor with options

[lkingland]

Changes

• 🧹 simplified client constructor and direct usage of fn.Option

A prerequisite for #842, this PR adds back in the ability to provide custom Client concrete implementations of subsystems, while retaining the new centralized default Client instantiation (which is in place of the previous Client Factories).

Other than some minor cleanup throughout cmd, changes worth explaining a bit include:

NewClient Constructor

The recent inclusion of the verbose flag as a constructor argument
allowed for further simplification of the Client instantiation block; now
extracted to be simply NewClient()

Furthermore, the ClientFactory itself can be directly referenced in the method signature of the NewClient constructor,
which adds flexibility while retaining the ability to fully mock for tests.

WithVerbose Options

Subsystems which use Functional Options have been updated to include
a WithVerbose Option rather than a standalone constructor argument.

ClientOptions Struct

The ClientOptions struct was renamed to ClientConfig, added to the signature are actual Client Options (...fn.Option)
such that individual commands can directly utilize the ClientFactory, and provide their own fn.Options as needed.
This includes removing XConfigToClientOptions methods because the number of members would have been merely
two (namespace and verbose), and the ClientFactory is no longer different from the method signature of NewClient itself.
This assistance method could of course be added back if we move back to having a larger number of Client construction
arguments.

ClientFactory

Now even further simplified, using a standard NewClient constructor,
since the deferred instantiation is not necessary now that Client Options
are provided using actual client options ([]fn.Option) in stead of a
rather than a separate struct.

Command Args

To avoid edge cases where some commands were failing in IDEs which precomile
tests, we are now explicitly indicating that arguments provided to the test command
are not in turn propagated to command objects created within tests by ensuring
cmd.SetArgs([]string{}) is used throughout.

Version

Now using the Version struct in the root command directly rather than mapping through
an interstitial config struct.

IsOpenShift

Places where OpenShift status results in a different implementation have been
extracted into separate, dedicated functions for clarity, including newTransport() and newCredentialsProvider(transport).

Repositories Tests in cmd

Now runs from a temp directory defined using XDG_CONFIG_HOME rather than
an in-memory-only mock and removed several chuncks of complexity that was
supporting the former methodology.

Flags Tests and Bugs

Added and upgraded flags tests, including fixing a few missing flag mappings.

/kind cleanup

[codecov]

Codecov Report

Merging #883 (6ca171f) into main (9a6aaa5) will increase coverage by 1.09%.
The diff coverage is 71.96%.

@@            Coverage Diff             @@
##             main     #883      +/-   ##
==========================================
+ Coverage   42.75%   43.85%   +1.09%     
==========================================
  Files          55       55              
  Lines        5183     5163      -20     
==========================================
+ Hits         2216     2264      +48     
+ Misses       2652     2576      -76     
- Partials      315      323       +8     

Impacted Files	Coverage Δ
docker/pusher.go	60.97% <0.00%> (-1.01%)	⬇️
cmd/invoke.go	35.77% <16.66%> (-0.13%)	⬇️
cmd/create.go	43.96% <26.66%> (-1.10%)	⬇️
cmd/info.go	27.14% <33.33%> (-0.64%)	⬇️
cmd/root.go	70.62% <69.23%> (-2.11%)	⬇️
cmd/repository.go	56.26% <80.00%> (+4.50%)	⬆️
cmd/client.go	80.85% <83.33%> (+61.98%)	⬆️
cmd/build.go	59.63% <100.00%> (-0.37%)	⬇️
cmd/delete.go	60.49% <100.00%> (-0.49%)	⬇️
cmd/deploy.go	44.44% <100.00%> (-0.31%)	⬇️
... and 5 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9a6aaa5...6ca171f. Read the comment docs.

[matejvasek]

@lkingland I still believe the factory should be settable from top level to NewRootCmd() for sake of overall wiring testing.

Lately we had issue where refactor broken --verbose flag wring.
That could have been easily detcted by unittest that could look like:

func TestVerboseFlagPropagation(t *testing.T) {

	tests := []struct {
		name string
		args []string
	}{
		{
			name: "verbose as sub-commands flag",
			args: []string{"build", "-v"},
		},
		{
			name: "verbose as roots flag",
			args: []string{"-v", "deploy"},
		},
		{
			name: "verbose as sub-sub-command flag",
			args: []string{"repositories", "list", "-v"},
		},
	}

	for _, tt := range tests {
		t.Run(tt.name, func(t *testing.T) {
			var err error
			defer viper.Reset()
			verboseSetByCommand := false
			cmd, err := NewRootCmd(RootCommandConfig{
				Name:    "func",
				Date:    "",
				Version: "0.0.0",
				Hash:    "",
				NewClient: func(opts ClientOptions) *fn.Client {
					verboseSetByCommand = opts.Verbose
					return fn.New() // noop client
				},
			})
			if err != nil {
				t.Fatal(err)
			}
			cmd.SetArgs(tt.args)
			err = cmd.Execute()
			if err != nil {
				t.Fatal(err)
			}
			if !verboseSetByCommand {
				t.Error("expected verbose to be set to true by the sub-command")
			}
		})

	}

}

(Note the code above wouldn't work right now because we are not injecting Load/Store functionality for loading fn.Function from func.yaml, to there is still dependency on filesystem, so the test would have to run from withing a directory containing some minimal func.yaml)

[lkingland]

I added a root-level ClientFactory and the following test for both the global flags, namespace and verbose. Thanks @matejvasek for the review; PTAL

func TestRoot_PersistentFlags(t *testing.T) {
  tests := []struct {
    name          string
    args          []string
    skipNamespace bool
  }{
    {
      name: "provided as root flags",
      args: []string{"--verbose", "--namespace=namespace", "list"},
    },
    {
      name: "provided as sub-command flags",
      args: []string{"list", "--verbose", "--namespace=namespace"},
    },
    {
      name:          "provided as sub-sub-command flags",
      args:          []string{"repositories", "list", "--verbose"},
      skipNamespace: true, // NOTE: no sub-sub commands yet use namespace, so it is not checked.
    },
  }

  for _, tt := range tests {
    t.Run(tt.name, func(t *testing.T) {
      // Create a new Function in a temp directory
      defer fromTempDir(t)()
      cmd := NewCreateCmd(NewClient)
      cmd.SetArgs([]string{"--language", "go"})
      if err := cmd.Execute(); err != nil {
        t.Fatal(err)
      }

      // Invoke the test's command, confirming global flags are populated in the ClientFactory's config
      cmd = NewRootCmd("func", Version{}, func(cfg ClientConfig, _ ...fn.Option) (*fn.Client, func()) {
        if cfg.Namespace != "namespace" && !tt.skipNamespace {
          t.Fatal("namespace not propagated")
        }
        if cfg.Verbose != true {
          t.Fatal("verbose not propagated")
        }
        return fn.New(), func() {}
      })
      cmd.SetArgs(tt.args)
      if err := cmd.Execute(); err != nil {
        t.Fatal(err)
      }
    })
  }
}

[lkingland]

I meant our own testing package, but if it has been added to Go, that even better.

There is one in the Go stdlib now, but we'll need to upgrade to go1.17 before we can use it, unfortunately.

The rebase was quite divergent, so I merged main this time. Thanks @matejvasek

[matejvasek]

Here and some other places there is removed propagation of newClient, instead the cmd.NewClient() is called directly.
I believe this shouldn't be done this way. It affect potential testability.

[lkingland]

Thanks @matejvasek !

This would be a good place for a test as well.

[lkingland]

The ClientFactory is now passed through in all situations

[matejvasek]

I think you might have misses on test file.

[matejvasek]

I had TestCreateConfig_RepositoriesPath specifically in mind here. Because of no propagation, it had to be rewritten -- accessing/testing internal details (newCreateConfig). With propagation it can be reverted I think.

[matejvasek]

I had TestCreateConfig_RepositoriesPath specifically in mind here. Because of no propagation it had to be rewritten -- accessing/testing internal details (newCreateConfig). With propagation it can be reverted I think.

or maybe not. The factory has been changed in a way it's not possible.

[lkingland]

I think you might have misses on test file.

It has been un-missed

[lkingland]

accessing/testing internal details (newCreateConfig)

Yes, I think this is probably just fine, because the use of XDG_CONFIG_HOME to derive the config home directory is an implementation detail of the func package's config, which is tested in config_test.go. The cmd package is only utilizing this feature in order to prepopulate a flag which ends up in the command's config object. The responsibility of the cmd package is to ensure this is plumbed through from flag to config. So each is testing their part of the puzzle; not each-others :)

As stated in the other comment, I would hope we can pull all environment-variable-dependent code out of the core and into main (or a helper package specifically for main, such as Global Config). That should make this division more clear.

[knative-prow]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lkingland, zroubalik

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 [lkingland,zroubalik]

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

[matejvasek]

Inability to do this test is exactly why I don't like using fn.Options which is totally opaque.
But I guess I can live with it.

[lkingland]

I understand the frustration! However this is as intended: XDG_CONFIG_HOME is an implementation detail of the func (core) package, and is therefore opaque to the command package. Tests don't directly verify it here to avoid coupling cmd and func packages. This is the same reason why tests in the func package are in actually in the func_test package.

It would be better to refactor the system such that environment variables are never used from anywhere outside of main, with their values plumbed through as needed. This would both improve the determinism of the system as well as make it the responsibility of the cmd (main) package to test them. Perhaps a task for another day

[matejvasek]

XDG_CONFIG_HOME is an implementation detail of the func (core) package, and is therefore opaque to the command package.

But then handing of XDG_CONFIG_HOME should be tested (and probably is) in core package and not here.

IMHO cmd pacakge can be free of dependency on fn core pacakge.
At least test should use only mock not actual fn.Client.

[matejvasek]

Inability to do this test is exactly why I don't like using fn.Options which is totally opaque.

I this forces us to test private functions in test which is not good IMHO.
Also it make tests more of a integration test, unittesting is difficult.

[matejvasek]

/lgtm
/hold