Fix: odo init overwrites personalized configuration when downloading starter project (#6800)

* Fix personalized configuration overwritten by starter project

Signed-off-by: Parthvi Vala <pvala@redhat.com>

* Update pkg/registry/registry_test.go

Co-authored-by: Armel Soro <armel@rm3l.org>
Signed-off-by: Parthvi Vala <pvala@redhat.com>

* Adding intergration tests for personalizing configurations with odo init

Signed-off-by: Ritu Deshmukh <rideshmu@redhat.com>

Modified changes

Signed-off-by: Ritu Deshmukh <rideshmu@redhat.com>

Update tests/integration/interactive_init_test.go

Co-authored-by: Parthvi Vala <pvala@redhat.com>

Update tests/integration/interactive_init_test.go

Co-authored-by: Parthvi Vala <pvala@redhat.com>

Update interactive_init_test.go

Update interactive_init_test.go

Update interactive_init_test.go

Update tests/integration/interactive_init_test.go

Co-authored-by: Parthvi Vala <pvala@redhat.com>

Update interactive_init_test.go

* Updated changes

Signed-off-by: Ritu Deshmukh <rideshmu@redhat.com>

* Attempt at fixing interactive tests

Signed-off-by: Parthvi Vala <pvala@redhat.com>

* Add multiple devfile version check

Co-authored-by: Armel Soro <asoro@redhat.com>
Signed-off-by: Parthvi Vala <pvala@redhat.com>

---------

Signed-off-by: Parthvi Vala <pvala@redhat.com>
Signed-off-by: Ritu Deshmukh <rideshmu@redhat.com>
Co-authored-by: Armel Soro <armel@rm3l.org>
Co-authored-by: Ritu Deshmukh <rideshmu@redhat.com>
Co-authored-by: Armel Soro <asoro@redhat.com>

pkg/init/init.go

@@ -221,15 +221,15 @@ func (o *InitClient) SelectStarterProject(devfile parser.DevfileObj, flags map[s
 	return backend.SelectStarterProject(devfile, flags)
 }
 
-func (o *InitClient) DownloadStarterProject(starter *v1alpha2.StarterProject, dest string) error {
+func (o *InitClient) DownloadStarterProject(starter *v1alpha2.StarterProject, dest string) (containsDevfile bool, err error) {
 	downloadSpinner := log.Spinnerf("Downloading starter project %q", starter.Name)
-	err := o.registryClient.DownloadStarterProject(starter, "", dest, false)
+	containsDevfile, err = o.registryClient.DownloadStarterProject(starter, "", dest, false)
 	if err != nil {
 		downloadSpinner.End(false)
-		return err
+		return containsDevfile, err
 	}
 	downloadSpinner.End(true)
-	return nil
+	return containsDevfile, nil
 }
 
 // PersonalizeName calls PersonalizeName methods of the adequate backend

pkg/init/init_test.go

@@ -349,7 +349,7 @@ func TestInitClient_downloadStarterProject(t *testing.T) {
 			o := &InitClient{
 				registryClient: tt.fields.registryClient(ctrl),
 			}
-			if err := o.DownloadStarterProject(&tt.args.project, "dest"); (err != nil) != tt.wantErr {
+			if _, err := o.DownloadStarterProject(&tt.args.project, "dest"); (err != nil) != tt.wantErr {
 				t.Errorf("InitClient.downloadStarterProject() error = %v, wantErr %v", err, tt.wantErr)
 			}
 		})

pkg/init/interface.go

@@ -49,7 +49,7 @@ type Client interface {
 
 	// DownloadStarterProject downloads the starter project referenced in devfile and stores it in dest directory
 	// WARNING: This will first remove all the content of dest.
-	DownloadStarterProject(project *v1alpha2.StarterProject, dest string) error
+	DownloadStarterProject(project *v1alpha2.StarterProject, dest string) (bool, error)
 
 	// PersonalizeName returns the customized Devfile Metadata Name.
 	// Depending on the flags, it may return a name set interactively or not.

pkg/init/mock.go

@@ -54,11 +54,12 @@ func (mr *MockClientMockRecorder) DownloadDevfile(ctx, devfileLocation, destDir
 }
 
 // DownloadStarterProject mocks base method.
-func (m *MockClient) DownloadStarterProject(project *v1alpha2.StarterProject, dest string) error {
+func (m *MockClient) DownloadStarterProject(project *v1alpha2.StarterProject, dest string) (bool, error) {
 	m.ctrl.T.Helper()
 	ret := m.ctrl.Call(m, "DownloadStarterProject", project, dest)
-	ret0, _ := ret[0].(error)
+	ret0, _ := ret[0].(bool)
-	return ret0
+	ret1, _ := ret[1].(error)
+	return ret0, ret1
 }
 
 // DownloadStarterProject indicates an expected call of DownloadStarterProject.

pkg/odo/cli/init/init.go

@@ -226,15 +226,16 @@ func (o *InitOptions) run(ctx context.Context) (devfileObj parser.DevfileObj, pa
 	}
 
 	if starterInfo != nil {
+		var containsDevfile bool
 		// WARNING: this will remove all the content of the destination directory, ie the devfile.yaml file
-		err = o.clientset.InitClient.DownloadStarterProject(starterInfo, workingDir)
+		containsDevfile, err = o.clientset.InitClient.DownloadStarterProject(starterInfo, workingDir)
 		if err != nil {
 			return parser.DevfileObj{}, "", "", nil, nil, fmt.Errorf("unable to download starter project %q: %w", starterInfo.Name, err)
 		}
 		starterDownloaded = true
 
 		// in case the starter project contains a devfile, read it again
-		if _, err = o.clientset.FS.Stat(devfilePath); err == nil {
+		if containsDevfile {
 			devfileObj, _, err = devfile.ParseDevfileAndValidate(parser.ParserArgs{
 				Path:               devfilePath,
 				FlattenedDevfile:   pointer.Bool(false),

pkg/registry/interface.go

@@ -13,7 +13,7 @@ import (
 type Client interface {
 	PullStackFromRegistry(registry string, stack string, destDir string, options library.RegistryOptions) error
 	DownloadFileInMemory(params dfutil.HTTPRequestParams) ([]byte, error)
-	DownloadStarterProject(starterProject *devfilev1.StarterProject, decryptedToken string, contextDir string, verbose bool) error
+	DownloadStarterProject(starterProject *devfilev1.StarterProject, decryptedToken string, contextDir string, verbose bool) (bool, error)
 	GetDevfileRegistries(registryName string) ([]api.Registry, error)
 	ListDevfileStacks(ctx context.Context, registryName, devfileFlag, filterFlag string, detailsFlag bool, withDevfileContent bool) (DevfileStackList, error)
 }

pkg/registry/mock.go

@@ -54,11 +54,12 @@ func (mr *MockClientMockRecorder) DownloadFileInMemory(params interface{}) *gomo
 }
 
 // DownloadStarterProject mocks base method.
-func (m *MockClient) DownloadStarterProject(starterProject *v1alpha2.StarterProject, decryptedToken, contextDir string, verbose bool) error {
+func (m *MockClient) DownloadStarterProject(starterProject *v1alpha2.StarterProject, decryptedToken, contextDir string, verbose bool) (bool, error) {
 	m.ctrl.T.Helper()
 	ret := m.ctrl.Call(m, "DownloadStarterProject", starterProject, decryptedToken, contextDir, verbose)
-	ret0, _ := ret[0].(error)
+	ret0, _ := ret[0].(bool)
-	return ret0
+	ret1, _ := ret[1].(error)
+	return ret0, ret1
 }
 
 // DownloadStarterProject indicates an expected call of DownloadStarterProject.

pkg/registry/registry.go

@@ -64,11 +64,11 @@ func (o RegistryClient) DownloadFileInMemory(params dfutil.HTTPRequestParams) ([
 // Case 1: If there is devfile in the starterproject, replace all the contents of contextDir with that of the starterproject; warn about this
 // Case 2: If there is no devfile, and there is no conflict between the contents of contextDir and starterproject, then copy the contents of the starterproject into contextDir.
 // Case 3: If there is no devfile, and there is conflict between the contents of contextDir and starterproject, copy contents of starterproject into a dir named CONFLICT_STARTER_PROJECT; warn about this
-func (o RegistryClient) DownloadStarterProject(starterProject *devfilev1.StarterProject, decryptedToken string, contextDir string, verbose bool) error {
+func (o RegistryClient) DownloadStarterProject(starterProject *devfilev1.StarterProject, decryptedToken string, contextDir string, verbose bool) (containsDevfile bool, err error) {
 	// Let the project be downloaded in a temp directory
 	starterProjectTmpDir, err := o.fsys.TempDir("", "odostarterproject")
 	if err != nil {
-		return err
+		return containsDevfile, err
 	}
 	defer func() {
 		err = o.fsys.RemoveAll(starterProjectTmpDir)
@@ -78,22 +78,21 @@ func (o RegistryClient) DownloadStarterProject(starterProject *devfilev1.Starter
 	}()
 	err = DownloadStarterProject(o.fsys, starterProject, decryptedToken, starterProjectTmpDir, verbose)
 	if err != nil {
-		return err
+		return containsDevfile, err
 	}
 
 	// Case 1: If there is devfile in the starterproject, replace all the contents of contextDir with that of the starterproject; warn about this
-	var containsDevfile bool
 	if containsDevfile, err = location.DirectoryContainsDevfile(o.fsys, starterProjectTmpDir); err != nil {
-		return err
+		return containsDevfile, err
 	}
 	if containsDevfile {
 		fmt.Println()
 		log.Warning("A Devfile is present inside the starter project; replacing the entire content of the current directory with the starter project")
 		err = removeDirectoryContents(contextDir, o.fsys)
 		if err != nil {
-			return fmt.Errorf("failed to delete contents of the current directory; cause %w", err)
+			return containsDevfile, fmt.Errorf("failed to delete contents of the current directory; cause %w", err)
 		}
-		return util.CopyDirWithFS(starterProjectTmpDir, contextDir, o.fsys)
+		return containsDevfile, util.CopyDirWithFS(starterProjectTmpDir, contextDir, o.fsys)
 	}
 
 	// Case 2: If there is no devfile, and there is no conflict between the contents of contextDir and starterproject, then copy the contents of the starterproject into contextDir.
@@ -101,29 +100,29 @@ func (o RegistryClient) DownloadStarterProject(starterProject *devfilev1.Starter
 	var conflictingFiles []string
 	conflictingFiles, err = getConflictingFiles(starterProjectTmpDir, contextDir, o.fsys)
 	if err != nil {
-		return err
+		return containsDevfile, err
 	}
 
 	// Case 2
 	if len(conflictingFiles) == 0 {
-		return util.CopyDirWithFS(starterProjectTmpDir, contextDir, o.fsys)
+		return containsDevfile, util.CopyDirWithFS(starterProjectTmpDir, contextDir, o.fsys)
 	}
 
 	// Case 3
 	conflictingDirPath := filepath.Join(contextDir, CONFLICT_DIR_NAME)
 	err = o.fsys.MkdirAll(conflictingDirPath, 0750)
 	if err != nil {
-		return err
+		return containsDevfile, err
 	}
 
 	err = util.CopyDirWithFS(starterProjectTmpDir, conflictingDirPath, o.fsys)
 	if err != nil {
-		return err
+		return containsDevfile, err
 	}
 	fmt.Println()
 	log.Warningf("There are conflicting files (%s) between starter project and the current directory, hence the starter project has been copied to %s", strings.Join(conflictingFiles, ", "), conflictingDirPath)
 
-	return nil
+	return containsDevfile, nil
 }
 
 // removeDirectoryContents attempts to remove dir contents without deleting the directory itself, unlike os.RemoveAll() method

pkg/registry/registry_test.go

@@ -666,11 +666,12 @@ func TestRegistryClient_DownloadStarterProject(t *testing.T) {
 		verbose        bool
 	}
 	tests := []struct {
-		name    string
+		name                string
-		fields  fields
+		fields              fields
-		args    args
+		args                args
-		want    []string
+		want                []string
-		wantErr bool
+		wantContainsDevfile bool
+		wantErr             bool
 	}{
 		// TODO: Add test cases.
 		{
@@ -690,8 +691,9 @@ func TestRegistryClient_DownloadStarterProject(t *testing.T) {
 					},
 				},
 			},
-			want:    []string{"devfile.yaml", "docker", filepath.Join("docker", "Dockerfile"), "README.md", "main.go", "go.mod", "someFile.txt"},
+			want:                []string{"devfile.yaml", "docker", filepath.Join("docker", "Dockerfile"), "README.md", "main.go", "go.mod", "someFile.txt"},
-			wantErr: false,
+			wantErr:             false,
+			wantContainsDevfile: true,
 		},
 		{
 			name: "Starter project has conflicting files",
@@ -793,8 +795,13 @@ func TestRegistryClient_DownloadStarterProject(t *testing.T) {
 				preferenceClient: tt.fields.preferenceClient,
 				kubeClient:       tt.fields.kubeClient,
 			}
-			if err := o.DownloadStarterProject(tt.args.starterProject, tt.args.decryptedToken, tt.args.contextDir, tt.args.verbose); (err != nil) != tt.wantErr {
+			gotContainsDevfile, gotErr := o.DownloadStarterProject(tt.args.starterProject, tt.args.decryptedToken, tt.args.contextDir, tt.args.verbose)
-				t.Errorf("DownloadStarterProject() error = %v, wantErr %v", err, tt.wantErr)
+			if (gotErr != nil) != tt.wantErr {
+				t.Errorf("DownloadStarterProject() error = %v, wantErr %v", gotErr, tt.wantErr)
+				return
+			}
+			if tt.wantContainsDevfile != gotContainsDevfile {
+				t.Errorf("DownloadStarterProject() containsDevfile = %v, want = %v", gotContainsDevfile, tt.wantContainsDevfile)
 				return
 			}
 			var got []string

tests/integration/interactive_init_test.go

@@ -5,6 +5,7 @@ import (
 	"log"
 	"os"
 	"path/filepath"
+	"regexp"
 	"strings"
 
 	"github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2"
@@ -81,6 +82,81 @@ var _ = Describe("odo init interactive command tests", func() {
 				Expect(helper.ListFilesInDir(commonVar.Context)).To(ContainElements("devfile.yaml"))
 			})
 
+			Context("personalizing Devfile configuration", func() {
+				var hasMultipleVersions bool
+				BeforeEach(func() {
+					out := helper.Cmd("odo", "registry", "--devfile", "nodejs", "--devfile-registry", "DefaultDevfileRegistry").ShouldPass().Out()
+					// Version pattern has always been in the form of X.X.X
+					vMatch := regexp.MustCompile(`(\d.\d.\d)`)
+					if matches := vMatch.FindAll([]byte(out), -1); len(matches) > 1 {
+						hasMultipleVersions = true
+					}
+				})
+				It("should allow for personalizing configurations", func() {
+					command := []string{"odo", "init"}
+					output, err := helper.RunInteractive(command, nil, func(ctx helper.InteractiveContext) {
+
+						helper.ExpectString(ctx, "Select language")
+						helper.SendLine(ctx, "Javascript")
+
+						helper.ExpectString(ctx, "Select project type")
+						helper.SendLine(ctx, "")
+
+						if hasMultipleVersions {
+							helper.ExpectString(ctx, "Select version")
+							helper.SendLine(ctx, "")
+						}
+
+						helper.ExpectString(ctx, "Select container for which you want to change configuration?")
+						helper.ExpectString(ctx, "runtime")
+						helper.SendLine(ctx, "runtime")
+
+						helper.ExpectString(ctx, "What configuration do you want change")
+						helper.SendLine(ctx, "Add new port")
+
+						helper.ExpectString(ctx, "Enter port number:")
+						helper.SendLine(ctx, "5000")
+
+						helper.ExpectString(ctx, "What configuration do you want change")
+						helper.SendLine(ctx, "Delete port \"3000\"")
+
+						helper.ExpectString(ctx, "What configuration do you want change")
+						helper.SendLine(ctx, "Add new environment variable")
+
+						helper.ExpectString(ctx, "Enter new environment variable name: ")
+						helper.SendLine(ctx, "DEBUG_PROJECT_PORT")
+
+						helper.ExpectString(ctx, "Enter value for \"DEBUG_PROJECT_PORT\" environment variable:")
+						helper.SendLine(ctx, "5858")
+
+						helper.ExpectString(ctx, "What configuration do you want change")
+						helper.SendLine(ctx, "Delete environment variable \"DEBUG_PORT\"")
+
+						helper.ExpectString(ctx, "What configuration do you want change")
+						helper.SendLine(ctx, "NOTHING - configuration is correct")
+
+						helper.ExpectString(ctx, "Select container for which you want to change configuration?")
+						helper.SendLine(ctx, "")
+
+						helper.ExpectString(ctx, "Which starter project do you want to use")
+						helper.SendLine(ctx, "nodejs-starter")
+
+						helper.ExpectString(ctx, "Enter component name:")
+						helper.SendLine(ctx, "my-nodejs-app")
+
+						helper.ExpectString(ctx, "Your new component 'my-nodejs-app' is ready in the current directory.")
+
+					})
+					Expect(err).To(BeNil())
+					Expect(output).To(ContainSubstring("Your new component 'my-nodejs-app' is ready in the current directory."))
+					Expect(helper.ListFilesInDir(commonVar.Context)).To(ContainElements("devfile.yaml"))
+					helper.FileShouldContainSubstring(filepath.Join(commonVar.Context, "devfile.yaml"), "5000")
+					helper.FileShouldNotContainSubstring(filepath.Join(commonVar.Context, "devfile.yaml"), "3000")
+					helper.FileShouldContainSubstring(filepath.Join(commonVar.Context, "devfile.yaml"), "DEBUG_PROJECT_PORT")
+					helper.FileShouldNotContainSubstring(filepath.Join(commonVar.Context, "devfile.yaml"), "DEBUG_PORT")
+				})
+			})
+
 			It("should ask to re-enter the component name when an invalid value is passed", func() {
 				command := []string{"odo", "init"}
 				_, err := helper.RunInteractive(command, nil, func(ctx helper.InteractiveContext) {
@@ -155,6 +231,7 @@ var _ = Describe("odo init interactive command tests", func() {
 				Expect(output).To(ContainSubstring("odo init --name %s --devfile %s --devfile-registry DefaultDevfileRegistry --devfile-version %s --starter %s", componentName, devfileName, devfileVersion, starter))
 				Expect(output).To(ContainSubstring("Your new component 'my-go-app' is ready in the current directory"))
 				Expect(helper.ListFilesInDir(commonVar.Context)).To(ContainElements("devfile.yaml"))
+
 			})
 
 			It("should download correct devfile", func() {