agent/agent: add StateServingInfo to config

agent/agent: add StateServingInfo to config

Add StateServingInfo and SetStateServingInfo
to configInternal, along with related changes
to flow the information through (including
marshalling and unmarshalling).

https://code.launchpad.net/~mfoord/juju-core/stateservinginfo/+merge/212874

(do not edit description out of merge proposal)

[mfoord, 2014-04-01 17:08:29]

Please take a look.

[rog, 2014-04-01 17:32:12]

LGTM with the following addressed. Thanks!

https://codereview.appspot.com/82960044/diff/1/agent/agent.go
File agent/agent.go (right):

https://codereview.appspot.com/82960044/diff/1/agent/agent.go#newcode158
agent/agent.go:158: // SetStateServingInfo sets the state server related fields
// SetStateServingInfo sets the information needed to
// run a state server.
?

https://codereview.appspot.com/82960044/diff/1/agent/agent.go#newcode319
agent/agent.go:319: config := config0.(*configInternal)
config.SetStateServingInfo(params.StateServingInfo{
...
})
?
then there's no need for the dynamic type conversion.

https://codereview.appspot.com/82960044/diff/1/agent/agent.go#newcode528
agent/agent.go:528: if c.servingInfo.StatePort == 0 {
perhaps better to gate on a secret here?

if len(c.servingInfo.PrivateKey) == 0 {

https://codereview.appspot.com/82960044/diff/1/agent/agent_test.go
File agent/agent_test.go (right):

https://codereview.appspot.com/82960044/diff/1/agent/agent_test.go#newcode393
agent/agent_test.go:393: result, available := conf.StateServingInfo()
available is fine, but "ok" is perhaps more conventional in Go.

https://codereview.appspot.com/82960044/diff/1/agent/bootstrap_test.go
File agent/bootstrap_test.go (right):

https://codereview.appspot.com/82960044/diff/1/agent/bootstrap_test.go#newcode26
agent/bootstrap_test.go:26: func parseStatePort(c *gc.C, address string) int {
i'd move this to the end of the file, as a not-very-interesting helper function.

https://codereview.appspot.com/82960044/diff/1/agent/bootstrap_test.go#newcod...
agent/bootstrap_test.go:162: func (s *bootstrapSuite)
TestInitializeStateServingInfoNotAvailable(c *gc.C) {
s/Initialize/InitializeWith/ ?

https://codereview.appspot.com/82960044/diff/1/agent/bootstrap_test.go#newcod...
agent/bootstrap_test.go:223: StatePort: parseStatePort(c,
configParams.StateAddresses[0]),
StatePort: testing.MgoServer.Port()

?

(then perhaps you won't need parseStatePort at all)

https://codereview.appspot.com/82960044/diff/1/agent/format-1.16.go
File agent/format-1.16.go (right):

https://codereview.appspot.com/82960044/diff/1/agent/format-1.16.go#newcode113
agent/format-1.16.go:113: if len(config.servingInfo.PrivateKey) != 0 {
// There's a private key, then we need the state
// port, which wasn't directly available in the 1.16 format,
// but we can infer it from the ports in the state addresses.

https://codereview.appspot.com/82960044/diff/1/agent/format-1.18.go
File agent/format-1.18.go (right):

https://codereview.appspot.com/82960044/diff/1/agent/format-1.18.go#newcode105
agent/format-1.18.go:105: if len(config.servingInfo.PrivateKey) != 0 {
Similar comment here?

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

https://codereview.appspot.com/82960044/diff/1/cmd/jujud/machine.go#newcode370
cmd/jujud/machine.go:370: panic("StateServingInfo not avilable and we need it")
probably better to return an error here rather than panic,
because we can.

also:
s/avilable/available/

[mfoord, 2014-04-02 10:46:24]

Please take a look.

[rog, 2014-04-02 12:17:38]

LGTM with a few trivial suggestions below.

https://codereview.appspot.com/82960044/diff/20001/agent/agent.go
File agent/agent.go (right):

https://codereview.appspot.com/82960044/diff/20001/agent/agent.go#newcode316
agent/agent.go:316: config0, err :=
NewAgentConfig(configParams.AgentConfigParams)
s/config0/config/

https://codereview.appspot.com/82960044/diff/20001/agent/format-1.16.go
File agent/format-1.16.go (right):

https://codereview.appspot.com/82960044/diff/20001/agent/format-1.16.go#newco...
agent/format-1.16.go:102: if len(stateServerKey) != 0 {
d

https://codereview.appspot.com/82960044/diff/20001/agent/format-1.18.go
File agent/format-1.18.go (right):

https://codereview.appspot.com/82960044/diff/20001/agent/format-1.18.go#newco...
agent/format-1.18.go:109: if config.servingInfo.StatePort == 0 &&
len(format.StateAddresses) > 0 {
 if config.servingInfo.StatePort == 0 {
     if len(format.StateAddresses) == 0 {
         return nil, fmt.Errorf("no state addresses found to infer state port
from")
     }
     ....
  }

and lose the else (better error message, and marginally more understandable
code, i think)