Skip to content
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.

Sign up for GitHub

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

fix: fail when path is not a directory for peercontainer upload/download #1028

Merged
merged 1 commit into from
May 29, 2023
Merged

fix: fail when path is not a directory for peercontainer upload/download #1028

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions environment/peercontainer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,13 @@ func (e *PeerContainer) Destroy() error {

// Download downloads the path to outside the environment
func (e *PeerContainer) Download(path string) (string, error) {
fileInfo, err := e.Stat(path)
if err != nil {
return path, fmt.Errorf("failed to stat the given path : %s. Error: %v", path, err)
}
if !fileInfo.IsDir() {
return path, fmt.Errorf("download only supports directory paths. The path provided is %s. Error: %v", path, err)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason to support only directories? Or is our current implementation support only directories?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ashokponkumar From what I understand, I think it is for directories as of now. If you see the containerized transformer code, we actually create a tmp folder to upload the input json.

Having the upload and download path to include files would be a good flexibility to have.

}
output, err := os.MkdirTemp(e.TempPath, "*")
if err != nil {
return path, fmt.Errorf("failed to create temp dir. Error: %w", err)
Expand All @@ -186,6 +193,13 @@ func (e *PeerContainer) Download(path string) (string, error) {
// Upload uploads the path from outside the environment into it
func (e *PeerContainer) Upload(outpath string) (string, error) {
envpath := "/var/tmp/" + uniuri.NewLen(5) + "/" + filepath.Base(outpath)
fileInfo, err := os.Stat(outpath)
if err != nil {
return envpath, fmt.Errorf("failed to stat the given path : %s. Error: %v", outpath, err)
}
if !fileInfo.IsDir() {
return envpath, fmt.Errorf("upload only supports directory paths. The path provided is %s. Error: %v", outpath, err)
Comment on lines +200 to +201
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ashokponkumar would it be a good idea to support files as well as directories? Right now, this PR just raises an error if files are passed to upload.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#1039

}
cengine, err := container.GetContainerEngine(false)
if err != nil {
return envpath, fmt.Errorf("failed to get the container engine. Error: %w", err)
Expand Down