fix actions - return value, configApplied and registry on subsequent …

…deploy different

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

cmd/deploy.go

@@ -256,11 +256,13 @@ func runDeploy(cmd *cobra.Command, newClient ClientFactory) (err error) {
 	if cfg.Namespace != oldF.Deploy.Namespace && oldF.Deploy.Namespace != "" {
 		// TODO: when new prompt is implemented, add here a "are you sure?" check possibly
 
-		// fmt.Fprintf(cmd.OutOrStdout(), "Info: Deleting old func in '%s' because the namespace has changed\n", oldF.Deploy.Namespace)
+		fmt.Fprintf(cmd.OutOrStdout(), "Info: Deleting old func in '%s' because the namespace has changed\n", oldF.Deploy.Namespace)
 		oldClient, doneOld := newClient(ClientConfig{Namespace: oldF.Deploy.Namespace, Verbose: cfg.Verbose}, clientOptions...)
 		defer doneOld()
-		oldClient.Remove(cmd.Context(), oldF, true)
-
+		err = oldClient.Remove(cmd.Context(), oldF, true)
+		if err != nil {
+			return
+		}
 	}
 
 	// check if --image was provided with a digest
@@ -325,18 +327,38 @@ func runDeploy(cmd *cobra.Command, newClient ClientFactory) (err error) {
 // optional build step.
 func build(cmd *cobra.Command, flag string, f fn.Function, client *fn.Client, buildOptions []fn.BuildOption) (fn.Function, error) {
 	var err error
+
+	// TODO: gauron99 - This checks whether a registry has changed on a subsequent build
+	// decision (Like on func deploy --registry=X when already built image is Y)
+	// This can probably be added into f.Built() function for simpler and more global
+	// implementation
+	registryChangedSubsequently := false
+	if !strings.Contains(f.Build.Image, f.Registry) && f.Image == "" && f.Build.Image != "" {
+		registryChangedSubsequently = true
+	}
+
+	build, _ := strconv.ParseBool(flag)
 	if flag == "auto" {
-		if f.Built() {
+		if f.Built() && !registryChangedSubsequently {
 			fmt.Fprintln(cmd.OutOrStdout(), "function up-to-date. Force rebuild with --build")
 		} else {
 			if f, err = client.Build(cmd.Context(), f, buildOptions...); err != nil {
 				return f, err
 			}
 		}
-	} else if build, _ := strconv.ParseBool(flag); build {
+	} else if build {
 		if f, err = client.Build(cmd.Context(), f, buildOptions...); err != nil {
 			return f, err
 		}
+	} else if !build && registryChangedSubsequently {
+		// This else-if-branch solves edge case problem.
+		// This happens when --registry is given with deploy when a different registry
+		// has been used to build image previously.
+		// Ex.:
+		// step 1) Init -> Build with: --registry example.com/alice
+		// step 2) Deploy with: --registry example.com/fred --build=false
+		fmt.Fprintf(cmd.OutOrStdout(), "Error: --build flag was disabled but you most likely provided new registry '%v'. Enable build (--build=true) to override or build beforehand with your new registry to apply it\n", f.Registry)
+		return f, fmt.Errorf("new registry '%v' was provided but build was disabled, cannot update image name", f.Registry)
 	} else if _, err = strconv.ParseBool(flag); err != nil {
 		return f, fmt.Errorf("--build ($FUNC_BUILD) %q not recognized.  Should be 'auto' or a truthy value such as 'true', 'false', '0', or '1'.", flag)
 

cmd/deploy_test.go

@@ -193,6 +193,9 @@ func testConfigApplied(cmdFn commandConstructor, t *testing.T) {
 	if err := cmdFn(clientFn).Execute(); err != nil {
 		t.Fatal(err)
 	}
+	if err := f.Write(); err != nil {
+		t.Fatal(err)
+	}
 	if f, err = fn.NewFunction(root); err != nil {
 		t.Fatal(err)
 	}
@@ -1546,3 +1549,68 @@ func Test_ValidateBuilder(t *testing.T) {
 		t.Fatalf("did not get expected error validating an invalid builder name")
 	}
 }
+
+// TestReDeploy_ErrorOnRegistryChangeWithoutBuild tests that subsequent deploy
+// with different --registry and --build disabled throws an error because client
+// wasnt able to build on a registry change
+func TestReDeploy_ErrorOnRegistryChangeWithoutBuild(t *testing.T) {
+	// Change profile to one whose current profile is 'test-ns-deploy'
+	kubeconfig := filepath.Join(cwd(), "testdata", "TestReDeploy_ErrorOnRegistryChangeWithoutBuild/kubeconfig")
+	root := fromTempDirectory(t)
+	t.Setenv("KUBECONFIG", kubeconfig)
+
+	// Create a basic go Function
+	f := fn.Function{
+		Runtime: "go",
+		Root:    root,
+	}
+	_, err := fn.New().Init(f)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// create build cmd
+	cmdBuild := NewBuildCmd(NewTestClient(fn.WithBuilder(mock.NewBuilder())))
+	cmdBuild.SetArgs([]string{"--registry=" + TestRegistry})
+
+	// First: prebuild Function
+	if err := cmdBuild.Execute(); err != nil {
+		t.Fatal(err)
+	}
+
+	// change namespace and deploy again
+	newRegistry := "example.com/fred"
+
+	// Redeploy the function without specifying namespace.
+	cmd := NewDeployCmd(NewTestClient(
+		fn.WithDeployer(mock.NewDeployer()),
+	))
+
+	cmd.SetArgs([]string{"--registry=" + newRegistry, "--build=false"})
+
+	stdout := strings.Builder{}
+	cmd.SetOut(&stdout)
+
+	// Second: Deploy with different registry and build disabled (check err here)
+	err = cmd.Execute()
+
+	expectedPrint := fmt.Sprintf("Error: --build flag was disabled but you most likely provided new registry '%v'. Enable build (--build=true) to override or build beforehand with your new registry to apply it\n", newRegistry)
+	expectedError := fmt.Sprintf("new registry '%v' was provided but build was disabled, cannot update image name", newRegistry)
+
+	// ASSERT
+
+	// Ensure output contained error message
+	if !strings.Contains(stdout.String(), expectedPrint) {
+		t.Log("STDOUT:\n" + stdout.String())
+		t.Fatalf("Expected message not found:\n%v", expectedPrint)
+	}
+
+	// ensure we get the right error
+	if err == nil {
+		t.Fatal("expected error but got nil")
+	}
+
+	if err.Error() != expectedError {
+		t.Fatalf("expected error message '%v' but got '%v'", expectedError, err.Error())
+	}
+}

pkg/functions/client_test.go

@@ -21,6 +21,7 @@ import (
 	"time"
 
 	cloudevents "github.com/cloudevents/sdk-go/v2"
+	cmd "knative.dev/func/cmd"
 	"knative.dev/func/pkg/builders"
 	fn "knative.dev/func/pkg/functions"
 	"knative.dev/func/pkg/mock"
@@ -1949,3 +1950,51 @@ func TestClient_BuildCleanFingerprint(t *testing.T) {
 }
 
 // TestClient_BuildPopulatesImage
+
+// Test_RemoveInvokedOnOldFunction checks that Remover was invoked after a
+// subsequent redeploy to a new namespace.
+// specifically: deploy to 'nsone', redeploy to 'nstwo' with --namespace flag
+// overriding the ns and expect the remover to be invoked for old Function in ns 'nsone'
+func TestClient_RemoveInvokedOnOldFunction(t *testing.T) {
+	// Create a temporary directory
+	root, cleanup := Mktemp(t)
+	defer cleanup()
+
+	nsOne := "nsone"
+	nsTwo := "nstwo"
+
+	// A mock remover which fails if the name from the func.yaml is not received.
+	remover := mock.NewRemover()
+
+	client := fn.New()
+	// initialize function with namespace defined as nsone
+	_, err := client.Init(fn.Function{Runtime: "go", Root: root,
+		Deploy: fn.DeploySpec{Namespace: nsOne}})
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// 	// set deploy command
+	cmd := cmd.NewDeployCmd(cmd.NewTestClient(
+		fn.WithDeployer(mock.NewDeployer()),
+		fn.WithRegistry(TestRegistry),
+		fn.WithRemover(remover),
+	))
+
+	// set arguments for deploy command and execute the command
+	cmd.SetArgs([]string{fmt.Sprintf("--namespace=%s", nsOne)})
+	if err = cmd.Execute(); err != nil {
+		t.Fatal(err)
+	}
+
+	// now deploy again with a change of namespace via flag -- should delete old Func
+	cmd.SetArgs([]string{fmt.Sprintf("--namespace=%s", nsTwo)})
+	if err = cmd.Execute(); err != nil {
+		t.Fatal(err)
+	}
+
+	// check that a remove was invoked getting rid of the old Function
+	if !remover.RemoveInvoked {
+		t.Fatal(fmt.Errorf("remover was not invoked on an old function"))
+	}
+}

pkg/knative/integration_test.go

@@ -130,7 +130,7 @@ func TestIntegration(t *testing.T) {
 		Created: now,
 		Deploy: fn.DeploySpec{
 			// TODO: gauron99 - is it okay to have this explicitly set to deploy.image already?
-			// With this I skip the logic of setting the .Deploy.Image field which should be fine for this test, I think?
+			// With this I skip the logic of setting the .Deploy.Image field but it should be fine for this test
 			Image:     "quay.io/mvasek/func-test-service@sha256:2eca4de00d7569c8791634bdbb0c4d5ec8fb061b001549314591e839dabd5269",
 			Namespace: namespace,
 			Labels:    []fn.Label{{Key: ptr("my-label"), Value: ptr("my-label-value")}},