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
  deployer implementations.
- Updates some tests which needed to have their environment cleared.
- Removes namespaces as a state variable on various structures, shifting it
  to an argument.
- Moves FromTempDirectory to testing package for use outside cmd.

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,10 +21,15 @@ 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
+	// PipelinesNamespace 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
+	//
+	// DEPRECATED:  This is being removed.  Individual commands should use
+	// either a supplied --namespace flag, the current active k8s context,
+	// the global default (if defined) or the static default "default", in
+	// that order.
+	PipelinesNamespace string
 
 	// Verbose logging.  By default, logging output is kept to the bare minimum.
 	// Use this flag to configure verbose logging throughout.
@@ -62,16 +67,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(cfg.PipelinesNamespace, 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(
@@ -128,9 +133,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

@@ -69,7 +69,7 @@ type configGitRemoveConfig struct {
 	// working directory of the process.
 	Path string
 
-	Namespace string
+	PipelinesNamespace string
 
 	// informs whether any specific flag for deleting only a subset of resources has been set
 	flagSet bool
@@ -93,7 +93,7 @@ func newConfigGitRemoveConfig(_ *cobra.Command) (c configGitRemoveConfig) {
 	}
 
 	c = configGitRemoveConfig{
-		Namespace: viper.GetString("namespace"),
+		PipelinesNamespace: viper.GetString("namespace"),
 
 		flagSet: flagSet,
 
@@ -181,7 +181,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{PipelinesNamespace: cfg.PipelinesNamespace, Verbose: cfg.Verbose})
 	defer done()
 
 	return client.RemovePAC(cmd.Context(), f, cfg.metadata)

cmd/config_git_set.go

@@ -93,7 +93,7 @@ func NewConfigGitSetCmd(newClient ClientFactory) *cobra.Command {
 type configGitSetConfig struct {
 	buildConfig // further embeds config.Global
 
-	Namespace string
+	PipelinesNamespace string
 
 	GitProvider   string
 	GitURL        string
@@ -126,8 +126,8 @@ func newConfigGitSetConfig(_ *cobra.Command) (c configGitSetConfig) {
 	}
 
 	c = configGitSetConfig{
-		buildConfig: newBuildConfig(),
-		Namespace:   viper.GetString("namespace"),
+		buildConfig:        newBuildConfig(),
+		PipelinesNamespace: viper.GetString("namespace"),
 
 		GitURL:        viper.GetString("git-url"),
 		GitRevision:   viper.GetString("git-branch"),
@@ -307,7 +307,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{PipelinesNamespace: cfg.PipelinesNamespace, 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())
 

cmd/delete.go

@@ -28,7 +28,7 @@ No local files are deleted.
 {{rootCmdUse}} delete
 
 # Undeploy the function 'myfunc' in namespace 'apps'
-{{rootCmdUse}} delete -n apps myfunc
+{{rootCmdUse}} delete myfunc --namespace apps
 `,
 		SuggestFor:        []string{"remove", "del"},
 		Aliases:           []string{"rm"},
@@ -47,7 +47,7 @@ No local files are deleted.
 	}
 
 	// Flags
-	cmd.Flags().StringP("namespace", "n", cfg.Namespace, "The namespace in which to delete. ($FUNC_NAMESPACE)")
+	cmd.Flags().StringP("namespace", "n", defaultNamespace(fn.Function{}, false), "The namespace when deleting by name. ($FUNC_NAMESPACE)")
 	cmd.Flags().StringP("all", "a", "true", "Delete all resources created for a function, eg. Pipelines, Secrets, etc. ($FUNC_ALL) (allowed values: \"true\", \"false\")")
 	addConfirmFlag(cmd, cfg.Confirm)
 	addPathFlag(cmd)
@@ -57,77 +57,63 @@ No local files are deleted.
 }
 
 func runDelete(cmd *cobra.Command, args []string, newClient ClientFactory) (err error) {
-	cfg, err := newDeleteConfig(args).Prompt()
+	cfg, err := newDeleteConfig(cmd, args)
 	if err != nil {
 		return
 	}
-
-	// check that name is defined when deleting a Function in specific namespace
-	if cfg.Name == "" && cfg.Namespace != "" {
-		return fmt.Errorf("function name is required when namespace is specified")
+	if cfg, err = cfg.Prompt(); err != nil {
+		return
 	}
 
-	var function fn.Function
-	// initialize namespace from the config
-	var namespace = cfg.Namespace
+	client, done := newClient(ClientConfig{Verbose: cfg.Verbose})
+	defer done()
 
-	// Initialize func with explicit name (when provided)
-	if len(args) > 0 && args[0] != "" {
-		pathChanged := cmd.Flags().Changed("path")
-		if pathChanged {
-			return fmt.Errorf("only one of --path and [NAME] should be provided")
-		}
-		function = fn.Function{
-			Name: args[0],
-		}
-	} else {
-		function, err = fn.NewFunction(cfg.Path)
+	if cfg.Name != "" { // Delete by name if provided
+		return client.Remove(cmd.Context(), cfg.Name, cfg.Namespace, fn.Function{}, cfg.All)
+	} else { // Otherwise; delete the function at path (cwd by default)
+		f, err := fn.NewFunction(cfg.Path)
 		if err != nil {
-			return
-		}
-
-		// Check if the function has been initialized
-		if !function.Initialized() {
-			return fn.NewErrNotInitialized(function.Root)
-		}
-
-		// use the function's extant namespace -- already deployed function
-		if !cmd.Flags().Changed("namespace") && function.Deploy.Namespace != "" {
-			namespace = function.Deploy.Namespace
+			return err
 		}
+		return client.Remove(cmd.Context(), "", "", f, cfg.All)
 	}
-
-	// Create a client instance from the now-final config
-	client, done := newClient(ClientConfig{Namespace: namespace, Verbose: cfg.Verbose})
-	defer done()
-
-	function.Deploy.Namespace = namespace
-	// Invoke remove using the concrete client impl
-	return client.Remove(cmd.Context(), function, cfg.DeleteAll)
 }
 
 type deleteConfig struct {
 	Name      string
 	Namespace string
 	Path      string
-	DeleteAll bool
+	All       bool
 	Verbose   bool
 }
 
 // newDeleteConfig returns a config populated from the current execution context
 // (args, flags and environment variables)
-func newDeleteConfig(args []string) deleteConfig {
+func newDeleteConfig(cmd *cobra.Command, args []string) (cfg deleteConfig, err error) {
 	var name string
 	if len(args) > 0 {
 		name = args[0]
 	}
-	return deleteConfig{
-		Path:      viper.GetString("path"),
+	cfg = deleteConfig{
+		All:       viper.GetBool("all"),
+		Name:      name, // args[0] or derived
 		Namespace: viper.GetString("namespace"),
-		DeleteAll: viper.GetBool("all"),
-		Name:      deriveName(name, viper.GetString("path")), // args[0] or derived
-		Verbose:   viper.GetBool("verbose"),                  // defined on root
+		Path:      viper.GetString("path"),
+		Verbose:   viper.GetBool("verbose"), // defined on root
+	}
+	if cfg.Name == "" && cmd.Flags().Changed("namespace") {
+		// logicially inconsistent to supply only a namespace.
+		// Either use the function's local state in its entirety, or specify
+		// both a name and a namespace to ignore any local function source.
+		err = fmt.Errorf("must also specify a name when specifying namespace.")
+	}
+	if cfg.Name != "" && cmd.Flags().Changed("path") {
+		// logically inconsistent to provide both a name and a path to source.
+		// Either use the function's local state on disk (--path), or specify
+		// a name and a namespace to ignore any local function source.
+		err = fmt.Errorf("only one of --path and [NAME] should be provided")
 	}
+	return
 }
 
 // Prompt the user with value of config members, allowing for interaractive changes.
@@ -151,7 +137,7 @@ func (c deleteConfig) Prompt() (deleteConfig, error) {
 			Name: "all",
 			Prompt: &survey.Confirm{
 				Message: "Do you want to delete all resources?",
-				Default: c.DeleteAll,
+				Default: c.All,
 			},
 		},
 	}
@@ -166,7 +152,7 @@ func (c deleteConfig) Prompt() (deleteConfig, error) {
 	}
 
 	dc.Name = answers.Name
-	dc.DeleteAll = answers.All
+	dc.All = answers.All
 
 	return dc, err
 }