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)