fix: the --registry flag is ignored when image: <imagename> exists in func.yaml (#1310)

* fix: add handling in case of differences between registry value and image tag (#1297)

* fix: add additional test cases (#1297)

cmd/build.go

@@ -3,6 +3,7 @@ package cmd
 import (
 	"errors"
 	"fmt"
+	"strings"
 
 	"github.com/AlecAivazis/survey/v2"
 	"github.com/AlecAivazis/survey/v2/terminal"
@@ -103,7 +104,7 @@ func runBuild(cmd *cobra.Command, _ []string, newClient ClientFactory) (err erro
 	// pertinent values from the config.
 	//
 	// NOTE: the checks for .Changed and altered conditionals for defaults will
-	// be removed when Global Config is integreated, because the config object
+	// be removed when Global Config is integrated, because the config object
 	// will at that time contain the final value for the attribute, taking into
 	// account whether or not the value was altered via flags or env variables.
 	// This condition is also only necessary for config members whose default
@@ -119,13 +120,26 @@ func runBuild(cmd *cobra.Command, _ []string, newClient ClientFactory) (err erro
 		// Sets default AND accepts any user-provided overrides
 		f.Registry = config.Registry
 	}
+	if config.Image != "" {
+		f.Image = config.Image
+	}
+	// Checks if there is a difference between defined registry and its value used as a prefix in the image tag
+	// In case of a mismatch a new image tag is created and used for build
+	// Do not react if image tag has been changed outside configuration
+	if f.Registry != "" && !cmd.Flags().Changed("image") && strings.Index(f.Image, "/") > 0 && !strings.HasPrefix(f.Image, f.Registry) {
+		prfx := f.Registry
+		if prfx[len(prfx)-1:] != "/" {
+			prfx = prfx + "/"
+		}
+		sps := strings.Split(f.Image, "/")
+		updImg := prfx + sps[len(sps)-1]
+		fmt.Fprintf(cmd.ErrOrStderr(), "Warning: image tag '%s' contains different registry than the specified '%s'. It will be overwritten with value '%s'\n", f.Image, f.Registry, updImg)
+		f.Image = updImg
+	}
 	if f.Build.Builder == "" || cmd.Flags().Changed("builder") {
 		// Sets default AND accepts any user-provided overrides
 		f.Build.Builder = config.Builder
 	}
-	if config.Image != "" {
-		f.Image = config.Image
-	}
 	if config.Builder != "" {
 		f.Build.Builder = config.Builder
 	}

cmd/build_test.go

@@ -2,8 +2,11 @@ package cmd
 
 import (
 	"errors"
+	"fmt"
 	"testing"
 
+	"gotest.tools/v3/assert"
+
 	fn "knative.dev/kn-plugin-func"
 	"knative.dev/kn-plugin-func/mock"
 )
@@ -62,7 +65,7 @@ func TestBuild_RegistryOrImageRequired(t *testing.T) {
 		}
 	}
 
-	// earlire test covers the --registry only case, test here that --image
+	// earlier test covers the --registry only case, test here that --image
 	// also succeeds.
 	cmd.SetArgs([]string{"--image=example.com/alice/myfunc"})
 	if err := cmd.Execute(); err != nil {
@@ -70,7 +73,7 @@ func TestBuild_RegistryOrImageRequired(t *testing.T) {
 	}
 }
 
-// TestBuild_InvalidRegistry ensures that providing an invalid resitry
+// TestBuild_InvalidRegistry ensures that providing an invalid registry
 // fails with the expected error.
 func TestBuild_InvalidRegistry(t *testing.T) {
 	testInvalidRegistry(NewBuildCmd, t)
@@ -140,7 +143,7 @@ func TestBuild_Push(t *testing.T) {
 		t.Fatal("push should be invoked when requested and a successful build")
 	}
 
-	// Exeute with push enabled but with a failed build
+	// Execute with push enabled but with a failed build
 	builder.BuildFn = func(f fn.Function) error {
 		return errors.New("mock error")
 	}
@@ -152,3 +155,75 @@ func TestBuild_Push(t *testing.T) {
 		t.Fatal("push should not be invoked on a failed build")
 	}
 }
+
+type buildWithRegistryTestCase struct {
+	desc        string
+	testFn      fn.Function
+	testFnArgs  []string
+	expRegistry string
+	expImage    string
+}
+
+func TestBuild_RegistryHandling(t *testing.T) {
+	root := fromTempDirectory(t)
+	for i, tc := range []buildWithRegistryTestCase{
+
+		{
+			desc: "should update func.yaml's image tag if mismatch with func.yaml's registry",
+			testFn: fn.Function{
+				Runtime:  "go",
+				Root:     root + "/1",
+				Registry: TestRegistry,              // defined as "example.com/alice"
+				Image:    "docker.io/tigerteam/foo", // image uses different registry in its tag, so it has to be updated
+			},
+			testFnArgs:  []string{"--path", root + "/1"},
+			expRegistry: TestRegistry,
+			expImage:    TestRegistry + "/foo",
+		},
+		{
+			desc: "should update func.yaml's image tag and registry if mismatch with --registry flag",
+			testFn: fn.Function{
+				Runtime:  "go",
+				Root:     root + "/2",
+				Registry: TestRegistry,
+				Image:    "docker.io/tigerteam/foo",
+			},
+			testFnArgs:  []string{"--path", root + "/2", "--registry", "example.com/test"}, // registry flag should overwrite func.yaml's image and registry
+			expRegistry: "example.com/test",
+			expImage:    "example.com/test/foo",
+		},
+
+		{
+			desc: "should NOT update func.yaml's registry if --image flag provided",
+			testFn: fn.Function{
+				Runtime:  "go",
+				Root:     root + "/3",
+				Registry: TestRegistry,
+				Image:    "docker.io/tigerteam/foo",
+			},
+			testFnArgs:  []string{"--path", root + "/3", "--image", "example.com/test/boo"}, // image flag should NOT overwrite func.yaml's registry
+			expRegistry: TestRegistry,
+			expImage:    "example.com/test/boo",
+		},
+	} {
+		var builder = mock.NewBuilder()
+		cmd := NewBuildCmd(NewClientFactory(func() *fn.Client {
+			return fn.New(fn.WithBuilder(builder))
+		}))
+		tci := i + 1
+		t.Logf("Test case %d: %s", tci, tc.desc)
+
+		err := fn.New().Create(tc.testFn)
+		assert.Assert(t, err == nil)
+
+		cmd.SetArgs(tc.testFnArgs)
+		err = cmd.Execute()
+		assert.Assert(t, err == nil)
+
+		f, err := fn.NewFunction(tc.testFn.Root)
+		assert.Assert(t, err == nil)
+
+		assert.Assert(t, f.Registry == tc.expRegistry, fmt.Sprintf("Test case %d: expected registry to be '"+tc.expRegistry+"', but got '%s'", tci, f.Registry))
+		assert.Assert(t, f.Image == tc.expImage, fmt.Sprintf("Test case %d: expected image to be '"+tc.expImage+"', but got '%s'", tci, f.Image))
+	}
+}

cmd/root_test.go

@@ -299,7 +299,7 @@ func piped(t *testing.T) func() string {
 // - creates a temp directory and changes to it as the new working directory
 // - creates a temp directory and uses it for XDG_CONFIG_HOME
 // - removes temp directories on cleanup
-// - resets viper on cleanum (the reason this is "cli-specific")
+// - resets viper on cleanup (the reason this is "cli-specific")
 func fromTempDirectory(t *testing.T) string {
 	t.Helper()
 	t.Setenv("XDG_CONFIG_HOME", t.TempDir())

function.go

@@ -171,7 +171,6 @@ func NewFunctionWith(defaults Function) Function {
 // selectively consider the minimal validation necessary to enable migration.
 func NewFunction(path string) (f Function, err error) {
 	f.Root = path // path is not persisted, as this is the purview of the FS itself
-	f.Root = path // path is not persisted, as this is the purvew of the FS itself
 	f.Build.BuilderImages = make(map[string]string)
 	f.Deploy.Annotations = make(map[string]string)
 	var filename = filepath.Join(path, FunctionFile)