cmd/jujud: run API server

cmd/jujud: run API server

We'll make everything connect to it in a forthcoming CL.

https://code.launchpad.net/~rogpeppe/juju-core/179-jujud-server/+merge/144158

(do not edit description out of merge proposal)

[rog, 2013-01-21 19:50:49]

Please take a look.

[niemeyer, 2013-01-22 12:02:12]

This is looking good. A few details:

https://codereview.appspot.com/7178044/diff/7001/cmd/jujud/machine.go
File cmd/jujud/machine.go (right):

https://codereview.appspot.com/7178044/diff/7001/cmd/jujud/machine.go#newcode62
cmd/jujud/machine.go:62: // so that it can mutate it independently.
The most interesting thing to comment here is why we're "maybe running the API
server" ahead of time. It's fresh in our minds, but it won't be obvious in a
while.

https://codereview.appspot.com/7178044/diff/7001/cmd/jujud/machine.go#newcode82
cmd/jujud/machine.go:82: if err == worker.ErrDead {
The tomb will only hold the first error. If the second channel has a real issue,
it will be eaten up, won't it?

https://codereview.appspot.com/7178044/diff/7001/cmd/jujud/machine.go#newcode85
cmd/jujud/machine.go:85: if ug, ok := err.(*UpgradeReadyError); ok {
Same.

https://codereview.appspot.com/7178044/diff/7001/cmd/jujud/machine.go#newcode107
cmd/jujud/machine.go:107: case state.JobServeAPI:
There should be a comment here explaining why we're ignoring it.

https://codereview.appspot.com/7178044/diff/7001/cmd/jujud/machine.go#newcode135
cmd/jujud/machine.go:135: func (a *MachineAgent) maybeRunAPIServerOnce(conf
*agent.Conf) error {
Should be documented since its context is not entirely straightforward.

https://codereview.appspot.com/7178044/diff/7001/cmd/jujud/machine.go#newcode145
cmd/jujud/machine.go:145: runAPI = true
break

https://codereview.appspot.com/7178044/diff/7001/environs/agent/agent.go
File environs/agent/agent.go (right):

https://codereview.appspot.com/7178044/diff/7001/environs/agent/agent.go#newc...
environs/agent/agent.go:87: // EntityName returns the configuration's entity
name,
Could be clarified a bit. What's a configuration's entity? Also, it's ending in
a comma.

https://codereview.appspot.com/7178044/diff/7001/environs/agent/agent.go#newc...
environs/agent/agent.go:187: // returns a non-empty newPassword, the StateInfo
password should be
"the StateInfo password" isn't very clear. It would be best to describe which
password has actually been changed, instead of describing a type name. If it was
merely about changing the StateInfo password, it could do that by itself.

https://codereview.appspot.com/7178044/diff/7001/environs/agent/agent_test.go
File environs/agent/agent_test.go (right):

https://codereview.appspot.com/7178044/diff/7001/environs/agent/agent_test.go...
environs/agent/agent_test.go:335: c.Assert(changed, Equals, "")
changed?

https://codereview.appspot.com/7178044/diff/7001/environs/agent/agent_test.go...
environs/agent/agent_test.go:349: c.Assert(newPassword, Not(Equals), "")
Matches, ...?

https://codereview.appspot.com/7178044/diff/7001/environs/agent/agent_test.go...
environs/agent/agent_test.go:367: c.Assert(newPassword, Not(Equals), "")
Same

https://codereview.appspot.com/7178044/diff/7001/state/api/serve.go
File state/api/serve.go (right):

https://codereview.appspot.com/7178044/diff/7001/state/api/serve.go#newcode1
state/api/serve.go:1: package api
Perhaps not for this CL, but this file seems misnamed. client.go, serve*r*.go..
even the type is Server.

[rog, 2013-01-22 14:44:16]

Please take a look.

https://codereview.appspot.com/7178044/diff/7001/cmd/jujud/machine.go
File cmd/jujud/machine.go (right):

https://codereview.appspot.com/7178044/diff/7001/cmd/jujud/machine.go#newcode62
cmd/jujud/machine.go:62: // so that it can mutate it independently.
On 2013/01/22 12:02:12, niemeyer wrote:
> The most interesting thing to comment here is why we're "maybe running the API
> server" ahead of time. It's fresh in our minds, but it won't be obvious in a
> while.

Done.

https://codereview.appspot.com/7178044/diff/7001/cmd/jujud/machine.go#newcode82
cmd/jujud/machine.go:82: if err == worker.ErrDead {
On 2013/01/22 12:02:12, niemeyer wrote:
> The tomb will only hold the first error. If the second channel has a real
issue,
> it will be eaten up, won't it?

done, hopefully.

https://codereview.appspot.com/7178044/diff/7001/cmd/jujud/machine.go#newcode107
cmd/jujud/machine.go:107: case state.JobServeAPI:
On 2013/01/22 12:02:12, niemeyer wrote:
> There should be a comment here explaining why we're ignoring it.
> 

Done.

https://codereview.appspot.com/7178044/diff/7001/cmd/jujud/machine.go#newcode135
cmd/jujud/machine.go:135: func (a *MachineAgent) maybeRunAPIServerOnce(conf
*agent.Conf) error {
On 2013/01/22 12:02:12, niemeyer wrote:
> Should be documented since its context is not entirely straightforward.

Done.

https://codereview.appspot.com/7178044/diff/7001/cmd/jujud/machine.go#newcode145
cmd/jujud/machine.go:145: runAPI = true
On 2013/01/22 12:02:12, niemeyer wrote:
> break

Done.

https://codereview.appspot.com/7178044/diff/7001/environs/agent/agent.go
File environs/agent/agent.go (right):

https://codereview.appspot.com/7178044/diff/7001/environs/agent/agent.go#newc...
environs/agent/agent.go:87: // EntityName returns the configuration's entity
name,
On 2013/01/22 12:02:12, niemeyer wrote:
> Could be clarified a bit. What's a configuration's entity? Also, it's ending
in
> a comma.

Done.

https://codereview.appspot.com/7178044/diff/7001/environs/agent/agent.go#newc...
environs/agent/agent.go:187: // returns a non-empty newPassword, the StateInfo
password should be
On 2013/01/22 12:02:12, niemeyer wrote:
> "the StateInfo password" isn't very clear. It would be best to describe which
> password has actually been changed, instead of describing a type name. If it
was
> merely about changing the StateInfo password, it could do that by itself.

StateInfo isn't a type name, it's the name of a field in the configuration.

I'm not sure what you're after, but I've made a change anyway.

https://codereview.appspot.com/7178044/diff/7001/environs/agent/agent_test.go
File environs/agent/agent_test.go (right):

https://codereview.appspot.com/7178044/diff/7001/environs/agent/agent_test.go...
environs/agent/agent_test.go:335: c.Assert(changed, Equals, "")
On 2013/01/22 12:02:12, niemeyer wrote:
> changed?

Done.

https://codereview.appspot.com/7178044/diff/7001/environs/agent/agent_test.go...
environs/agent/agent_test.go:349: c.Assert(newPassword, Not(Equals), "")
On 2013/01/22 12:02:12, niemeyer wrote:
> Matches, ...?

Done.

https://codereview.appspot.com/7178044/diff/7001/environs/agent/agent_test.go...
environs/agent/agent_test.go:367: c.Assert(newPassword, Not(Equals), "")
On 2013/01/22 12:02:12, niemeyer wrote:
> Same

Done.

https://codereview.appspot.com/7178044/diff/7001/state/api/serve.go
File state/api/serve.go (right):

https://codereview.appspot.com/7178044/diff/7001/state/api/serve.go#newcode1
state/api/serve.go:1: package api
On 2013/01/22 12:02:12, niemeyer wrote:
> Perhaps not for this CL, but this file seems misnamed. client.go,
serve*r*.go..
> even the type is Server.

i've been thinking the same thing. will change.

[niemeyer, 2013-01-22 14:50:43]

LGTM

[rog, 2013-01-22 15:00:55]

*** Submitted:

cmd/jujud: run API server

We'll make everything connect to it in a forthcoming CL.

R=niemeyer
CC=
https://codereview.appspot.com/7178044