Feature/cobra cli #79
Conversation
|
This introduces as |
|
This last commit should address the double cmd folder structure by renaming the inner cmd folder to "commands" (as it contains command implementations of the CLI tool). If need be, the outer cmd folder could be renamed to cli as well, but I think the former is the "standard" in Go. |
consolethinks
force-pushed
the
feature/cobra-cli
branch
from
09e475a
to
e0584ff
Compare
consolethinks
force-pushed
the
feature/cobra-cli
branch
from
e0584ff
to
e9a4504
Compare
| Long: `This library comprises a few subcommands for managing SciCat | ||
| and datasets on it, as well as interacting with the archival system connected | ||
| to it.`, | ||
| // uncomment the next line if there's a default action |
There was a problem hiding this comment.
maybe remove the comment?
There was a problem hiding this comment.
I might add a default action since the version parameter is common between all commands
cmd/commands/datasetArchiver.go
Outdated
|
|
||
| flag.Parse() | ||
|
|
||
| if datasetUtils.TestFlags != nil { |
There was a problem hiding this comment.
was this block removed by mistake?
There was a problem hiding this comment.
having this will allow to check that passing variables by reference did not introduce any error
There was a problem hiding this comment.
it was deleted because the code was original only copied from before I added tests, this is readded in the latest commit.
|
|
||
| import cmd "github.com/paulscherrerinstitute/scicat/cmd/commands" | ||
|
|
||
| func main() { |
There was a problem hiding this comment.
could we test here that when we call this main with an arg (e.g. datasetIngestor) the datasetIngestor from commands is executed?
There was a problem hiding this comment.
I have done something similar to that in the latest commit (calling Execute() instead of main, but it does effectively the same thing)
| // from the array of strings | ||
| commonPrefix := func(arr []string) string { | ||
| // return shortest string, length given in bytes | ||
| findMinLength := func(arr []string) int { |
There was a problem hiding this comment.
why do these need to become internal functions? is this because the command is now a pointer? I am not a fun of internal functions, as they will be very hard to test later
There was a problem hiding this comment.
they are internal because the functions are only used once and only exist to make the code more readable. If they weren't internal, the functions' definitions would be shared among all the commands. This wasn't an issue when we had lots of little "main" packages, but now there's a bigger "cmd" package that ties everything together.
| Timeout: 10 * time.Second} | ||
|
|
||
| // internal functions | ||
| assembleRsyncCommands := func(username string, datasetDetails []datasetUtils.Dataset, destinationPath string) ([]string, []string) { |
|
Not to hold up this merge even more, but are we ok with removing the old tools? I had imagined keeping the old executable around for backwards compatibility. After removing them we will have to maintain two branches until we drop support for the old cli names. |
|
I think we can double check the CI and the version comparison and use this eventually. From memory, the release workflow in the CI overrides the "prerelease" and "latest" flags, so we should check that |
Issue
Resolves #75
PR Description
It copies the pre-existing code in each executable's main function, adapts them to Cobra's way of getting flags and parameters.
Each command supported by this one executable should behave the same way, for a given set of flags and parameters, as the corresponding tool that it was based on.
Notes
This PR should be tested against PSI usecases still.