cmd/juju: factor out deploy functionality into juju package

cmd/juju: factor out deploy functionality into juju package



https://code.launchpad.net/~rogpeppe/juju-core/juju-deploy/+merge/115956

(do not edit description out of merge proposal)

[fwereade, 2012-07-20 13:26:50]

Generally good, couple of suggestions.

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

https://codereview.appspot.com/6428061/diff/4001/cmd/juju/cmd_test.go#newcode213
cmd/juju/cmd_test.go:213: &DeployCommand{UpgradeCharm: true},
Was changed to BumpRevision at niemeyer's request, on the basis that it's much
clearer for us, even if we can't fix the user-visible interface.

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

https://codereview.appspot.com/6428061/diff/4001/cmd/juju/deploy.go#newcode108
cmd/juju/deploy.go:108: _, err := conn.StartUnit(svc)
Can we make this StartUnits(svc, c.NumUnits) please?

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

https://codereview.appspot.com/6428061/diff/4001/cmd/juju/deploy_test.go#newc...
cmd/juju/deploy_test.go:153: c.Assert(err, ErrorMatches, `cannot upgrade charm
"local:precise/dummy-1": not a directory`)
Hmm, I've been using "can't" on the basis that I see it everywhere... I think,
if we *have* a convention, it's "can't".

https://codereview.appspot.com/6428061/diff/4001/juju/deploy.go
File juju/deploy.go (right):

https://codereview.appspot.com/6428061/diff/4001/juju/deploy.go#newcode23
juju/deploy.go:23: svcName = sch.URL().Name	// TODO sch.Meta().Name ?
Should be the same, I think: IIRC repos ignore the dir name in favour of the
Meta name.

https://codereview.appspot.com/6428061/diff/4001/juju/deploy.go#newcode36
juju/deploy.go:36: state.RelationScope(rel.Scope),
Hmm, this makes me think the RelationRole and RelationScope types should
actually be in the charm package. But then, counterpartRole...

https://codereview.appspot.com/6428061/diff/4001/juju/deploy.go#newcode68
juju/deploy.go:68: // directory.
drop "unless upgrade is true"; explain that attempting to "upgrade" (I'd prefer
bumpRevision) a charm that is *not* a local directory will cause an error.

https://codereview.appspot.com/6428061/diff/4001/juju/deploy.go#newcode102
juju/deploy.go:102: var buf bytes.Buffer
Maybe drop in a TODO here for not using a buffer?

https://codereview.appspot.com/6428061/diff/4001/state/service.go
File state/service.go (left):

https://codereview.appspot.com/6428061/diff/4001/state/service.go#oldcode105
state/service.go:105: func (s *Service) AddUnit() (*Unit, error) {
AddUnits(count int) ([]*Unit, error), perhaps?

That's surely one for another CL; I feel a bit more strongly that Conn should
have AddUnit*s*.

Failures half way through would be bad, but no worse than they currently are, I
think.

https://codereview.appspot.com/6428061/diff/4001/state/service.go
File state/service.go (right):

https://codereview.appspot.com/6428061/diff/4001/state/service.go#newcode107
state/service.go:107: // each time we add a unit.
I don't think so: we need to at least get the charm URL every time to be sure it
hasn't changed.

[dfc, 2012-07-24 08:22:14]

LGTM.

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

https://codereview.appspot.com/6428061/diff/6008/cmd/juju/deploy.go#newcode86
cmd/juju/deploy.go:86: // TODO get default series from state environ config.
Please put your name against this TODO, or raise a bug so we don't loose it.

https://codereview.appspot.com/6428061/diff/6008/cmd/juju/deploy.go#newcode98
cmd/juju/deploy.go:98: }
I know you didn't write this bit, but return errors.New(...)

[rog, 2012-07-24 15:17:54]

Please take a look.

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

https://codereview.appspot.com/6428061/diff/4001/cmd/juju/cmd_test.go#newcode213
cmd/juju/cmd_test.go:213: &DeployCommand{UpgradeCharm: true},
On 2012/07/20 13:26:50, fwereade wrote:
> Was changed to BumpRevision at niemeyer's request, on the basis that it's much
> clearer for us, even if we can't fix the user-visible interface.

Done.

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

https://codereview.appspot.com/6428061/diff/4001/cmd/juju/deploy.go#newcode108
cmd/juju/deploy.go:108: _, err := conn.StartUnit(svc)
On 2012/07/20 13:26:50, fwereade wrote:
> Can we make this StartUnits(svc, c.NumUnits) please?

Done.

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

https://codereview.appspot.com/6428061/diff/4001/cmd/juju/deploy_test.go#newc...
cmd/juju/deploy_test.go:153: c.Assert(err, ErrorMatches, `cannot upgrade charm
"local:precise/dummy-1": not a directory`)
On 2012/07/20 13:26:50, fwereade wrote:
> Hmm, I've been using "can't" on the basis that I see it everywhere... I think,
> if we *have* a convention, it's "can't".

after discussion, we'll move to "cannot" throughout.

https://codereview.appspot.com/6428061/diff/4001/juju/deploy.go
File juju/deploy.go (right):

https://codereview.appspot.com/6428061/diff/4001/juju/deploy.go#newcode36
juju/deploy.go:36: state.RelationScope(rel.Scope),
On 2012/07/20 13:26:50, fwereade wrote:
> Hmm, this makes me think the RelationRole and RelationScope types should
> actually be in the charm package. But then, counterpartRole...

will do in upcoming branch

https://codereview.appspot.com/6428061/diff/4001/juju/deploy.go#newcode68
juju/deploy.go:68: // directory.
On 2012/07/20 13:26:50, fwereade wrote:
> drop "unless upgrade is true"; explain that attempting to "upgrade" (I'd
prefer
> bumpRevision) a charm that is *not* a local directory will cause an error.

Done.

https://codereview.appspot.com/6428061/diff/4001/juju/deploy.go#newcode102
juju/deploy.go:102: var buf bytes.Buffer
On 2012/07/20 13:26:50, fwereade wrote:
> Maybe drop in a TODO here for not using a buffer?

it's bug #1017733

https://codereview.appspot.com/6428061/diff/4001/state/service.go
File state/service.go (left):

https://codereview.appspot.com/6428061/diff/4001/state/service.go#oldcode105
state/service.go:105: func (s *Service) AddUnit() (*Unit, error) {
On 2012/07/20 13:26:50, fwereade wrote:
> AddUnits(count int) ([]*Unit, error), perhaps?
> 
> That's surely one for another CL; I feel a bit more strongly that Conn should
> have AddUnit*s*.
> 
> Failures half way through would be bad, but no worse than they currently are,
I
> think.

Done.

https://codereview.appspot.com/6428061/diff/4001/state/service.go
File state/service.go (right):

https://codereview.appspot.com/6428061/diff/4001/state/service.go#newcode107
state/service.go:107: // each time we add a unit.
On 2012/07/20 13:26:50, fwereade wrote:
> I don't think so: we need to at least get the charm URL every time to be sure
it
> hasn't changed.

i've added bug #1028482.

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

https://codereview.appspot.com/6428061/diff/6008/cmd/juju/deploy.go#newcode86
cmd/juju/deploy.go:86: // TODO get default series from state environ config.
On 2012/07/24 08:22:14, dfc wrote:
> Please put your name against this TODO, or raise a bug so we don't loose it.

bug #1028484

https://codereview.appspot.com/6428061/diff/6008/cmd/juju/deploy.go#newcode98
cmd/juju/deploy.go:98: }
On 2012/07/24 08:22:14, dfc wrote:
> I know you didn't write this bit, but return errors.New(...)

Done.

[dfc, 2012-07-24 15:31:15]

LGTM. 

I think you should create a separate branch for the gofmt and test changes.

https://codereview.appspot.com/6428061/diff/5007/charm/dir.go
File charm/dir.go (right):

https://codereview.appspot.com/6428061/diff/5007/charm/dir.go#newcode110
charm/dir.go:110: // It is conventional to give charm bundle files a ".charm"
suffix.
maybe less conversationally: "By convention, a charm bundle should have a
".charm" suffix.

https://codereview.appspot.com/6428061/diff/5007/juju/conn_test.go
File juju/conn_test.go (right):

https://codereview.appspot.com/6428061/diff/5007/juju/conn_test.go#newcode61
juju/conn_test.go:61: defer conn.Destroy()
What about line 65? I think it would be better to change lines 63 and 64 to use
c.Check

https://codereview.appspot.com/6428061/diff/5007/juju/deploy.go
File juju/deploy.go (right):

https://codereview.appspot.com/6428061/diff/5007/juju/deploy.go#newcode72
juju/deploy.go:72: if err = chd.SetDiskRevision(chd.Revision() + 1); err != nil
{
Does this have a data race ? Is it crucial that the revision is monotonically
increasing ?

[rog, 2012-07-24 17:27:28]

https://codereview.appspot.com/6428061/diff/5007/charm/dir.go
File charm/dir.go (right):

https://codereview.appspot.com/6428061/diff/5007/charm/dir.go#newcode110
charm/dir.go:110: // It is conventional to give charm bundle files a ".charm"
suffix.
On 2012/07/24 15:31:15, dfc wrote:
> maybe less conversationally: "By convention, a charm bundle should have a
> ".charm" suffix.

Done.

https://codereview.appspot.com/6428061/diff/5007/juju/conn_test.go
File juju/conn_test.go (right):

https://codereview.appspot.com/6428061/diff/5007/juju/conn_test.go#newcode61
juju/conn_test.go:61: defer conn.Destroy()
On 2012/07/24 15:31:15, dfc wrote:
> What about line 65? I think it would be better to change lines 63 and 64 to
use
> c.Check

Done.

https://codereview.appspot.com/6428061/diff/5007/juju/deploy.go
File juju/deploy.go (right):

https://codereview.appspot.com/6428061/diff/5007/juju/deploy.go#newcode72
juju/deploy.go:72: if err = chd.SetDiskRevision(chd.Revision() + 1); err != nil
{
On 2012/07/24 15:31:15, dfc wrote:
> Does this have a data race ? Is it crucial that the revision is monotonically
> increasing ?

Yes, there is a data race. No, there's nothing we can do about it, I don't
think, and I doubt that it matters. If someone is writing to the charm
concurrently with uploading it, they probably deserve what they get.

[rog, 2012-07-24 17:37:18]

Please take a look.

[niemeyer, 2012-07-24 18:31:45]

Good stuff. Only a few trivials for us to talk about.

https://codereview.appspot.com/6428061/diff/5007/juju/deploy.go
File juju/deploy.go (right):

https://codereview.appspot.com/6428061/diff/5007/juju/deploy.go#newcode17
juju/deploy.go:17: func (conn *Conn) NewService(sch *state.Charm, svcName
string) (*state.Service, error) {
I believe this should be called AddService, and we should also make the
parameters match the ordering of state.AddService (in either direction).

I'd also name the parameter as "serviceName". Saving 4 letters in the simple use
below isn't worth the lack of readability in the API docs, IMO.

https://codereview.appspot.com/6428061/diff/3003/juju/deploy.go
File juju/deploy.go (right):

https://codereview.appspot.com/6428061/diff/3003/juju/deploy.go#newcode17
juju/deploy.go:17: func (conn *Conn) NewService(sch *state.Charm, svcName
string) (*state.Service, error) {
This should be called AddService as usual in the command line and state
interface, and parameters should be in the same order as we give them to
AddService in state (we can fix in either direction).

https://codereview.appspot.com/6428061/diff/3003/juju/deploy.go#newcode127
juju/deploy.go:127: func (conn *Conn) StartUnits(svc *state.Service, n int)
([]*state.Unit, error) {
s/StartUnits/AddUnits/ (both the command line and the state package refer to it
like this)

https://codereview.appspot.com/6428061/diff/3003/juju/deploy_test.go
File juju/deploy_test.go (right):

https://codereview.appspot.com/6428061/diff/3003/juju/deploy_test.go#newcode130
juju/deploy_test.go:130: c.Assert(sch.BundleSha256(), Not(Equals), sha256)
It'd be nicer to check the revision instead. That's what bump means there.

https://codereview.appspot.com/6428061/diff/3003/testing/zk.go
File testing/zk.go (right):

https://codereview.appspot.com/6428061/diff/3003/testing/zk.go#newcode70
testing/zk.go:70: func (s ZkSuite) TearDownTest(c *C) {
Is this being run without it?

[rog, 2012-07-25 10:24:37]

Please take a look.

https://codereview.appspot.com/6428061/diff/5007/juju/deploy.go
File juju/deploy.go (right):

https://codereview.appspot.com/6428061/diff/5007/juju/deploy.go#newcode17
juju/deploy.go:17: func (conn *Conn) NewService(sch *state.Charm, svcName
string) (*state.Service, error) {
On 2012/07/24 18:31:45, niemeyer wrote:
> I believe this should be called AddService, and we should also make the
> parameters match the ordering of state.AddService (in either direction).
> 
> I'd also name the parameter as "serviceName". Saving 4 letters in the simple
use
> below isn't worth the lack of readability in the API docs, IMO.

done. except i used "name" instead of serviceName, following state.

https://codereview.appspot.com/6428061/diff/3003/juju/deploy_test.go
File juju/deploy_test.go (right):

https://codereview.appspot.com/6428061/diff/3003/juju/deploy_test.go#newcode130
juju/deploy_test.go:130: c.Assert(sch.BundleSha256(), Not(Equals), sha256)
On 2012/07/24 18:31:46, niemeyer wrote:
> It'd be nicer to check the revision instead. That's what bump means there.

i've done both and changed the name of the test.

https://codereview.appspot.com/6428061/diff/3003/testing/zk.go
File testing/zk.go (right):

https://codereview.appspot.com/6428061/diff/3003/testing/zk.go#newcode70
testing/zk.go:70: func (s ZkSuite) TearDownTest(c *C) {
On 2012/07/24 18:31:46, niemeyer wrote:
> Is this being run without it?

discussed in person.

[niemeyer, 2012-07-25 10:29:58]

LGTM. Only a trivial in the doc:

https://codereview.appspot.com/6428061/diff/17001/juju/deploy.go
File juju/deploy.go (right):

https://codereview.appspot.com/6428061/diff/17001/juju/deploy.go#newcode15
juju/deploy.go:15: // NewService creates a new service with the given name to
run the given
s/New/Add/

[rog, 2012-07-25 11:14:47]

Please take a look.

[rog, 2012-07-25 11:16:54]

*** Submitted:

cmd/juju: factor out deploy functionality into juju package

R=fwereade, dfc, niemeyer
CC=
https://codereview.appspot.com/6428061