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
feat(sos-download): Add flag that allows overwriting the destination #251
Conversation
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.
Thank you for your contribution @wdullaer, LGTM with the exception of the flag name (see inline suggestion).
cmd/sos_download.go
Outdated
func init() { | ||
sosCmd.AddCommand(downloadCmd) | ||
downloadCmd.Flags().Bool("allow-overwrite", false, "Allow overwriting the destination") |
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.
I'd prefer we go with the usual parlance in CLI tools:
downloadCmd.Flags().Bool("allow-overwrite", false, "Allow overwriting the destination") | |
downloadCmd.Flags().BoolP("force", "f", false, "Overwrite the destination file if it already exists") |
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.
Will do. I'll refactor the variable names to force
as well.
return err | ||
} else if exists { | ||
return fmt.Errorf("file %q: already exists", localFilePath) | ||
} | ||
} |
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.
An alternative implementation
allowOverwrite, err := cmd.Flags().GetBool("allow-overwrite")
if err != nil {
return err
}
if !allowOverwrite {
exists, err := destinationExists(localFilePath, objectName)
if err != nil {
return err
}
if exists {
return fmt.Errorf("file %q: already exists", localFilePath)
}
}
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.
LGTM, thanks for your contribution, I just left a little suggestion ;)
b9b6376
to
3a502a6
Compare
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.
Thanks @wdullaer!
Adds a flag to
exo sos download
which will skip the file existence check.It's off by default.
There are scenarios where this is valid behaviour.
Slightly related: I think that the
os.Rename
step at https://github.com/exoscale/cli/compare/sos-download-overwrite?expand=1#diff-cbe63d0320814a34674804f20dcfac94R140 does not take into account the case wherelocalFilePath
is a directory, and thus needs to be appended withobjectName
.