From 0cdc2e55428427e6463d46102fd8bbbecb6aa455 Mon Sep 17 00:00:00 2001 From: Parthvi Vala Date: Fri, 12 May 2023 01:36:49 +0530 Subject: [PATCH] Fix: `odo init` overwrites personalized configuration when downloading starter project (#6800) * Fix personalized configuration overwritten by starter project Signed-off-by: Parthvi Vala * Update pkg/registry/registry_test.go Co-authored-by: Armel Soro Signed-off-by: Parthvi Vala * Adding intergration tests for personalizing configurations with odo init Signed-off-by: Ritu Deshmukh Modified changes Signed-off-by: Ritu Deshmukh Update tests/integration/interactive_init_test.go Co-authored-by: Parthvi Vala Update tests/integration/interactive_init_test.go Co-authored-by: Parthvi Vala 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 Update interactive_init_test.go * Updated changes Signed-off-by: Ritu Deshmukh * Attempt at fixing interactive tests Signed-off-by: Parthvi Vala * Add multiple devfile version check Co-authored-by: Armel Soro Signed-off-by: Parthvi Vala --------- Signed-off-by: Parthvi Vala Signed-off-by: Ritu Deshmukh Co-authored-by: Armel Soro Co-authored-by: Ritu Deshmukh Co-authored-by: Armel Soro --- pkg/init/init.go | 8 +-- pkg/init/init_test.go | 2 +- pkg/init/interface.go | 2 +- pkg/init/mock.go | 7 +- pkg/odo/cli/init/init.go | 5 +- pkg/registry/interface.go | 2 +- pkg/registry/mock.go | 7 +- pkg/registry/registry.go | 23 ++++--- pkg/registry/registry_test.go | 25 ++++--- tests/integration/interactive_init_test.go | 77 ++++++++++++++++++++++ 10 files changed, 122 insertions(+), 36 deletions(-) diff --git a/pkg/init/init.go b/pkg/init/init.go index 3b47ab53e..9bf5dee4f 100644 --- a/pkg/init/init.go +++ b/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 diff --git a/pkg/init/init_test.go b/pkg/init/init_test.go index e6231f697..986ae3ddf 100644 --- a/pkg/init/init_test.go +++ b/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) } }) diff --git a/pkg/init/interface.go b/pkg/init/interface.go index 8e95fbb00..4d4439d55 100644 --- a/pkg/init/interface.go +++ b/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. diff --git a/pkg/init/mock.go b/pkg/init/mock.go index d865ab2a7..2d6977896 100644 --- a/pkg/init/mock.go +++ b/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) - return ret0 + ret0, _ := ret[0].(bool) + ret1, _ := ret[1].(error) + return ret0, ret1 } // DownloadStarterProject indicates an expected call of DownloadStarterProject. diff --git a/pkg/odo/cli/init/init.go b/pkg/odo/cli/init/init.go index 81d43a190..659d43428 100644 --- a/pkg/odo/cli/init/init.go +++ b/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), diff --git a/pkg/registry/interface.go b/pkg/registry/interface.go index 0e3b5344d..f68d96003 100644 --- a/pkg/registry/interface.go +++ b/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) } diff --git a/pkg/registry/mock.go b/pkg/registry/mock.go index 9f99b909d..74a1a1f5c 100644 --- a/pkg/registry/mock.go +++ b/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) - return ret0 + ret0, _ := ret[0].(bool) + ret1, _ := ret[1].(error) + return ret0, ret1 } // DownloadStarterProject indicates an expected call of DownloadStarterProject. diff --git a/pkg/registry/registry.go b/pkg/registry/registry.go index 64c353b28..05e020a07 100644 --- a/pkg/registry/registry.go +++ b/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 diff --git a/pkg/registry/registry_test.go b/pkg/registry/registry_test.go index 453b669bd..a25247390 100644 --- a/pkg/registry/registry_test.go +++ b/pkg/registry/registry_test.go @@ -666,11 +666,12 @@ func TestRegistryClient_DownloadStarterProject(t *testing.T) { verbose bool } tests := []struct { - name string - fields fields - args args - want []string - wantErr bool + name string + fields fields + args args + want []string + 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"}, - wantErr: false, + want: []string{"devfile.yaml", "docker", filepath.Join("docker", "Dockerfile"), "README.md", "main.go", "go.mod", "someFile.txt"}, + 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 { - t.Errorf("DownloadStarterProject() error = %v, wantErr %v", err, tt.wantErr) + gotContainsDevfile, gotErr := o.DownloadStarterProject(tt.args.starterProject, tt.args.decryptedToken, tt.args.contextDir, tt.args.verbose) + 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 diff --git a/tests/integration/interactive_init_test.go b/tests/integration/interactive_init_test.go index d0e3b58fb..e63af0715 100644 --- a/tests/integration/interactive_init_test.go +++ b/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() {