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

On 2014/03/28 10:03:41, axw wrote:
> Please take a look.

> 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
> On 2014/03/28 07:54:23, rog wrote:
> > use this series for newly started instances ?

> Done.

https://codereview.appspot.com/81700043/diff/1/cmd/juju/ensureha.go#newcode50
> cmd/juju/ensureha.go:50: Name: "ensure-ha",
> On 2014/03/28 07:54:23, rog wrote:
> > 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.

> Yeah, it didn't look quite right to me, I was following what was in
the card.
> I've changed it to ensure-availability to be consistent with the API.

> If others disagree, it can be changed easily enough later -- this
command is not
> yet exposed.

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")
> On 2014/03/28 07:54:23, rog wrote:
> > 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.

> Good point, will disallow for now.

> 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 {
> On 2014/03/28 07:54:23, rog wrote:
> > There's no point in having an Error in the result as well as an
error return.

> Done.

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
> On 2014/03/28 07:54:23, rog wrote:
> > d

> Done.

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 {
> On 2014/03/28 07:54:23, rog wrote:
> > err := c.api.state.EnsureAvailability(args.NumStateServers,
args.Constraints,
> > series)
> > return params.EnsureAvailabilityResult{}, err

> Done, except the result is no longer necessary so I've dropped it.

LGTM, thanks!

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