cmd/juju: deploy checks params in Init

cmd/juju: deploy checks params in Init

...thereby rejecting obviously-crazy charm or service specifiers before
connecting to state. Fixes lp:1090172.

https://code.launchpad.net/~fwereade/juju-core/deploy-check-names-early/+merge/141159

(do not edit description out of merge proposal)

[fwereade, 2012-12-22 19:25:04]

Please take a look.

[dimitern, 2013-01-03 22:09:05]

LGTM, with some questions.

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

https://codereview.appspot.com/7001050/diff/1/cmd/juju/deploy.go#newcode83
cmd/juju/deploy.go:83: // Reviewers, start your opinions.
OK, what happens when deploying subordinates?

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

https://codereview.appspot.com/7001050/diff/1/cmd/juju/deploy_test.go#newcode129
cmd/juju/deploy_test.go:129: err:  `invalid service name "burble-1"`,
so only the last error is reported? is "craziness" a valid charm name?

[fwereade, 2013-01-04 08:25:49]

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

https://codereview.appspot.com/7001050/diff/1/cmd/juju/deploy.go#newcode83
cmd/juju/deploy.go:83: // Reviewers, start your opinions.
On 2013/01/03 22:09:05, dimitern wrote:
> OK, what happens when deploying subordinates?

Deploying a subordinate service just creates the service; subordinate units are
created by their principals as a result of adding relations with the subordinate
service. It's not sane to directly add or destroy subordinate units.

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

https://codereview.appspot.com/7001050/diff/1/cmd/juju/deploy_test.go#newcode129
cmd/juju/deploy_test.go:129: err:  `invalid service name "burble-1"`,
On 2013/01/03 22:09:05, dimitern wrote:
> so only the last error is reported? is "craziness" a valid charm name?

It's not a valid charm URL, but it is a valid name, and there's no reason at
this stage to believe that we won't be able to infer a valid charm URL from it.

[dimitern, 2013-01-04 09:15:19]

On 2013/01/04 08:25:49, fwereade wrote:
> https://codereview.appspot.com/7001050/diff/1/cmd/juju/deploy.go
> File cmd/juju/deploy.go (right):
> 
> https://codereview.appspot.com/7001050/diff/1/cmd/juju/deploy.go#newcode83
> cmd/juju/deploy.go:83: // Reviewers, start your opinions.
> On 2013/01/03 22:09:05, dimitern wrote:
> > OK, what happens when deploying subordinates?
> 
> Deploying a subordinate service just creates the service; subordinate units
are
> created by their principals as a result of adding relations with the
subordinate
> service. It's not sane to directly add or destroy subordinate units.
> 
> https://codereview.appspot.com/7001050/diff/1/cmd/juju/deploy_test.go
> File cmd/juju/deploy_test.go (right):
> 
>
https://codereview.appspot.com/7001050/diff/1/cmd/juju/deploy_test.go#newcode129
> cmd/juju/deploy_test.go:129: err:  `invalid service name "burble-1"`,
> On 2013/01/03 22:09:05, dimitern wrote:
> > so only the last error is reported? is "craziness" a valid charm name?
> 
> It's not a valid charm URL, but it is a valid name, and there's no reason at
> this stage to believe that we won't be able to infer a valid charm URL from
it.

OK, fair enough, looks solid to me.

[jameinel, 2013-01-06 10:15:53]

LGTM

[rog, 2013-01-07 08:48:01]

looks good, modulo the below.

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

https://codereview.appspot.com/7001050/diff/1/cmd/juju/deploy.go#newcode81
cmd/juju/deploy.go:81: if c.NumUnits < 1 {
why not just remove this check? i think it's perfectly reasonable to want to
create a service with no units. zero is a fine number.

[fwereade, 2013-01-07 08:48:04]

*** Submitted:

cmd/juju: deploy checks params in Init

...thereby rejecting obviously-crazy charm or service specifiers before
connecting to state. Fixes lp:1090172.

R=dimitern, jameinel
CC=
https://codereview.appspot.com/7001050