-
Notifications
You must be signed in to change notification settings - Fork 243
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adding sparseCheckoutDir functionality to devfiles #3042
Changes from 18 commits
63bd13a
85daabe
2462f56
acf05b0
4e39e36
9a6de10
67681f5
cfbdd2a
b2c8fc8
26928c7
4968786
dca7532
330807c
ef285fb
bf0f368
4645969
8aa566b
2c416b8
0824130
e109230
f0b93b0
080e030
14c6c44
24e341b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -801,8 +801,9 @@ func GetGitHubZipURL(repoURL string) (string, error) { | |
} | ||
|
||
// GetAndExtractZip downloads a zip file from a URL with a http prefix or | ||
// takes an absolute path prefixed with file:// and extracts it to a destination | ||
func GetAndExtractZip(zipURL string, destination string) error { | ||
// takes an absolute path prefixed with file:// and extracts it to a destination. | ||
// pathToUnzip specifies the path within the zip folder to extract | ||
func GetAndExtractZip(zipURL string, destination string, pathToUnzip string) error { | ||
if zipURL == "" { | ||
return errors.Errorf("Empty zip url: %s", zipURL) | ||
} | ||
|
@@ -836,18 +837,23 @@ func GetAndExtractZip(zipURL string, destination string) error { | |
return errors.Errorf("Invalid Zip URL: %s . Should either be prefixed with file://, http:// or https://", zipURL) | ||
} | ||
|
||
_, err := Unzip(pathToZip, destination) | ||
filenames, err := Unzip(pathToZip, destination, pathToUnzip) | ||
if err != nil { | ||
return err | ||
} | ||
|
||
if len(filenames) == 0 { | ||
return errors.New("No files were unzipped, ensure that the project repo is not empty or that sparseCheckoutDir has a valid path") | ||
CameronMcWilliam marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
return nil | ||
} | ||
|
||
// Unzip will decompress a zip archive, moving all files and folders | ||
// within the zip file (parameter 1) to an output directory (parameter 2). | ||
// Unzip will decompress a zip archive, moving specified files and folders | ||
// within the zip file (parameter 1) to an output directory (parameter 2) | ||
// Source: https://golangcode.com/unzip-files-in-go/ | ||
CameronMcWilliam marked this conversation as resolved.
Show resolved
Hide resolved
|
||
func Unzip(src, dest string) ([]string, error) { | ||
// pathToUnzip (parameter 3) is the path within the zip folder to extract | ||
func Unzip(src, dest, pathToUnzip string) ([]string, error) { | ||
var filenames []string | ||
|
||
r, err := zip.OpenReader(src) | ||
|
@@ -856,16 +862,48 @@ func Unzip(src, dest string) ([]string, error) { | |
} | ||
defer r.Close() | ||
|
||
for _, f := range r.File { | ||
// change path separator to correct character for windows | ||
if runtime.GOOS == "windows" { | ||
pathToUnzip = strings.Replace(pathToUnzip, "/", string(os.PathSeparator), -1) | ||
CameronMcWilliam marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
for _, f := range r.File { | ||
// Store filename/path for returning and using later on | ||
index := strings.Index(f.Name, "/") | ||
index := strings.Index(f.Name, string(os.PathSeparator)) | ||
filename := f.Name[index+1:] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you could probably use https://golang.org/pkg/path/filepath/#Base here instead of using getting the index of |
||
if filename == "" { | ||
continue | ||
} | ||
|
||
// if sparseCheckoutDir has a pattern | ||
match, err := filepath.Match(pathToUnzip, filename) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we need to be careful here with pattern matching: Using the quarkus devfile, we're pattern matching all of the folders that are prefixed with
But when doing it manually via git, it only gives me the
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So it seems it was the prefix that was causing this error, I have something in place where we add a path separator to the end of the pathToUnzip if there isn't one present and that seems to fix it in all aspects. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ... But I would be willing to hear your opinion on that change :) |
||
if err != nil { | ||
return filenames, err | ||
} | ||
|
||
// removes first slash of pathToUnzip if present, adds trailing slash | ||
pathToUnzip = strings.TrimPrefix(pathToUnzip, string(os.PathSeparator)) | ||
if pathToUnzip != "" && pathToUnzip[len(pathToUnzip)-1:] != string(os.PathSeparator) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. there has be to be a better way to do this. |
||
pathToUnzip = pathToUnzip + string(os.PathSeparator) | ||
} | ||
// destination filepath before trim | ||
fpath := filepath.Join(dest, filename) | ||
|
||
// used for pattern matching | ||
fpathDir := filepath.Dir(fpath) | ||
|
||
// check for prefix or match | ||
if strings.HasPrefix(filename, pathToUnzip) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See my other note about pattern matching above. |
||
filename = strings.TrimPrefix(filename, pathToUnzip) | ||
} else if !strings.HasPrefix(filename, pathToUnzip) && !match && !sliceContainsString(fpathDir, filenames) { | ||
continue | ||
} | ||
// adds trailing slash to destination if needed as filepath.Join removes it | ||
if (len(filename) == 1 && os.IsPathSeparator(filename[0])) || filename == "" { | ||
fpath = dest + string(os.PathSeparator) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can see problems arising in the future in this section with regards to Windows support. Can you double check that your PR works correctly in Windows? |
||
} else { | ||
fpath = filepath.Join(dest, filename) | ||
} | ||
// Check for ZipSlip. More Info: http://bit.ly/2MsjAWE | ||
if !strings.HasPrefix(fpath, filepath.Clean(dest)+string(os.PathSeparator)) { | ||
return filenames, fmt.Errorf("%s: illegal file path", fpath) | ||
|
@@ -996,3 +1034,13 @@ func ValidateURL(sourceURL string) error { | |
|
||
return nil | ||
} | ||
|
||
// sliceContainsString checks for existence of given string in given slice | ||
func sliceContainsString(str string, slice []string) bool { | ||
CameronMcWilliam marked this conversation as resolved.
Show resolved
Hide resolved
|
||
for _, b := range slice { | ||
if b == str { | ||
return true | ||
} | ||
} | ||
return false | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,47 @@ | ||
apiVersion: 1.0.0 | ||
metadata: | ||
name: test-devfile | ||
projects: | ||
- | ||
name: nodejs-web-app | ||
source: | ||
type: git | ||
location: "https://github.com/che-samples/web-nodejs-sample.git" | ||
sparseCheckoutDir: /app/ | ||
components: | ||
- type: dockerimage | ||
image: quay.io/eclipse/che-nodejs10-ubi:nightly | ||
endpoints: | ||
- name: "3000/tcp" | ||
port: 3000 | ||
alias: runtime | ||
env: | ||
- name: FOO | ||
value: "bar" | ||
memoryLimit: 1024Mi | ||
mountSources: true | ||
commands: | ||
- name: build | ||
actions: | ||
- type: exec | ||
component: runtime | ||
command: "npm install" | ||
workdir: ${CHE_PROJECTS_ROOT}/nodejs-web-app/app | ||
- name: devbuild | ||
actions: | ||
- type: exec | ||
component: runtime | ||
command: "npm install" | ||
workdir: ${CHE_PROJECTS_ROOT}/nodejs-web-app/app | ||
- name: run | ||
actions: | ||
- type: exec | ||
component: runtime | ||
command: "nodemon app.js" | ||
workdir: ${CHE_PROJECTS_ROOT}/nodejs-web-app/app | ||
- name: devrun | ||
actions: | ||
- type: exec | ||
component: runtime | ||
command: "nodemon app.js" | ||
workdir: ${CHE_PROJECTS_ROOT}/nodejs-web-app/app |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -191,4 +191,36 @@ var _ = Describe("odo devfile create command tests", func() { | |
// helper.DeleteDir(contextDevfile) | ||
// }) | ||
//}) | ||
|
||
// Context("When executing odo create with devfile component, --downloadSource flag and sparseContextDir has a valid value", func() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 Thanks for writing the tests even if they can't be enabled just yet. A couple small modifications I'd recommend:
|
||
// It("should only extract the specified path in the sparseContextDir field", func() { | ||
// helper.CmdShouldPass("odo", "preference", "set", "Experimental", "true") | ||
// contextDevfile := helper.CreateNewContext() | ||
// helper.Chdir(contextDevfile) | ||
// devfile := "devfile.yaml" | ||
// helper.CopyExampleDevFile(filepath.Join("source", "devfiles", "nodejs", "devfile-with-sparseCheckoutDir"), filepath.Join(contextDevfile, devfile)) | ||
// componentNamespace := helper.RandString(6) | ||
// helper.CmdShouldPass("odo", "create", "--downloadSource", "--project", componentNamespace) | ||
// expectedFiles := []string{"app.js", devfile} | ||
// Expect(helper.VerifyFilesExist(contextDevfile, expectedFiles)).To(Equal(true)) | ||
// helper.DeleteDir(contextDevfile) | ||
// }) | ||
// }) | ||
|
||
// Context("When executing odo create with devfile component, --downloadSource flag and sparseContextDir has an invalid value", func() { | ||
// It("should fail and alert the user that the specified path in sparseContextDir does not exist", func() { | ||
// helper.CmdShouldPass("odo", "preference", "set", "Experimental", "true") | ||
// contextDevfile := helper.CreateNewContext() | ||
// helper.Chdir(contextDevfile) | ||
// devfile := "devfile.yaml" | ||
// devfilePath := filepath.Join(contextDevfile, devfile) | ||
// helper.CopyExampleDevFile(filepath.Join("source", "devfiles", "nodejs", "devfile-with-sparseCheckoutDir"), devfilePath) | ||
// helper.ReplaceDevfileField(devfilePath, "sparseCheckoutDir", "/invalid/") | ||
// componentNamespace := helper.RandString(6) | ||
// output := helper.CmdShouldFail("odo", "create", "--downloadSource", "--project", componentNamespace) | ||
// expectedString := "No files were unzipped, ensure that the project repo is not empty or that sparseCheckoutDir has a valid path" | ||
// helper.MatchAllInOutput(output, []string{expectedString}) | ||
// helper.DeleteDir(contextDevfile) | ||
// }) | ||
// }) | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this code https://github.com/openshift/odo/pull/3042/files#diff-58fe314b4e3f9120402717ab8c2b3049R769-R783 can be extracted into a function, would make things cleaner