Code review comment for lp:~axwalk/juju-core/cmdapi-ensureavailability

Revision history for this message
Roger Peppe (rogpeppe) wrote :

LGTM modulo the below, thanks!

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

https://codereview.appspot.com/81700043/diff/1/cmd/juju/ensureha.go#newcode19
cmd/juju/ensureha.go:19: // If specified, use this series, else use the
environment default-series
use this series for newly started instances ?

https://codereview.appspot.com/81700043/diff/1/cmd/juju/ensureha.go#newcode50
cmd/juju/ensureha.go:50: Name: "ensure-ha",
I'm not sure about "ha" in lower case - it looks too much like a word.
I'm thinking that either we make it into a proper word ("availability")
or we capitalise the ha - ensure-HA, thus making it clearer that it's an
acronym.

https://codereview.appspot.com/81700043/diff/1/cmd/juju/ensureha.go#newcode58
cmd/juju/ensureha.go:58: f.IntVar(&c.NumStateServers, "n", 1, "number of
state servers to make available")
Thought for the future - it would be good to be able to run "juju
ensure-HA" without any arguments at all, for when we just want to
maintain the current availability. Perhaps rather than defaulting to 1,
we should default to -1 and complain if it's not specified. That way we
leave that path open.

https://codereview.appspot.com/81700043/diff/1/state/api/client.go
File state/api/client.go (right):

https://codereview.appspot.com/81700043/diff/1/state/api/client.go#newcode680
state/api/client.go:680: } else if err := result.Error; err != nil {
There's no point in having an Error in the result as well as an error
return.

https://codereview.appspot.com/81700043/diff/1/state/api/params/params.go
File state/api/params/params.go (right):

https://codereview.appspot.com/81700043/diff/1/state/api/params/params.go#newcode649
state/api/params/params.go:649: Error *Error
d

https://codereview.appspot.com/81700043/diff/1/state/apiserver/client/client.go
File state/apiserver/client/client.go (right):

https://codereview.appspot.com/81700043/diff/1/state/apiserver/client/client.go#newcode990
state/apiserver/client/client.go:990: if err :=
c.api.state.EnsureAvailability(args.NumStateServers, args.Constraints,
series); err != nil {
err := c.api.state.EnsureAvailability(args.NumStateServers,
args.Constraints, series)
return params.EnsureAvailabilityResult{}, err

https://codereview.appspot.com/81700043/

« Back to merge proposal