Merge lp:~fwereade/pyjuju/go-deploy-cmd-parsing into lp:pyjuju/go

Proposed by William Reade
Status: Merged
Approved by: Gustavo Niemeyer
Approved revision: 198
Merged at revision: 203
Proposed branch: lp:~fwereade/pyjuju/go-deploy-cmd-parsing
Merge into: lp:pyjuju/go
Diff against target: 0 lines
To merge this branch: bzr merge lp:~fwereade/pyjuju/go-deploy-cmd-parsing
Reviewer Review Type Date Requested Status
Juju Engineering Pending
Review via email: mp+108375@code.launchpad.net

Description of the change

first step towards a deploy command

(all this does is parse *most* args; but IMO it's a chunk of useful
functionality nonetheless)

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

To post a comment you must log in.
Revision history for this message
William Reade (fwereade) wrote :

Reviewers: mp+108375_code.launchpad.net,

Message:
Please take a look.

Description:
first step towards a deploy command

(all this does is parse *most* args; but IMO it's a chunk of useful
functionality nonetheless)

https://code.launchpad.net/~fwereade/juju/go-deploy-cmd-parsing/+merge/108375

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/6249075/

Affected files:
   A [revision details]
   M cmd/juju/cmd_test.go
   A cmd/juju/deploy.go

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/

Revision history for this message
William Reade (fwereade) wrote :

*** 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.

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

199. By William Reade on 2012-06-06

merge parent, address review

Preview Diff

Empty

Subscribers

People subscribed via source and target branches