Code review - Issue 6249075: first step towards a deploy commandhttps://codereview.appspot.com/2012-06-06T09:54:28+00:00rietveld
Message from unknown
2012-06-01T16:15:13+00:00fwereadeurn:md5:dc2d411524c9d33252ee04cf48e2f5a2
Message from fwereade@gmail.com
2012-06-01T16:15:17+00:00fwereadeurn:md5:43137a1249c8b5e32d01984c9451fedb
Please take a look.
Message from n13m3y3r@gmail.com
2012-06-05T21:12:52+00:00niemeyerurn:md5:ad1303af1a6bf6a8b25778ed917580cf
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?
Message from unknown
2012-06-06T09:53:57+00:00fwereadeurn:md5:8ee8e370f45a45fc6ea1aa7ce8cc55f5
Message from fwereade@gmail.com
2012-06-06T09:54:28+00:00fwereadeurn:md5:20a00bacb5b3466cc0efcceb452adef6
*** Submitted:
first step towards a deploy command
(all this does is parse *most* args; but IMO it's a chunk of useful
functionality nonetheless)
R=niemeyer
CC=
https://codereview.appspot.com/6249075
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")
On 2012/06/05 21:12:52, niemeyer wrote:
> 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{
> ...
> },
> }, {
> ...
> },
> }
>
>
>
Done.
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", "",
On 2012/06/05 21:12:52, niemeyer wrote:
> 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?
Done.