fix: test and namespace isolation (#2187)

* fix: namespace logic cleanup and test isolation

- Pulls logic for defaulting to active namespace (K8S) moved UP to CLI during
  flag default calculation.
- Pushes logic of deciding between f.Namespace vs f.Deploy.Namespace down into
  implementations.
- Updates some tests which needed to have their environment cleared.
- Refactors Pipelines tests to use client API.
- Removes namespaces as a state variable all structures, instead passing as
  an argument.
- Moves FromTempDirectory to testing package for use outside cmd.

* fix spelling errors

* fix: pipelines remover always returned (empty) error

* update tekton namespace handling

* fix: remove deprecated GitLab config: max_concurrency

cmd/build_test.go

@@ -6,6 +6,7 @@ import (
 
 	fn "knative.dev/func/pkg/functions"
 	"knative.dev/func/pkg/mock"
+	. "knative.dev/func/pkg/testing"
 )
 
 // TestBuild_BuilderPersists ensures that the builder chosen is read from
@@ -100,7 +101,7 @@ func TestBuild_Authentication(t *testing.T) {
 // - Push not triggered after an unsuccessful build
 // - Push can be disabled
 func TestBuild_Push(t *testing.T) {
-	root := fromTempDirectory(t)
+	root := FromTempDirectory(t)
 
 	f := fn.Function{
 		Root:     root,

cmd/client.go

@@ -21,11 +21,6 @@ import (
 // These are the minimum settings necessary to create the default client
 // instance which has most subsystems initialized.
 type ClientConfig struct {
-	// Namespace in the remote cluster to use for any client commands which
-	// touch the remote.  Optional.  Empty namespace indicates the namespace
-	// currently configured in the client's connection should be used.
-	Namespace string
-
 	// Verbose logging.  By default, logging output is kept to the bare minimum.
 	// Use this flag to configure verbose logging throughout.
 	Verbose bool
@@ -62,16 +57,16 @@ func NewClient(cfg ClientConfig, options ...fn.Option) (*fn.Client, func()) {
 	var (
 		t  = newTransport(cfg.InsecureSkipVerify)    // may provide a custom impl which proxies
 		c  = newCredentialsProvider(config.Dir(), t) // for accessing registries
-		d  = newKnativeDeployer(cfg.Namespace, cfg.Verbose)
-		pp = newTektonPipelinesProvider(cfg.Namespace, c, cfg.Verbose)
+		d  = newKnativeDeployer(cfg.Verbose)
+		pp = newTektonPipelinesProvider(c, cfg.Verbose)
 		o  = []fn.Option{ // standard (shared) options for all commands
 			fn.WithVerbose(cfg.Verbose),
 			fn.WithTransport(t),
 			fn.WithRepositoriesPath(config.RepositoriesPath()),
 			fn.WithBuilder(buildpacks.NewBuilder(buildpacks.WithVerbose(cfg.Verbose))),
 			fn.WithRemover(knative.NewRemover(cfg.Verbose)),
-			fn.WithDescriber(knative.NewDescriber(cfg.Namespace, cfg.Verbose)),
-			fn.WithLister(knative.NewLister(cfg.Namespace, cfg.Verbose)),
+			fn.WithDescriber(knative.NewDescriber(cfg.Verbose)),
+			fn.WithLister(knative.NewLister(cfg.Verbose)),
 			fn.WithDeployer(d),
 			fn.WithPipelinesProvider(pp),
 			fn.WithPusher(docker.NewPusher(
@@ -117,9 +112,8 @@ func newCredentialsProvider(configPath string, t http.RoundTripper) docker.Crede
 	return creds.NewCredentialsProvider(configPath, options...)
 }
 
-func newTektonPipelinesProvider(namespace string, creds docker.CredentialsProvider, verbose bool) *tekton.PipelinesProvider {
+func newTektonPipelinesProvider(creds docker.CredentialsProvider, verbose bool) *tekton.PipelinesProvider {
 	options := []tekton.Opt{
-		tekton.WithNamespace(namespace),
 		tekton.WithCredentialsProvider(creds),
 		tekton.WithVerbose(verbose),
 		tekton.WithPipelineDecorator(deployDecorator{}),
@@ -128,9 +122,8 @@ func newTektonPipelinesProvider(namespace string, creds docker.CredentialsProvid
 	return tekton.NewPipelinesProvider(options...)
 }
 
-func newKnativeDeployer(namespace string, verbose bool) fn.Deployer {
+func newKnativeDeployer(verbose bool) fn.Deployer {
 	options := []knative.DeployerOpt{
-		knative.WithDeployerNamespace(namespace),
 		knative.WithDeployerVerbose(verbose),
 		knative.WithDeployerDecorator(deployDecorator{}),
 	}

cmd/client_test.go

@@ -8,8 +8,6 @@ import (
 	"knative.dev/func/pkg/mock"
 )
 
-const namespace = "func"
-
 // Test_NewTestClient ensures that the convenience method for
 // constructing a mocked client for testing properly considers options:
 // options provided to the factory constructor are considered exaustive,
@@ -24,30 +22,34 @@ func Test_NewTestClient(t *testing.T) {
 
 	// Factory constructor options which should be used when invoking later
 	clientFn := NewTestClient(fn.WithRemover(remover))
+
 	// Factory should ignore options provided when invoking
 	client, _ := clientFn(ClientConfig{}, fn.WithDescriber(describer))
 
-	// Trigger an invocation of the mocks
-	err := client.Remove(context.Background(), fn.Function{Name: "test", Deploy: fn.DeploySpec{Namespace: namespace}}, true)
-	if err != nil {
-		t.Fatal(err)
-	}
-	f, err := fn.NewFunction("")
+	// Trigger an invocation of the mocks by running the associated client
+	// methods which depend on them
+	err := client.Remove(context.Background(), "myfunc", "myns", fn.Function{}, true)
 	if err != nil {
 		t.Fatal(err)
 	}
-	_, err = client.Describe(context.Background(), "test", f)
+	_, err = client.Describe(context.Background(), "myfunc", "myns", fn.Function{})
 	if err != nil {
 		t.Fatal(err)
 	}
 
-	// Ensure the first set of options, held on the factory, were used
+	// Ensure the first set of options, held on the factory (the mock remover)
+	// is invoked.
 	if !remover.RemoveInvoked {
 		t.Fatalf("factory (outer) options not carried through to final client instance")
 	}
-	// Ensure the second set of options, provided when constructing the
-	// client using the factory, were ignored
+	// Ensure the second set of options, provided when constructing the client
+	// using the factory, are ignored.
 	if describer.DescribeInvoked {
 		t.Fatalf("test client factory should ignore options when invoked.")
 	}
+
+	// This ensures that the NewTestClient function, when provided a set
+	// of optional implementations (mocks) will override any which are set
+	// by commands, allowing tests to "force" a command to use the mocked
+	// implementations.
 }

cmd/completion_util.go

@@ -15,9 +15,9 @@ import (
 )
 
 func CompleteFunctionList(cmd *cobra.Command, args []string, toComplete string) (strings []string, directive cobra.ShellCompDirective) {
-	lister := knative.NewLister("", false)
+	lister := knative.NewLister(false)
 
-	list, err := lister.List(cmd.Context())
+	list, err := lister.List(cmd.Context(), "")
 	if err != nil {
 		directive = cobra.ShellCompDirectiveError
 		return

cmd/config_git_remove.go

@@ -25,7 +25,7 @@ func NewConfigGitRemoveCmd(newClient ClientFactory) *cobra.Command {
 	such as local generated Pipelines resources and any resources generated on the cluster.
 	`,
 		SuggestFor: []string{"rem", "rmeove", "del", "dle"},
-		PreRunE:    bindEnv("path", "namespace", "delete-local", "delete-cluster"),
+		PreRunE:    bindEnv("path", "delete-local", "delete-cluster"),
 		RunE: func(cmd *cobra.Command, args []string) (err error) {
 			return runConfigGitRemoveCmd(cmd, newClient)
 		},
@@ -37,20 +37,6 @@ func NewConfigGitRemoveCmd(newClient ClientFactory) *cobra.Command {
 		fmt.Fprintf(cmd.OutOrStdout(), "error loading config at '%v'. %v\n", config.File(), err)
 	}
 
-	// Function Context
-	f, _ := fn.NewFunction(effectivePath())
-	if f.Initialized() {
-		cfg = cfg.Apply(f)
-	}
-
-	// Flags
-	//
-	// Globally-Configurable Flags:
-	//   Options whose value may be defined globally may also exist on the
-	//  contextually relevant function; but sets are flattened via cfg.Apply(f)
-	cmd.Flags().StringP("namespace", "n", cfg.Namespace,
-		"Deploy into a specific namespace. Will use function's current namespace by default if already deployed, and the currently active namespace if it can be determined. ($FUNC_NAMESPACE)")
-
 	// Resources generated related Flags:
 	cmd.Flags().Bool("delete-local", false, "Delete local resources (pipeline templates).")
 	cmd.Flags().Bool("delete-cluster", false, "Delete cluster resources (credentials and config on the cluster).")
@@ -69,8 +55,6 @@ type configGitRemoveConfig struct {
 	// working directory of the process.
 	Path string
 
-	Namespace string
-
 	// informs whether any specific flag for deleting only a subset of resources has been set
 	flagSet bool
 
@@ -93,8 +77,6 @@ func newConfigGitRemoveConfig(_ *cobra.Command) (c configGitRemoveConfig) {
 	}
 
 	c = configGitRemoveConfig{
-		Namespace: viper.GetString("namespace"),
-
 		flagSet: flagSet,
 
 		metadata: pipelines.PacMetadata{
@@ -181,7 +163,7 @@ func runConfigGitRemoveCmd(cmd *cobra.Command, newClient ClientFactory) (err err
 		return
 	}
 
-	client, done := newClient(ClientConfig{Namespace: cfg.Namespace, Verbose: cfg.Verbose})
+	client, done := newClient(ClientConfig{Verbose: cfg.Verbose})
 	defer done()
 
 	return client.RemovePAC(cmd.Context(), f, cfg.metadata)

cmd/config_git_set.go

@@ -25,7 +25,7 @@ func NewConfigGitSetCmd(newClient ClientFactory) *cobra.Command {
 	directory or from the directory specified with --path.
 	`,
 		SuggestFor: []string{"add", "ad", "update", "create", "insert", "append"},
-		PreRunE:    bindEnv("path", "builder", "builder-image", "image", "registry", "namespace", "git-provider", "git-url", "git-branch", "git-dir", "gh-access-token", "config-local", "config-cluster", "config-remote"),
+		PreRunE:    bindEnv("path", "builder", "builder-image", "image", "registry", "git-provider", "git-url", "git-branch", "git-dir", "gh-access-token", "config-local", "config-cluster", "config-remote"),
 		RunE: func(cmd *cobra.Command, args []string) (err error) {
 			return runConfigGitSetCmd(cmd, newClient)
 		},
@@ -93,8 +93,6 @@ func NewConfigGitSetCmd(newClient ClientFactory) *cobra.Command {
 type configGitSetConfig struct {
 	buildConfig // further embeds config.Global
 
-	Namespace string
-
 	GitProvider   string
 	GitURL        string
 	GitRevision   string
@@ -127,7 +125,6 @@ func newConfigGitSetConfig(_ *cobra.Command) (c configGitSetConfig) {
 
 	c = configGitSetConfig{
 		buildConfig: newBuildConfig(),
-		Namespace:   viper.GetString("namespace"),
 
 		GitURL:        viper.GetString("git-url"),
 		GitRevision:   viper.GetString("git-branch"),
@@ -307,7 +304,7 @@ func runConfigGitSetCmd(cmd *cobra.Command, newClient ClientFactory) (err error)
 		return
 	}
 
-	client, done := newClient(ClientConfig{Namespace: cfg.Namespace, Verbose: cfg.Verbose}, fn.WithRegistry(cfg.Registry))
+	client, done := newClient(ClientConfig{Verbose: cfg.Verbose}, fn.WithRegistry(cfg.Registry))
 	defer done()
 
 	return client.ConfigurePAC(cmd.Context(), f, cfg.metadata)

cmd/create_test.go

@@ -4,13 +4,14 @@ import (
 	"errors"
 	"testing"
 
+	. "knative.dev/func/pkg/testing"
 	"knative.dev/func/pkg/utils"
 )
 
 // TestCreate_Execute ensures that an invocation of create with minimal settings
 // and valid input completes without error; degenerate case.
 func TestCreate_Execute(t *testing.T) {
-	_ = fromTempDirectory(t)
+	_ = FromTempDirectory(t)
 
 	cmd := NewCreateCmd(NewClient)
 	cmd.SetArgs([]string{"--language", "go", "myfunc"})
@@ -23,7 +24,7 @@ func TestCreate_Execute(t *testing.T) {
 // TestCreate_NoRuntime ensures that an invocation of create must be
 // done with a runtime.
 func TestCreate_NoRuntime(t *testing.T) {
-	_ = fromTempDirectory(t)
+	_ = FromTempDirectory(t)
 
 	cmd := NewCreateCmd(NewClient)
 	cmd.SetArgs([]string{"myfunc"}) // Do not use test command args
@@ -38,7 +39,7 @@ func TestCreate_NoRuntime(t *testing.T) {
 // TestCreate_WithNoRuntime ensures that an invocation of create must be
 // done with one of the valid runtimes only.
 func TestCreate_WithInvalidRuntime(t *testing.T) {
-	_ = fromTempDirectory(t)
+	_ = FromTempDirectory(t)
 
 	cmd := NewCreateCmd(NewClient)
 	cmd.SetArgs([]string{"--language", "invalid", "myfunc"})
@@ -53,7 +54,7 @@ func TestCreate_WithInvalidRuntime(t *testing.T) {
 // TestCreate_InvalidTemplate ensures that an invocation of create must be
 // done with one of the valid templates only.
 func TestCreate_InvalidTemplate(t *testing.T) {
-	_ = fromTempDirectory(t)
+	_ = FromTempDirectory(t)
 
 	cmd := NewCreateCmd(NewClient)
 	cmd.SetArgs([]string{"--language", "go", "--template", "invalid", "myfunc"})
@@ -68,7 +69,7 @@ func TestCreate_InvalidTemplate(t *testing.T) {
 // TestCreate_ValidatesName ensures that the create command only accepts
 // DNS-1123 labels for function name.
 func TestCreate_ValidatesName(t *testing.T) {
-	_ = fromTempDirectory(t)
+	_ = FromTempDirectory(t)
 
 	// Execute the command with a function name containing invalid characters and
 	// confirm the expected error is returned
@@ -84,7 +85,7 @@ func TestCreate_ValidatesName(t *testing.T) {
 // TestCreate_ConfigOptional ensures that the system can be used without
 // any additional configuration being required.
 func TestCreate_ConfigOptional(t *testing.T) {
-	_ = fromTempDirectory(t)
+	_ = FromTempDirectory(t)
 
 	t.Setenv("XDG_CONFIG_HOME", t.TempDir())