Code review comment for lp:~fwereade/pyjuju/go-deploy-cmd-parsing

Revision history for this message
Gustavo Niemeyer (niemeyer) wrote :

LGTM. A few suggestions, but feel free to submit:

https://codereview.appspot.com/6249075/diff/1/cmd/juju/cmd_test.go
File cmd/juju/cmd_test.go (right):

https://codereview.appspot.com/6249075/diff/1/cmd/juju/cmd_test.go#newcode193
cmd/juju/cmd_test.go:193: os.Setenv("JUJU_REPOSITORY", "/path/to/repo")
The test should preserve the environment as it found before the test
started. E.g.:

     defer os.Setenv("JUJU_REPOSITORY", os.Getenv("JUJU_REPOSITORY"))

The list of tests below look very appropriate for a table of tests as
well. Something along the lines of:

var deployTests = []struct{
     args []string
     cmd *DeployCommand
}{
         {
             {"charm-name", "service-name"},
             &DeployCommand{
                     ...
             },
         }, {
             ...
         },
}

https://codereview.appspot.com/6249075/diff/1/cmd/juju/deploy.go
File cmd/juju/deploy.go (right):

https://codereview.appspot.com/6249075/diff/1/cmd/juju/deploy.go#newcode22
cmd/juju/deploy.go:22: "deploy", "<charm-name> [<service-name>]",
"deploy a new service", "",
We can avoid the dashes in these cases. <charm name> and <service name>
read quite nicely.

Also, can you please document the fact that <service name> defaults to
<charm name> if not provided?

https://codereview.appspot.com/6249075/

« Back to merge proposal