fix test to have now required namespace

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

cmd/client_test.go

@@ -18,6 +18,7 @@ func Test_NewTestClient(t *testing.T) {
 	var (
 		remover   = mock.NewRemover()
 		describer = mock.NewDescriber()
+		namespace = "func"
 	)
 
 	// Factory constructor options which should be used when invoking later
@@ -26,7 +27,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)
 	}

cmd/delete.go

@@ -69,7 +69,7 @@ func runDelete(cmd *cobra.Command, args []string, newClient ClientFactory) (err
 		return
 	}
 
-	// check that name is defined when deliting a Function in specific namespace
+	// 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")
 	}
@@ -98,16 +98,18 @@ func runDelete(cmd *cobra.Command, args []string, newClient ClientFactory) (err
 
 		// --- Namespace determination ---
 
-		// use the function's extant namespace -- already deployed Function
-		if !cmd.Flags().Changed("namespace") {
+		// use the function's extant namespace -- already deployed Function if viable
+		// cfg.Namespace can be got from global config (is flag unchanged?)
+		if !cmd.Flags().Changed("namespace") && function.Deploy.Namespace != "" {
 			cfg.Namespace = function.Deploy.Namespace
 		}
-
-		// if user specified one, override (example: manually written to func.yaml)
-		if function.Namespace != "" {
-			cfg.Namespace = function.Namespace
-		}
-
+	}
+	ff, err := fn.NewFunction(cfg.Path)
+	if err != nil {
+		return
+	}
+	if cfg.Namespace == "" {
+		cfg.Namespace = ff.Deploy.Namespace
 	}
 
 	// assign final namespace to function to be passed to client.Remove

cmd/delete_test.go

@@ -170,10 +170,9 @@ func TestDelete_ByProject(t *testing.T) {
 
 	// Write a func.yaml config which specifies a name
 	funcYaml := `name: bar
-namespace: ""
+namespace: "func"
 runtime: go
 image: ""
-imageDigest: ""
 builder: quay.io/boson/faas-go-builder
 builders:
   default: quay.io/boson/faas-go-builder
@@ -218,16 +217,18 @@ func TestDelete_ByPath(t *testing.T) {
 	var (
 
 		// A mock remover which will be sampled to ensure it is not invoked.
-		remover = mock.NewRemover()
-		root    = fromTempDirectory(t)
-		err     error
+		remover   = mock.NewRemover()
+		root      = fromTempDirectory(t)
+		err       error
+		namespace = "func"
 	)
 
 	// Ensure the extant function's namespace is used
 	f := fn.Function{
 		Root:     root,
 		Runtime:  "go",
 		Registry: TestRegistry,
+		Deploy:   fn.DeploySpec{Namespace: namespace},
 	}
 
 	// Initialize a function in temp dir

pkg/functions/client.go

@@ -516,7 +516,7 @@ func (c *Client) New(ctx context.Context, cfg Function) (string, Function, error
 		return route, f, err
 	}
 
-	fmt.Fprint(os.Stderr, "Done")
+	fmt.Fprint(os.Stderr, "Done\n")
 
 	return route, f, err
 }
@@ -968,8 +968,9 @@ func (c *Client) List(ctx context.Context) ([]ListItem, error) {
 	return c.lister.List(ctx)
 }
 
-// Remove a function.  Name takes precedence.  If no name is provided,
-// the function defined at root is used if it exists.
+// Remove a function. Name takes precedence. If no name is provided, the
+// function defined at root is used if it exists. If calling this directly
+// namespace must be provided in .Deploy.Namespace field.
 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.
@@ -989,8 +990,12 @@ func (c *Client) Remove(ctx context.Context, cfg Function, deleteAll bool) error
 		return ErrNameRequired
 	}
 
+	if cfg.Deploy.Namespace == "" {
+		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, cfg.Deploy.Namespace)
 	errChan := make(chan error)
 	go func() {
 		errChan <- c.remover.Remove(ctx, functionName, cfg.Deploy.Namespace)

pkg/functions/client_int_test.go

@@ -130,9 +130,9 @@ func TestDeploy(t *testing.T) {
 func TestDeployWithOptions(t *testing.T) {
 	root, cleanup := Mktemp(t)
 	defer cleanup()
-	verbose := true
+	verbose := false
 
-	f := fn.Function{Runtime: "go", Name: "test-deploy-with-options", Root: root}
+	f := fn.Function{Runtime: "go", Name: "test-deploy-with-options", Root: root, Namespace: DefaultNamespace}
 	f.Deploy = fn.DeploySpec{
 		Options: fn.Options{
 			Scale: &fn.ScaleOptions{
@@ -575,7 +575,7 @@ func newClient(verbose bool) *fn.Client {
 func del(t *testing.T, c *fn.Client, name string) {
 	t.Helper()
 	waitFor(t, c, name)
-	if err := c.Remove(context.Background(), fn.Function{Name: name}, false); err != nil {
+	if err := c.Remove(context.Background(), fn.Function{Name: name, Deploy: fn.DeploySpec{Namespace: DefaultNamespace}}, false); err != nil {
 		t.Fatal(err)
 	}
 	cli, _, err := docker.NewClient(client.DefaultDockerHost)

pkg/functions/client_test.go

@@ -970,6 +970,7 @@ func TestClient_Remove_ByPath(t *testing.T) {
 		root         = "testdata/example.com/test-remove-by-path"
 		expectedName = "test-remove-by-path"
 		remover      = mock.NewRemover()
+		namespace    = "func"
 	)
 
 	defer Using(t, root)()
@@ -980,7 +981,7 @@ func TestClient_Remove_ByPath(t *testing.T) {
 
 	var f fn.Function
 	var err error
-	if _, f, err = client.New(context.Background(), fn.Function{Runtime: TestRuntime, Root: root}); err != nil {
+	if _, f, err = client.New(context.Background(), fn.Function{Runtime: TestRuntime, Root: root, Namespace: namespace}); err != nil {
 		t.Fatal(err)
 	}
 
@@ -1011,6 +1012,7 @@ func TestClient_Remove_DeleteAll(t *testing.T) {
 		remover           = mock.NewRemover()
 		pipelinesProvider = mock.NewPipelinesProvider()
 		deleteAll         = true
+		namespace         = "func"
 	)
 
 	defer Using(t, root)()
@@ -1022,7 +1024,7 @@ func TestClient_Remove_DeleteAll(t *testing.T) {
 
 	var f fn.Function
 	var err error
-	if _, f, err = client.New(context.Background(), fn.Function{Runtime: TestRuntime, Root: root}); err != nil {
+	if _, f, err = client.New(context.Background(), fn.Function{Runtime: TestRuntime, Root: root, Namespace: namespace}); err != nil {
 		t.Fatal(err)
 	}
 
@@ -1057,6 +1059,7 @@ func TestClient_Remove_Dont_DeleteAll(t *testing.T) {
 		remover           = mock.NewRemover()
 		pipelinesProvider = mock.NewPipelinesProvider()
 		deleteAll         = false
+		namespace         = "func"
 	)
 
 	defer Using(t, root)()
@@ -1068,7 +1071,7 @@ func TestClient_Remove_Dont_DeleteAll(t *testing.T) {
 
 	var f fn.Function
 	var err error
-	if _, f, err = client.New(context.Background(), fn.Function{Runtime: TestRuntime, Root: root}); err != nil {
+	if _, f, err = client.New(context.Background(), fn.Function{Runtime: TestRuntime, Root: root, Namespace: namespace}); err != nil {
 		t.Fatal(err)
 	}
 
@@ -1100,6 +1103,7 @@ func TestClient_Remove_ByName(t *testing.T) {
 		root         = "testdata/example.com/testRemoveByName"
 		expectedName = "explicitName.example.com"
 		remover      = mock.NewRemover()
+		namespace    = "func"
 	)
 
 	defer Using(t, root)()
@@ -1119,13 +1123,13 @@ func TestClient_Remove_ByName(t *testing.T) {
 		return nil
 	}
 
-	// Run remove with only a name
-	if err := client.Remove(context.Background(), fn.Function{Name: expectedName}, false); err != nil {
+	// Run remove with name (and namespace since client Remove is being called directly)
+	if err := client.Remove(context.Background(), fn.Function{Name: expectedName, Deploy: fn.DeploySpec{Namespace: namespace}}, false); err != nil {
 		t.Fatal(err)
 	}
 
 	// Run remove with a name and a root, which should be ignored in favor of the name.
-	if err := client.Remove(context.Background(), fn.Function{Name: expectedName, Root: root}, false); err != nil {
+	if err := client.Remove(context.Background(), fn.Function{Name: expectedName, Root: root, Deploy: fn.DeploySpec{Namespace: namespace}}, false); err != nil {
 		t.Fatal(err)
 	}
 

pkg/knative/deployer.go

@@ -33,6 +33,9 @@ import (
 const LIVENESS_ENDPOINT = "/health/liveness"
 const READINESS_ENDPOINT = "/health/readiness"
 
+// static default namespace
+const DefaultNamespace = "func"
+
 type DeployDecorator interface {
 	UpdateAnnotations(fn.Function, map[string]string) map[string]string
 	UpdateLabels(fn.Function, map[string]string) map[string]string
@@ -50,10 +53,10 @@ type Deployer struct {
 	decorator DeployDecorator
 }
 
-// DefaultNamespace attempts to read the kubernetes active namepsace.
+// ActiveNamespace attempts to read the kubernetes active namepsace.
 // Missing configs or not having an active kuberentes configuration are
 // equivalent to having no default namespace (empty string).
-func DefaultNamespace() string {
+func ActiveNamespace() string {
 	// Get client config, if it exists, and from that the namespace
 	ns, _, err := k8s.GetClientConfig().Namespace()
 	if err != nil {
@@ -92,17 +95,17 @@ func WithDeployerDecorator(decorator DeployDecorator) DeployerOpt {
 
 // Checks the status of the "user-container" for the ImagePullBackOff reason meaning that
 // the container image is not reachable probably because a private registry is being used.
-func (d *Deployer) isImageInPrivateRegistry(ctx context.Context, client clientservingv1.KnServingClient, funcName string) bool {
-	ksvc, err := client.GetService(ctx, funcName)
+func (d *Deployer) isImageInPrivateRegistry(ctx context.Context, client clientservingv1.KnServingClient, f fn.Function) bool {
+	ksvc, err := client.GetService(ctx, f.Name)
 	if err != nil {
 		return false
 	}
 	k8sClient, err := k8s.NewKubernetesClientset()
 	if err != nil {
 		return false
 	}
-	list, err := k8sClient.CoreV1().Pods(d.Namespace).List(ctx, metav1.ListOptions{
-		LabelSelector: "serving.knative.dev/revision=" + ksvc.Status.LatestCreatedRevisionName + ",serving.knative.dev/service=" + funcName,
+	list, err := k8sClient.CoreV1().Pods(namespace(d.Namespace, f)).List(ctx, metav1.ListOptions{
+		LabelSelector: "serving.knative.dev/revision=" + ksvc.Status.LatestCreatedRevisionName + ",serving.knative.dev/service=" + f.Name,
 		FieldSelector: "status.phase=Pending",
 	})
 	if err != nil {
@@ -120,32 +123,45 @@ func (d *Deployer) isImageInPrivateRegistry(ctx context.Context, client clientse
 	return false
 }
 
-func (d *Deployer) Deploy(ctx context.Context, f fn.Function) (fn.DeploymentResult, error) {
-	// f.Namespace is now the "desired Namespace" got from configs.
-	// deployer.Namespace is used for WithDeployer Method therefore it is kept.
+// returns correct namespace to deploy to, ordered in a descending order by
+// priority: User specified via cli -> client WithDeployer -> already deployed ->
+// -> k8s default; if fails, use static default
+func namespace(dflt string, f fn.Function) string {
+	// namespace ordered by highest priority decending
+	namespace := f.Namespace
 
-	var err error
-	// set default first
-	d.Namespace, err = k8s.GetDefaultNamespace()
-	if err != nil {
-		return fn.DeploymentResult{}, err
+	// if deployed before: use already deployed namespace
+	if namespace == "" {
+		namespace = f.Deploy.Namespace
 	}
 
-	// if deployed before: use already deployed namespace
-	if f.Deploy.Namespace != "" {
-		d.Namespace = f.Deploy.Namespace
+	// deployer WithDeployerNamespace provided
+	if namespace == "" {
+		namespace = dflt
 	}
 
-	// if namespace was specified: use desired namespace
-	if f.Namespace != "" {
-		d.Namespace = f.Namespace
+	if namespace == "" {
+		var err error
+		// still not set, just use the defaultest default
+		namespace, err = k8s.GetDefaultNamespace()
+		if err != nil {
+			fmt.Fprintf(os.Stderr, "trying to get default namespace returns an error: %s\n", err)
+			namespace = DefaultNamespace
+		}
 	}
+	return namespace
+}
+
+func (d *Deployer) Deploy(ctx context.Context, f fn.Function) (fn.DeploymentResult, error) {
+
+	// returns correct namespace combining
+	namespace := namespace(d.Namespace, f)
 
-	client, err := NewServingClient(d.Namespace)
+	client, err := NewServingClient(namespace)
 	if err != nil {
 		return fn.DeploymentResult{}, err
 	}
-	eventingClient, err := NewEventingClient(d.Namespace)
+	eventingClient, err := NewEventingClient(namespace)
 	if err != nil {
 		return fn.DeploymentResult{}, err
 	}
@@ -158,7 +174,7 @@ func (d *Deployer) Deploy(ctx context.Context, f fn.Function) (fn.DeploymentResu
 	}
 	since := time.Now()
 	go func() {
-		_ = GetKServiceLogs(ctx, d.Namespace, f.Name, f.Deploy.Image, &since, out)
+		_ = GetKServiceLogs(ctx, namespace, f.Name, f.Deploy.Image, &since, out)
 	}()
 
 	previousService, err := client.GetService(ctx, f.Name)
@@ -175,7 +191,7 @@ func (d *Deployer) Deploy(ctx context.Context, f fn.Function) (fn.DeploymentResu
 				return fn.DeploymentResult{}, err
 			}
 
-			err = checkResourcesArePresent(ctx, d.Namespace, &referencedSecrets, &referencedConfigMaps, &referencedPVCs, f.Deploy.ServiceAccountName)
+			err = checkResourcesArePresent(ctx, namespace, &referencedSecrets, &referencedConfigMaps, &referencedPVCs, f.Deploy.ServiceAccountName)
 			if err != nil {
 				err = fmt.Errorf("knative deployer failed to generate the Knative Service: %v", err)
 				return fn.DeploymentResult{}, err
@@ -196,7 +212,7 @@ func (d *Deployer) Deploy(ctx context.Context, f fn.Function) (fn.DeploymentResu
 				private := false
 				for !private {
 					time.Sleep(5 * time.Second)
-					private = d.isImageInPrivateRegistry(ctx, client, f.Name)
+					private = d.isImageInPrivateRegistry(ctx, client, f)
 					chprivate <- private
 				}
 				close(chprivate)
@@ -249,12 +265,12 @@ func (d *Deployer) Deploy(ctx context.Context, f fn.Function) (fn.DeploymentResu
 			}
 
 			if d.verbose {
-				fmt.Printf("Function deployed in namespace %q and exposed at URL:\n%s\n", d.Namespace, route.Status.URL.String())
+				fmt.Printf("Function deployed in namespace %q and exposed at URL:\n%s\n", namespace, route.Status.URL.String())
 			}
 			return fn.DeploymentResult{
 				Status:    fn.Deployed,
 				URL:       route.Status.URL.String(),
-				Namespace: d.Namespace,
+				Namespace: namespace,
 			}, nil
 
 		} else {
@@ -277,7 +293,7 @@ func (d *Deployer) Deploy(ctx context.Context, f fn.Function) (fn.DeploymentResu
 			return fn.DeploymentResult{}, err
 		}
 
-		err = checkResourcesArePresent(ctx, d.Namespace, &referencedSecrets, &referencedConfigMaps, &referencedPVCs, f.Deploy.ServiceAccountName)
+		err = checkResourcesArePresent(ctx, namespace, &referencedSecrets, &referencedConfigMaps, &referencedPVCs, f.Deploy.ServiceAccountName)
 		if err != nil {
 			err = fmt.Errorf("knative deployer failed to update the Knative Service: %v", err)
 			return fn.DeploymentResult{}, err
@@ -315,7 +331,7 @@ func (d *Deployer) Deploy(ctx context.Context, f fn.Function) (fn.DeploymentResu
 		return fn.DeploymentResult{
 			Status:    fn.Updated,
 			URL:       route.Status.URL.String(),
-			Namespace: d.Namespace,
+			Namespace: namespace,
 		}, nil
 	}
 }

pkg/knative/deployer_test.go

@@ -20,8 +20,8 @@ func Test_DefaultNamespace(t *testing.T) {
 	// "test-ns-deploy"
 	t.Setenv("KUBECONFIG", fmt.Sprintf("%s/testdata/test_default_namespace", cwd()))
 
-	if DefaultNamespace() != "test-ns-deploy" {
-		t.Fatalf("expected 'test-ns-deploy', got '%v'", DefaultNamespace())
+	if ActiveNamespace() != "test-ns-deploy" {
+		t.Fatalf("expected 'test-ns-deploy', got '%v'", ActiveNamespace())
 	}
 }