From 80e54c7804da8172717c6fb3db690e3415c83088 Mon Sep 17 00:00:00 2001
From: gauron99 <fridrich.david19@gmail.com>
Date: Wed, 31 Jan 2024 21:20:42 +0100
Subject: [PATCH] small changes to remover and ns added to its tests

Signed-off-by: gauron99 <fridrich.david19@gmail.com>
---
 cmd/client_test.go      |  4 +++-
 cmd/delete_test.go      | 13 +++++++++----
 pkg/functions/client.go | 15 +++++++++++++--
 pkg/knative/remover.go  | 12 ++----------
 4 files changed, 27 insertions(+), 17 deletions(-)

diff --git a/cmd/client_test.go b/cmd/client_test.go
index aa9652be0c..b7a63f1b5a 100644
--- a/cmd/client_test.go
+++ b/cmd/client_test.go
@@ -8,6 +8,8 @@ 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,
@@ -26,7 +28,7 @@ func Test_NewTestClient(t *testing.T) {
 	client, _ := clientFn(ClientConfig{}, fn.WithDescriber(describer))
 
 	// Trigger an invocation of the mocks
-	err := client.Remove(context.Background(), fn.Function{Name: "test"}, true)
+	err := client.Remove(context.Background(), fn.Function{Name: "test", Deploy: fn.DeploySpec{Namespace: namespace}}, true)
 	if err != nil {
 		t.Fatal(err)
 	}
diff --git a/cmd/delete_test.go b/cmd/delete_test.go
index 32b2501ac3..a26dee8a3d 100644
--- a/cmd/delete_test.go
+++ b/cmd/delete_test.go
@@ -61,10 +61,11 @@ func TestDelete_Default(t *testing.T) {
 // function explicitly as an argument invokes the remover appropriately.
 func TestDelete_ByName(t *testing.T) {
 	var (
-		root     = fromTempDirectory(t)
-		testname = "testname"        // explicit name for the function
-		remover  = mock.NewRemover() // with a mock remover
-		err      error
+		root          = fromTempDirectory(t)
+		testname      = "testname"        // explicit name for the function
+		testnamespace = "testnamespace"   // explicit namespace for the function
+		remover       = mock.NewRemover() // with a mock remover
+		err           error
 	)
 
 	// Remover fails the test if it receives the incorrect name
@@ -86,9 +87,13 @@ func TestDelete_ByName(t *testing.T) {
 		t.Fatal(err)
 	}
 
+	// simulate deployed namespace for the client Remover
+	f.Deploy.Namespace = testnamespace
+
 	if err = f.Write(); err != nil {
 		t.Fatal(err)
 	}
+
 	// Create a command with a client constructor fn that instantiates a client
 	// with a mocked remover.
 	cmd := NewDeleteCmd(NewTestClient(fn.WithRemover(remover)))
diff --git a/pkg/functions/client.go b/pkg/functions/client.go
index 7224bfe127..9a1aad0910 100644
--- a/pkg/functions/client.go
+++ b/pkg/functions/client.go
@@ -977,6 +977,8 @@ func (c *Client) Remove(ctx context.Context, cfg Function, deleteAll bool) error
 	// If name is provided, it takes precedence.
 	// Otherwise load the function defined at root.
 	functionName := cfg.Name
+	functionNamespace := cfg.Deploy.Namespace
+
 	if cfg.Name == "" {
 		f, err := NewFunction(cfg.Root)
 		if err != nil {
@@ -986,22 +988,31 @@ func (c *Client) Remove(ctx context.Context, cfg Function, deleteAll bool) error
 			return fmt.Errorf("function at %v can not be removed unless initialized. Try removing by name", f.Root)
 		}
 		functionName = f.Name
+		if functionNamespace == "" && f.Deploy.Namespace != "" {
+			functionNamespace = f.Deploy.Namespace
+		}
 		cfg = f
 	}
+
 	if functionName == "" {
 		return ErrNameRequired
 	}
+	if functionNamespace == "" {
+		return ErrNamespaceRequired
+	}
 
 	// Delete Knative Service and dependent resources in parallel
-	fmt.Fprintf(os.Stderr, "Removing Knative Service: %v in namespace '%v'\n", functionName, cfg.Deploy.Namespace)
+	fmt.Fprintf(os.Stderr, "Removing Knative Service: %v in namespace '%v'\n", functionName, functionNamespace)
 	errChan := make(chan error)
 	go func() {
-		errChan <- c.remover.Remove(ctx, functionName, cfg.Deploy.Namespace)
+		errChan <- c.remover.Remove(ctx, functionName, functionNamespace)
 	}()
 
 	var errResources error
 	if deleteAll {
 		fmt.Fprintf(os.Stderr, "Removing Knative Service '%v' and all dependent resources\n", functionName)
+		// TODO: might not be necessary
+		cfg.Deploy.Namespace = functionNamespace
 		errResources = c.pipelinesProvider.Remove(ctx, cfg)
 	}
 
diff --git a/pkg/knative/remover.go b/pkg/knative/remover.go
index adeb73f79f..eeb76d2bf1 100644
--- a/pkg/knative/remover.go
+++ b/pkg/knative/remover.go
@@ -3,6 +3,7 @@ package knative
 import (
 	"context"
 	"fmt"
+	"os"
 	"time"
 
 	apiErrors "k8s.io/apimachinery/pkg/api/errors"
@@ -22,17 +23,8 @@ type Remover struct {
 }
 
 func (remover *Remover) Remove(ctx context.Context, name, ns string) (err error) {
-	// if namespace is not provided for any reason, use the active namespace
-	// otherwise just throw an error. I dont think this should default to any namespace
-	// because its a remover, therefore we dont want to just assume a default namespace
-	// to delete a function from. Use provided, get the current one or none at all.
-
-	if ns == "" {
-		ns = ActiveNamespace()
-	}
-
 	if ns == "" {
-		fmt.Print("normal error in Remove, no namespace found here\n")
+		fmt.Fprintf(os.Stderr, "no namespace defined when trying to delete a function in knative remover")
 		return fn.ErrNamespaceRequired
 	}