CLI: Download Detector Configuration as File #229
Conversation
Codecov Report
@@ Coverage Diff @@
## master #229 +/- ##
============================================
- Coverage 72.40% 72.25% -0.15%
+ Complexity 1289 1278 -11
============================================
Files 139 139
Lines 6073 6045 -28
Branches 469 469
============================================
- Hits 4397 4368 -29
- Misses 1464 1465 +1
Partials 212 212
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Go provides an abstraction to support writing on file and stdout as same interface, refactor cat to accept func as input. Later, download will pass writeInfile as parameter
VijayanB
requested review from
ohltyler,
sean-zheng-amazon and
yizheliu-amazon
VijayanB
changed the title
cli/cmd/cat.go
Outdated
| if err != nil { | ||
| fmt.Println(err) | ||
| } |
Printing error on console is added everywhere. Create a function to check error and print.
Download is similar to cat, excepts it download the configuration and save it as file with detector name as file name on current folder
cli/cmd/download.go
Outdated
| if err != nil { | ||
| return err | ||
| } | ||
| filePath := filepath.Join(cwd, d.Name) |
| return err | ||
| } | ||
| filePath := filepath.Join(cwd, d.Name) | ||
| f, err := os.Create(filePath) |
There was a problem hiding this comment.
If the filePath exists, will os.Create override the existing file?
There was a problem hiding this comment.
From this doc, https://golang.org/pkg/os/#Create
If the file already exists, it is truncated
How about check file exists or not before create ? If file exists, we should not truncate it directly in case user execute command by mistake.
There was a problem hiding this comment.
I introduced a flag -i similar to cp in linux commands, which prompt to stdout whether to overwrite the file or not.
There was a problem hiding this comment.
The flag -i is optional? What if user don't add -i?
There was a problem hiding this comment.
It is interactive. By default download will overwrite but if user passes -i, it will prompt for user confirmation. Do you think prompt should be default?
There was a problem hiding this comment.
I think prompt should be default for this case, not good user experience/safe to override user's existing file without any prompt.
cli/cmd/download.go
Outdated
| //file will be created inside current working directory, | ||
| //with detector name as file name | ||
| func WriteInFile(d *entity.DetectorOutput) error { | ||
| cwd, err := os.Getwd() |
There was a problem hiding this comment.
Is it possible to output detector into a different directory other than current directory? Can user specify the absolute file path? For example, user want to download detector and output it into "/tmp/my_detectors.json".
If user query detector with name pattern and return multiple detectors, we will write detectors into separate files. Seems not very easy for user to share if they have hundreds of such small detector configuration files. By specifying an absolute file path and output all detectors' configuration into one file will be easier for user to share, bulk recreate etc.
There was a problem hiding this comment.
Yes definitely possible. I am planning to include this feature as next PR.
There was a problem hiding this comment.
Added allow user to provide output folder as new commit.
There was a problem hiding this comment.
Cool, thanks for the change
Similar to cp from linux, add interactive to allow user to decide whether to overwrite file or not
Added flag to allow user to specify target location where detectors will be downloaded.
Download detector will always prompt if there is file exists in output directory.
Download configuration as files helps users to save it in source code control and maintains it.
This will also let users to share detector configuration across users.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.