fix: namespace isolation

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.
@@ -63,15 +68,15 @@ func NewClient(cfg ClientConfig, options ...fn.Option) (*fn.Client, func()) {
 		t  = newTransport(cfg.InsecureSkipVerify)    // may provide a custom impl which proxies
 		c  = newCredentialsProvider(config.Dir(), t) // for accessing registries
 		d  = newKnativeDeployer(cfg.Verbose)
-		pp = newTektonPipelinesProvider(cfg.Namespace, c, 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(

cmd/client_test.go

@@ -24,30 +24,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)
+	// 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)
 	}
-	f, err := fn.NewFunction("")
-	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(cmd *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(cmd *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/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 { // Onterwise 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
 }

cmd/delete_test.go

@@ -14,40 +14,48 @@ import (
 // with default options
 func TestDelete_Default(t *testing.T) {
 	var (
+		err       error
 		root      = FromTempDirectory(t)
-		namespace = "myns"
+		name      = "testdelete"
+		namespace = "testdeletedamespace"
 		remover   = mock.NewRemover()
-		err       error
+		ctx       = context.Background()
 	)
 
-	remover.RemoveFn = func(_, ns string) error {
+	remover.RemoveFn = func(n, ns string) error {
+		if name != name {
+			t.Fatalf("expected name '%v', got '%v'", name, n)
+		}
 		if ns != namespace {
-			t.Fatalf("expected delete namespace '%v', got '%v'", namespace, ns)
+			t.Fatalf("expected namespace '%v', got '%v'", namespace, ns)
 		}
 		return nil
 	}
 
+	client := fn.New(
+		fn.WithBuilder(mock.NewBuilder()),
+		fn.WithDeployer(mock.NewDeployer()),
+		fn.WithRemover(mock.NewRemover()),
+	)
+
 	// Ensure the extant function's namespace is used
 	f := fn.Function{
-		Root:     root,
-		Runtime:  "go",
-		Registry: TestRegistry,
-		Name:     "testname",
-		Deploy: fn.DeploySpec{
-			Namespace: namespace, //simulate deployed Function
-		},
+		Runtime:   "go",
+		Name:      name,
+		Namespace: namespace,
+		Root:      root,
+		Registry:  TestRegistry,
 	}
 
-	if f, err = fn.New().Init(f); err != nil {
+	if _, f, err = client.New(ctx, f); err != nil {
 		t.Fatal(err)
 	}
-
 	if err = f.Write(); err != nil {
 		t.Fatal(err)
 	}
 
 	cmd := NewDeleteCmd(NewTestClient(fn.WithRemover(remover)))
-	cmd.SetArgs([]string{}) //dont give any arguments to 'func delete' -- default
+	cmd.SetArgs([]string{})
 	if err := cmd.Execute(); err != nil {
 		t.Fatal(err)
 	}
@@ -99,7 +107,6 @@ func TestDelete_ByName(t *testing.T) {
 	// with a mocked remover.
 	cmd := NewDeleteCmd(NewTestClient(fn.WithRemover(remover)))
 	cmd.SetArgs([]string{testname}) // run: func delete <name>
-
 	if err := cmd.Execute(); err != nil {
 		t.Fatal(err)
 	}