Fix: Starterproject not replace existing devfile stack files of the context dir (#6633)

* Starterproject does not replace existing files of the context dir Signed-off-by: Parthvi Vala <pvala@redhat.com> * Fix issue with os.RemoveAll Signed-off-by: Parthvi Vala <pvala@redhat.com> * Fix errors, it works now Signed-off-by: Parthvi Vala <pvala@redhat.com> * Fix test failures Signed-off-by: Parthvi Vala <pvala@redhat.com> * Use filesystem to perform file operations wherever possible Signed-off-by: Parthvi Vala <pvala@redhat.com> * Use filesystem to perform file operations wherever possible (2) Signed-off-by: Parthvi Vala <pvala@redhat.com> * Add unit test for DownloadStarterProject 1/n Signed-off-by: Parthvi Vala <pvala@redhat.com> * Fix zip files Signed-off-by: Parthvi Vala <pvala@redhat.com> * Fix zip files (2) Signed-off-by: Parthvi Vala <pvala@redhat.com> * Add unit test starter project with Devfile Signed-off-by: Parthvi Vala <pvala@redhat.com> * Fix zip files (3) Signed-off-by: Parthvi Vala <pvala@redhat.com> * Finish unit tests Signed-off-by: Parthvi Vala <pvala@redhat.com> * Remove unnecessary integration test Signed-off-by: Parthvi Vala <pvala@redhat.com> * Add comments and verbosity, cleanup code Signed-off-by: Parthvi Vala <pvala@redhat.com> * Update pkg/registry/registry.go Co-authored-by: Armel Soro <armel@rm3l.org> * Update pkg/util/util.go Co-authored-by: Armel Soro <armel@rm3l.org> * Update pkg/registry/registry.go Co-authored-by: Armel Soro <armel@rm3l.org> * rm3l's suggestion on using err when not shadowed and enhance error messages Signed-off-by: Parthvi Vala <pvala@redhat.com> Fix validation errors Signed-off-by: Parthvi Vala <pvala@redhat.com> * Change arg position in IsValidProjectDir Signed-off-by: Parthvi Vala <pvala@redhat.com> * rename isConflicting to getConflictingFiles Signed-off-by: Parthvi Vala <pvala@redhat.com> * Add zip files for starterproject with empty dir Signed-off-by: Parthvi Vala <pvala@redhat.com> * Add unit tests covering starterprojects with empty dir for both conflicting and non-conflicting cases Signed-off-by: Parthvi Vala <pvala@redhat.com> * Handle error while walking Signed-off-by: Parthvi Vala <pvala@redhat.com> * Delete temp dirs Signed-off-by: Parthvi Vala <pvala@redhat.com> --------- Signed-off-by: Parthvi Vala <pvala@redhat.com> Co-authored-by: Armel Soro <armel@rm3l.org>
2025-10-19 03:06:19 +03:00 · 2023-03-21 18:25:42 +05:30
parent f543afac9d
commit 47234b94b3
11 changed files with 372 additions and 45 deletions
--- a/pkg/registry/registry.go
+++ b/pkg/registry/registry.go
@@ -2,8 +2,10 @@ package registry

 import (
 	"context"
-	"os"
+	"fmt"
+	"io/fs"
 	"path"
+	"path/filepath"
 	"sort"
 	"strings"
 	"sync"
@@ -34,6 +36,10 @@ type RegistryClient struct {

 var _ Client = (*RegistryClient)(nil)

+const (
+	CONFLICT_DIR_NAME = "CONFLICT_STARTER_PROJECT"
+)
+
 func NewRegistryClient(fsys filesystem.Filesystem, preferenceClient preference.Client, kubeClient kclient.ClientInterface) RegistryClient {
 	return RegistryClient{
 		fsys:             fsys,
@@ -54,9 +60,135 @@ func (o RegistryClient) DownloadFileInMemory(params dfutil.HTTPRequestParams) ([
 }

 // DownloadStarterProject downloads a starter project referenced in devfile
-// This will first remove the content of the contextDir
+// There are 3 cases to consider here:
+// 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 {
-	return DownloadStarterProject(starterProject, decryptedToken, contextDir, verbose)
+	// Let the project be downloaded in a temp directory
+	starterProjectTmpDir, err := o.fsys.TempDir("", "odostarterproject")
+	if err != nil {
+		return err
+	}
+	defer func() {
+		err = o.fsys.RemoveAll(starterProjectTmpDir)
+		if err != nil {
+			klog.V(2).Infof("failed to delete temporary starter project dir %s; cause: %s", starterProjectTmpDir, err.Error())
+		}
+	}()
+	err = DownloadStarterProject(o.fsys, starterProject, decryptedToken, starterProjectTmpDir, verbose)
+	if err != nil {
+		return 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
+	}
+	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 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.
+	// 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
+	var conflictingFiles []string
+	conflictingFiles, err = getConflictingFiles(starterProjectTmpDir, contextDir, o.fsys)
+	if err != nil {
+		return err
+	}
+
+	// Case 2
+	if len(conflictingFiles) == 0 {
+		return 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
+	}
+
+	err = util.CopyDirWithFS(starterProjectTmpDir, conflictingDirPath, o.fsys)
+	if err != nil {
+		return 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
+}
+
+// removeDirectoryContents attempts to remove dir contents without deleting the directory itself, unlike os.RemoveAll() method
+func removeDirectoryContents(path string, fsys filesystem.Filesystem) error {
+	dir, err := fsys.ReadDir(path)
+	if err != nil {
+		return err
+	}
+	for _, f := range dir {
+		// a bit of cheating by using absolute file name to make sure this works with a fake filesystem, especially a memory map which is used by our unit tests
+		// memorymap's Name() method trims the full path and returns just the file name, which then becomes impossible to find by the RemoveAll method that looks for prefix
+		// See: https://github.com/redhat-developer/odo/blob/d717421494f746a5cb12da135f561d12750935f3/vendor/github.com/spf13/afero/memmap.go#L282
+		absFileName := filepath.Join(path, f.Name())
+		err = fsys.RemoveAll(absFileName)
+		if err != nil {
+			return fmt.Errorf("failed to remove %s; cause: %w", absFileName, err)
+		}
+	}
+
+	return nil
+}
+
+// getConflictingFiles fetches the contents of the two directories in question and compares them to check for conflicting files.
+// it returns a list of conflicting files (if any) along with an error (if any).
+func getConflictingFiles(spDir, contextDir string, fsys filesystem.Filesystem) (conflictingFiles []string, err error) {
+	var (
+		contextDirMap = map[string]struct{}{}
+	)
+	// walk through the contextDir, trim the file path from the file name and append it to a map
+	err = fsys.Walk(contextDir, func(path string, info fs.FileInfo, err error) error {
+		if err != nil {
+			return fmt.Errorf("failed to fetch contents of dir %s; cause: %w", contextDirMap, err)
+		}
+		if info.IsDir() {
+			return nil
+		}
+		path = strings.TrimPrefix(path, contextDir)
+		contextDirMap[path] = struct{}{}
+
+		return nil
+	})
+	if err != nil {
+		return nil, fmt.Errorf("failed to walk %s dir; cause: %w", contextDir, err)
+	}
+
+	// walk through the starterproject dir, trim the file path from file name, and check if it exists in the contextDir map;
+	// if it does, it is a conflicting file, hence append it to the conflictingFiles list.
+	err = fsys.Walk(spDir, func(path string, info fs.FileInfo, err error) error {
+		if err != nil {
+			return fmt.Errorf("failed to fetch contents of dir %s; cause: %w", spDir, err)
+		}
+		if info.IsDir() {
+			return nil
+		}
+		path = strings.TrimPrefix(path, spDir)
+		if _, ok := contextDirMap[path]; ok {
+			conflictingFiles = append(conflictingFiles, path)
+		}
+		return nil
+	})
+	if err != nil {
+		return nil, fmt.Errorf("failed to walk %s dir; cause: %w", spDir, err)
+	}
+
+	return conflictingFiles, nil
 }

 // GetDevfileRegistries gets devfile registries from preference file,
@@ -271,11 +403,16 @@ func createRegistryDevfiles(registry api.Registry, devfileIndex []indexSchema.Sc
 func (o RegistryClient) retrieveDevfileDataFromRegistry(ctx context.Context, registryName string, devfileName string) (api.DevfileData, error) {

 	// Create random temporary file
-	tmpFile, err := os.MkdirTemp("", "odo")
+	tmpFile, err := o.fsys.TempDir("", "odo")
 	if err != nil {
 		return api.DevfileData{}, err
 	}
-	defer os.Remove(tmpFile)
+	defer func() {
+		err = o.fsys.RemoveAll(tmpFile)
+		if err != nil {
+			klog.V(2).Infof("failed to delete temporary starter project dir %s; cause: %s", tmpFile, err.Error())
+		}
+	}()

 	registries, err := o.GetDevfileRegistries(registryName)
 	if err != nil {
--- a/pkg/registry/registry_test.go
+++ b/pkg/registry/registry_test.go
@@ -3,9 +3,16 @@ package registry
 import (
 	"context"
 	"errors"
+	"fmt"
+	devfilev1 "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2"
+	"io/fs"
 	"net/http"
 	"net/http/httptest"
 	"os"
+	"path/filepath"
+	"runtime"
+	"sort"
+	"strings"
 	"testing"

 	"github.com/golang/mock/gomock"
@@ -618,3 +625,195 @@ func TestGetRegistryDevfiles(t *testing.T) {
 		})
 	}
 }
+
+func TestRegistryClient_DownloadStarterProject(t *testing.T) {
+	setupFS := func(contextDir string, fs filesystem.Filesystem) {
+		directoriesToBeCreated := []string{"kubernetes", "docker"}
+		for _, dirToBeCreated := range directoriesToBeCreated {
+			dirName := filepath.Join(contextDir, dirToBeCreated)
+			err := fs.MkdirAll(dirName, os.FileMode(0750))
+			if err != nil {
+				t.Errorf("failed to create %s; cause: %s", dirName, err)
+			}
+		}
+
+		filesToBeCreated := []string{"devfile.yaml", "kubernetes/deploy.yaml", "docker/Dockerfile"}
+		for _, fileToBeCreated := range filesToBeCreated {
+			fileName := filepath.Join(contextDir, fileToBeCreated)
+			_, err := fs.Create(fileName)
+			if err != nil {
+				t.Errorf("failed to create %s; cause: %s", fileName, err)
+			}
+		}
+	}
+
+	getZipFilePath := func(name string) string {
+		// filename of this file
+		_, filename, _, _ := runtime.Caller(0)
+		// path to the devfile
+		return filepath.Join(filepath.Dir(filename), "..", "..", "tests", "examples", filepath.Join("source", "devfiles", "zips", name))
+	}
+	type fields struct {
+		fsys             filesystem.Filesystem
+		preferenceClient preference.Client
+		kubeClient       kclient.ClientInterface
+	}
+	type args struct {
+		starterProject *devfilev1.StarterProject
+		decryptedToken string
+		contextDir     string
+		verbose        bool
+	}
+	tests := []struct {
+		name    string
+		fields  fields
+		args    args
+		want    []string
+		wantErr bool
+	}{
+		// TODO: Add test cases.
+		{
+			name: "Starter project has a Devfile",
+			fields: fields{
+				fsys: filesystem.NewFakeFs(),
+			},
+			args: args{
+				starterProject: &devfilev1.StarterProject{
+					Name: "starter-project-with-devfile",
+					ProjectSource: devfilev1.ProjectSource{
+						SourceType: "",
+						Zip: &devfilev1.ZipProjectSource{
+							CommonProjectSource: devfilev1.CommonProjectSource{},
+							Location:            fmt.Sprintf("file://%s", getZipFilePath("starterproject-with-devfile.zip")),
+						},
+					},
+				},
+			},
+			want:    []string{"devfile.yaml", "docker", filepath.Join("docker", "Dockerfile"), "README.md", "main.go", "go.mod", "someFile.txt"},
+			wantErr: false,
+		},
+		{
+			name: "Starter project has conflicting files",
+			fields: fields{
+				fsys: filesystem.NewFakeFs(),
+			},
+			args: args{
+				starterProject: &devfilev1.StarterProject{
+					Name: "starter-project-with-conflicting-files",
+					ProjectSource: devfilev1.ProjectSource{
+						SourceType: "",
+						Zip: &devfilev1.ZipProjectSource{
+							CommonProjectSource: devfilev1.CommonProjectSource{},
+							Location:            fmt.Sprintf("file://%s", getZipFilePath("starterproject-with-conflicts.zip")),
+						},
+					},
+				},
+			},
+			want: []string{"devfile.yaml", "docker", filepath.Join("docker", "Dockerfile"), "kubernetes", filepath.Join("kubernetes", "deploy.yaml"),
+				CONFLICT_DIR_NAME, filepath.Join(CONFLICT_DIR_NAME, "kubernetes"), filepath.Join(CONFLICT_DIR_NAME, "kubernetes", "deploy.yaml"),
+				filepath.Join(CONFLICT_DIR_NAME, "main.go"), filepath.Join(CONFLICT_DIR_NAME, "go.mod"), filepath.Join(CONFLICT_DIR_NAME, "README.md"), filepath.Join(CONFLICT_DIR_NAME, "someFile.txt")},
+			wantErr: false,
+		},
+		{
+			name: "Starter project has conflicting files and an empty dir",
+			fields: fields{
+				fsys: filesystem.NewFakeFs(),
+			},
+			args: args{
+				starterProject: &devfilev1.StarterProject{
+					Name: "starter-project-with-conflicting-files-and-empty-dir",
+					ProjectSource: devfilev1.ProjectSource{
+						SourceType: "",
+						Zip: &devfilev1.ZipProjectSource{
+							CommonProjectSource: devfilev1.CommonProjectSource{},
+							Location:            fmt.Sprintf("file://%s", getZipFilePath("starterproject-with-conflicts-and-empty-dir.zip")),
+						},
+					},
+				},
+			},
+			want: []string{"devfile.yaml", "docker", filepath.Join("docker", "Dockerfile"), "kubernetes", filepath.Join("kubernetes", "deploy.yaml"),
+				filepath.Join(CONFLICT_DIR_NAME, "kubernetes"), CONFLICT_DIR_NAME, filepath.Join(CONFLICT_DIR_NAME, "docker"), filepath.Join(CONFLICT_DIR_NAME, "docker", "Dockerfile"),
+				filepath.Join(CONFLICT_DIR_NAME, "main.go"), filepath.Join(CONFLICT_DIR_NAME, "go.mod"), filepath.Join(CONFLICT_DIR_NAME, "README.md")},
+			wantErr: false,
+		},
+		{
+			name: "Starter project does not have any conflicting files",
+			fields: fields{
+				fsys: filesystem.NewFakeFs(),
+			},
+			args: args{
+				starterProject: &devfilev1.StarterProject{
+					Name: "starter-project-with-no-conflicting-files",
+					ProjectSource: devfilev1.ProjectSource{
+						SourceType: "",
+						Zip: &devfilev1.ZipProjectSource{
+							CommonProjectSource: devfilev1.CommonProjectSource{},
+							Location:            fmt.Sprintf("file://%s", getZipFilePath("starterproject-with-no-conflicts.zip")),
+						},
+					},
+				},
+			},
+			want:    []string{"devfile.yaml", "docker", filepath.Join("docker", "Dockerfile"), "kubernetes", filepath.Join("kubernetes", "deploy.yaml"), "README.md", "main.go", "go.mod"},
+			wantErr: false,
+		},
+		{
+			name: "Starter project does not have any conflicting files but has empty dir",
+			fields: fields{
+				fsys: filesystem.NewFakeFs(),
+			},
+			args: args{
+				starterProject: &devfilev1.StarterProject{
+					Name: "starter-project-with-no-conflicting-files-and-empty-dir",
+					ProjectSource: devfilev1.ProjectSource{
+						SourceType: "",
+						Zip: &devfilev1.ZipProjectSource{
+							CommonProjectSource: devfilev1.CommonProjectSource{},
+							Location:            fmt.Sprintf("file://%s", getZipFilePath("starterproject-with-no-conflicts-and-empty-dir.zip")),
+						},
+					},
+				},
+			},
+			want:    []string{"devfile.yaml", "docker", filepath.Join("docker", "Dockerfile"), "kubernetes", filepath.Join("kubernetes", "deploy.yaml"), "README.md", "main.go", "go.mod"},
+			wantErr: false,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			contextDir, ferr := tt.fields.fsys.TempDir("", "downloadstarterproject")
+			if ferr != nil {
+				t.Errorf("failed to create temp dir; cause: %s", ferr)
+			}
+			tt.args.contextDir = contextDir
+
+			setupFS(tt.args.contextDir, tt.fields.fsys)
+
+			o := RegistryClient{
+				fsys:             tt.fields.fsys,
+				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)
+				return
+			}
+			var got []string
+			err := o.fsys.Walk(contextDir, func(path string, info fs.FileInfo, err error) error {
+				path = strings.TrimPrefix(path, contextDir)
+				path = strings.TrimPrefix(path, string(os.PathSeparator))
+				if path != "" {
+					got = append(got, path)
+				}
+				return nil
+			})
+			if err != nil {
+				t.Errorf("failed to walk %s; cause:%s", contextDir, err.Error())
+				return
+			}
+			sort.Strings(got)
+			sort.Strings(tt.want)
+			if diff := cmp.Diff(got, tt.want); diff != "" {
+				t.Errorf("DownloadStarterProject() mismatch (-want +got):\n%s", diff)
+			}
+		})
+	}
+}
--- a/pkg/registry/starter_project.go
+++ b/pkg/registry/starter_project.go
@@ -3,6 +3,7 @@ package registry
 import (
 	"errors"
 	"fmt"
+	"github.com/redhat-developer/odo/pkg/testingutil/filesystem"
 	"os"
 	"path/filepath"

@@ -21,12 +22,12 @@ const (
 	RegistryUser = "default"
 )

-func checkoutProject(subDir, zipURL, path, starterToken string) error {
+func checkoutProject(subDir, zipURL, path, starterToken string, fsys filesystem.Filesystem) error {

 	if subDir == "" {
 		subDir = "/"
 	}
-	err := util.GetAndExtractZip(zipURL, path, subDir, starterToken)
+	err := util.GetAndExtractZip(zipURL, path, subDir, starterToken, fsys)
 	if err != nil {
 		return fmt.Errorf("failed to download and extract project zip folder: %w", err)
 	}
@@ -35,7 +36,7 @@ func checkoutProject(subDir, zipURL, path, starterToken string) error {

 // DownloadStarterProject downloads a starter project referenced in devfile
 // This will first remove the content of the contextDir
-func DownloadStarterProject(starterProject *devfilev1.StarterProject, decryptedToken string, contextDir string, verbose bool) error {
+func DownloadStarterProject(fs filesystem.Filesystem, starterProject *devfilev1.StarterProject, decryptedToken string, contextDir string, verbose bool) error {
 	var path string
 	var err error
 	// Retrieve the working directory in order to clone correctly
@@ -49,7 +50,7 @@ func DownloadStarterProject(starterProject *devfilev1.StarterProject, decryptedT
 	}

 	// We will check to see if the project has a valid directory
-	err = util.IsValidProjectDir(path, location.DevfileLocation(""))
+	err = util.IsValidProjectDir(path, location.DevfileLocation(""), fs)
 	if err != nil {
 		return err
 	}
@@ -72,7 +73,7 @@ func DownloadStarterProject(starterProject *devfilev1.StarterProject, decryptedT
 		if verbose {
 			downloadSpinner = log.Spinnerf("Downloading starter project %s from %s", starterProject.Name, url)
 		}
-		err := checkoutProject(sparseDir, url, path, decryptedToken)
+		err := checkoutProject(sparseDir, url, path, decryptedToken, fs)
 		if err != nil {
 			if verbose {
 				downloadSpinner.End(false)