error definition; fix client tests; cli delete fixup

Signed-off-by: gauron99 <[email protected]>

cmd/delete.go

@@ -57,11 +57,23 @@ No local files are deleted.
 }
 
 func runDelete(cmd *cobra.Command, args []string, newClient ClientFactory) (err error) {
+
+	// TODO: (possibly) gauron99 -- I think this whole process of deletion might
+	// be cleaned up more for example: passing a function as a config from which
+	// only namespace and name is used, in sub-functions of client.Remove this is
+	// also the case. Furthermore the namespace being assigned to cfg&f around the
+	// place might be also on the list.
+
 	cfg, err := newDeleteConfig(args).Prompt()
 	if err != nil {
 		return
 	}
 
+	// check that name is defined when deliting a Function in specific namespace
+	if cfg.Name == "" && cfg.Namespace != "" {
+		return fmt.Errorf("function name is required when namespace is specified")
+	}
+
 	var function fn.Function
 
 	// Initialize func with explicit name (when provided)
@@ -84,13 +96,23 @@ func runDelete(cmd *cobra.Command, args []string, newClient ClientFactory) (err
 			return fn.NewErrNotInitialized(function.Root)
 		}
 
-		// If not provided, use the function's extant namespace
+		// --- Namespace determination ---
+
+		// use the function's extant namespace -- already deployed Function
 		if !cmd.Flags().Changed("namespace") {
 			cfg.Namespace = function.Deploy.Namespace
 		}
 
+		// if user specified one, override (example: manually written to func.yaml)
+		if function.Namespace != "" {
+			cfg.Namespace = function.Namespace
+		}
+
 	}
 
+	// assign final namespace to function to be passed to client.Remove
+	function.Deploy.Namespace = cfg.Namespace
+
 	// Create a client instance from the now-final config
 	client, done := newClient(ClientConfig{Namespace: cfg.Namespace, Verbose: cfg.Verbose})
 	defer done()

cmd/delete_test.go

@@ -2,92 +2,101 @@ package cmd
 
 import (
 	"os"
-	"path/filepath"
 	"testing"
 
 	fn "knative.dev/func/pkg/functions"
 	"knative.dev/func/pkg/mock"
 )
 
-// TestDelete_Namespace ensures that the namespace provided to the client
-// for use when deleting a function is set
-// 1. The flag /env variable if provided
-// 2. The namespace of the function at path if provided
-// 3. The user's current active namespace
-func TestDelete_Namespace(t *testing.T) {
-	root := fromTempDirectory(t)
-
-	// Ensure that the default is "default" when no context can be identified
-	t.Setenv("KUBECONFIG", filepath.Join(cwd(), "nonexistent"))
-	t.Setenv("KUBERNETES_SERVICE_HOST", "")
-	cmd := NewDeleteCmd(func(cc ClientConfig, options ...fn.Option) (*fn.Client, func()) {
-		if cc.Namespace != "" {
-			t.Fatalf("expected '', got '%v'", cc.Namespace)
+// TestDelete_Default ensures that the deployed Function is deleted correctly
+// with default options
+func TestDelete_Default(t *testing.T) {
+	var (
+		root = fromTempDirectory(t)
+		// ctx       = context.Background()
+		namespace = "myns"
+		remover   = mock.NewRemover()
+		err       error
+	)
+
+	remover.RemoveFn = func(_, ns string) error {
+		if ns != namespace {
+			t.Fatalf("expected delete namespace '%v', got '%v'", namespace, ns)
 		}
-		return fn.New(), func() {}
-	})
-	cmd.SetArgs([]string{"somefunc"}) // delete by name such that no f need be created
-	if err := cmd.Execute(); err != nil {
-		t.Fatal(err)
+		return nil
 	}
 
 	// Ensure the extant function's namespace is used
 	f := fn.Function{
-		Root:    root,
-		Runtime: "go",
+		Root:     root,
+		Runtime:  "go",
+		Registry: TestRegistry,
+		Name:     "testname",
 		Deploy: fn.DeploySpec{
-			Namespace: "deployed",
+			Namespace: namespace, //simulate deployed Function
 		},
 	}
-	if _, err := fn.New().Init(f); err != nil {
+
+	if f, err = fn.New().Init(f); err != nil {
 		t.Fatal(err)
 	}
-	cmd = NewDeleteCmd(func(cc ClientConfig, options ...fn.Option) (*fn.Client, func()) {
-		if cc.Namespace != "deployed" {
-			t.Fatalf("expected 'deployed', got '%v'", cc.Namespace)
-		}
-		return fn.New(), func() {}
-	})
-	cmd.SetArgs([]string{})
-	if err := cmd.Execute(); err != nil {
+
+	if err = f.Write(); err != nil {
 		t.Fatal(err)
 	}
 
-	// Ensure an explicit namespace is plumbed through
-	cmd = NewDeleteCmd(func(cc ClientConfig, options ...fn.Option) (*fn.Client, func()) {
-		if cc.Namespace != "ns" {
-			t.Fatalf("expected 'ns', got '%v'", cc.Namespace)
-		}
-		return fn.New(), func() {}
-	})
-	cmd.SetArgs([]string{"--namespace", "ns"})
+	cmd := NewDeleteCmd(NewTestClient(fn.WithRemover(remover)))
+	cmd.SetArgs([]string{}) //dont give any arguments to 'func delete' -- default
 	if err := cmd.Execute(); err != nil {
 		t.Fatal(err)
 	}
 
+	// Fail if remover's .Remove not invoked at all
+	if !remover.RemoveInvoked {
+		t.Fatal("fn.Remover not invoked")
+	}
 }
 
 // TestDelete_ByName ensures that running delete specifying the name of the
 // function explicitly as an argument invokes the remover appropriately.
 func TestDelete_ByName(t *testing.T) {
 	var (
-		testname = "testname"        // explicit name for the function
-		remover  = mock.NewRemover() // with a mock remover
+		root      = fromTempDirectory(t)
+		testname  = "testname" // explicit name for the function
+		namespace = "myns"
+		remover   = mock.NewRemover() // with a mock remover
+		err       error
 	)
 
 	// Remover fails the test if it receives the incorrect name
-	// an incorrect name.
-	remover.RemoveFn = func(n string) error {
+	remover.RemoveFn = func(n, _ string) error {
 		if n != testname {
 			t.Fatalf("expected delete name %v, got %v", testname, n)
 		}
 		return nil
 	}
 
+	f := fn.Function{
+		Root:     root,
+		Runtime:  "go",
+		Registry: TestRegistry,
+		Name:     "testname",
+		Deploy: fn.DeploySpec{
+			Namespace: namespace, //simulate deployed Function in this namespace
+		},
+	}
+
+	if f, err = fn.New().Init(f); err != nil {
+		t.Fatal(err)
+	}
+
+	if err = f.Write(); err != nil {
+		t.Fatal(err)
+	}
 	// Create a command with a client constructor fn that instantiates a client
-	// with a the mocked remover.
+	// with a mocked remover.
 	cmd := NewDeleteCmd(NewTestClient(fn.WithRemover(remover)))
-	cmd.SetArgs([]string{testname})
+	cmd.SetArgs([]string{testname}) // func delete <name>
 
 	if err := cmd.Execute(); err != nil {
 		t.Fatal(err)
@@ -99,6 +108,61 @@ func TestDelete_ByName(t *testing.T) {
 	}
 }
 
+// TestDelete_Namespace ensures that the deployed Function is deleted correctly
+// with namespace option
+func TestDelete_Namespace(t *testing.T) {
+	var (
+		root      = fromTempDirectory(t)
+		namespace = "myns"
+		remover   = mock.NewRemover()
+		testname  = "testname"
+		err       error
+	)
+
+	remover.RemoveFn = func(_, ns string) error {
+		if ns != namespace {
+			t.Fatalf("expected delete namespace '%v', got '%v'", namespace, ns)
+		}
+		return nil
+	}
+
+	// Ensure the extant function's namespace is used
+	f := fn.Function{
+		Root:     root,
+		Runtime:  "go",
+		Registry: TestRegistry,
+		Deploy: fn.DeploySpec{
+			Namespace: namespace,
+		},
+	}
+
+	if f, err = fn.New().Init(f); err != nil {
+		t.Fatal(err)
+	}
+
+	if err = f.Write(); err != nil {
+		t.Fatal(err)
+	}
+
+	cmd := NewDeleteCmd(NewTestClient(fn.WithRemover(remover)))
+	cmd.SetArgs([]string{testname, "--namespace", namespace})
+	if err := cmd.Execute(); err != nil {
+		t.Fatal(err)
+	}
+}
+
+// TestDelete_NamespaceWithoutNameFails ensures that providing wrong argument
+// combination fails nice and fast (no name of the Function)
+func TestDelete_NamespaceWithoutNameFails(t *testing.T) {
+	_ = fromTempDirectory(t)
+
+	cmd := NewDeleteCmd(NewTestClient())
+	cmd.SetArgs([]string{"--namespace=myns"})
+	if err := cmd.Execute(); err == nil {
+		t.Fatal("invoking Delete with namespace BUT without name provided anywhere")
+	}
+}
+
 // TestDelete_ByProject ensures that running delete with a valid project as its
 // context invokes remove and with the correct name (reads name from func.yaml)
 func TestDelete_ByProject(t *testing.T) {
@@ -124,7 +188,7 @@ created: 2021-01-01T00:00:00+00:00
 
 	// A mock remover which fails if the name from the func.yaml is not received.
 	remover := mock.NewRemover()
-	remover.RemoveFn = func(n string) error {
+	remover.RemoveFn = func(n, _ string) error {
 		if n != "bar" {
 			t.Fatalf("expected name 'bar', got '%v'", n)
 		}

pkg/functions/client.go

@@ -127,7 +127,7 @@ type Runner interface {
 // Remover of deployed services.
 type Remover interface {
 	// Remove the function from remote.
-	Remove(ctx context.Context, name string) error
+	Remove(ctx context.Context, name string, namespace string) error
 }
 
 // Lister of deployed functions.
@@ -755,6 +755,30 @@ func (c *Client) Deploy(ctx context.Context, f Function, opts ...DeployOption) (
 		return f, err
 	}
 
+	// If Redeployment to NEW namespace was successful -- undeploy dangling Function in old namespace.
+	// On forced namespace change (using --namespace flag)
+	if f.Namespace != f.Deploy.Namespace && f.Deploy.Namespace != "" {
+		// TODO: when new prompt is implemented, add here a "are you sure?" check possibly
+		if c.verbose {
+			fmt.Fprintf(os.Stderr, "Info: Deleting old func in '%s' because the namespace has changed\n", f.Deploy.Namespace)
+		}
+
+		// c.Remove removes a Function in f.Deploy.Namespace which removes the OLD Function
+		// because its not updated yet (see few lines below)
+		err = c.Remove(ctx, f, true)
+
+		// Warn when service is not found and set err to nil to continue. Function's
+		// service mightve been manually deleted prior to the subsequent deploy or the
+		// namespace is already deleted therefore there is nothing to delete
+		if ErrFunctionNotFound != err {
+			fmt.Fprintf(os.Stderr, "Warning: Cant undeploy Function in namespace '%s' - service not found. Namespace/Service might be deleted already\n", f.Deploy.Namespace)
+			err = nil
+		}
+		if err != nil {
+			return f, err
+		}
+	}
+
 	// Update the function with the namespace into which the function was
 	// deployed
 	f.Deploy.Namespace = result.Namespace
@@ -969,7 +993,7 @@ func (c *Client) Remove(ctx context.Context, cfg Function, deleteAll bool) error
 	fmt.Fprintf(os.Stderr, "Removing Knative Service: %v in namespace %v\n", functionName, cfg.Deploy.Namespace)
 	errChan := make(chan error)
 	go func() {
-		errChan <- c.remover.Remove(ctx, functionName)
+		errChan <- c.remover.Remove(ctx, functionName, cfg.Deploy.Namespace)
 	}()
 
 	var errResources error
@@ -1279,14 +1303,14 @@ func (n *noopPusher) Push(ctx context.Context, f Function) (string, error) { ret
 // Deployer
 type noopDeployer struct{ output io.Writer }
 
-func (n *noopDeployer) Deploy(ctx context.Context, _ Function) (DeploymentResult, error) {
-	return DeploymentResult{}, nil
+func (n *noopDeployer) Deploy(ctx context.Context, f Function) (DeploymentResult, error) {
+	return DeploymentResult{Namespace: f.Namespace}, nil
 }
 
 // Remover
 type noopRemover struct{ output io.Writer }
 
-func (n *noopRemover) Remove(context.Context, string) error { return nil }
+func (n *noopRemover) Remove(context.Context, string, string) error { return nil }
 
 // Lister
 type noopLister struct{ output io.Writer }