apiserver/provisioner: Rest of server-side API

apiserver/provisioner: Rest of server-side API

This completes the required server-side API calls
for the provisioner: Series, Constraints, InstanceId,
SetProvisioned, WatchEnvironMachines, Status, EnvironConfig,
and WatchForEnvironConfigChanges.

A drive-by fix was done to state and apiserver to make handling
NotProvisionedErrors from state easiert.

https://code.launchpad.net/~dimitern/juju-core/141-apiserver-provisioner-2/+merge/186355

Requires: https://code.launchpad.net/~dimitern/juju-core/140-provisioner-watchforenvironconfigchanges/+merge/186306

(do not edit description out of merge proposal)

[dimitern, 2013-09-18 15:08:55]

Please take a look.

[rog, 2013-09-19 12:48:06]

Looking good. Some comments and suggestions below.

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

https://codereview.appspot.com/13501051/diff/1/state/api/params/internal.go#n...
state/api/params/internal.go:124: type Config map[string]interface{}
any particular reason for defining this as a type rather than just using
map[string]interface{} directly?

https://codereview.appspot.com/13501051/diff/1/state/apiserver/provisioner/pr...
File state/apiserver/provisioner/provisioner.go (right):

https://codereview.appspot.com/13501051/diff/1/state/apiserver/provisioner/pr...
state/apiserver/provisioner/provisioner.go:86: }
perhaps a comment here to justify the implicit panic:

// The authorization function guarantees that the tag
// represents a machine.

?

https://codereview.appspot.com/13501051/diff/1/state/apiserver/provisioner/pr...
state/apiserver/provisioner/provisioner.go:144: result.Error =
common.ServerError(watcher.MustErr(watch))
i'd prefer if the error was returned as the result of the method.
(that fits with the convention used by other non-bulk calls).

https://codereview.appspot.com/13501051/diff/1/state/apiserver/provisioner/pr...
state/apiserver/provisioner/provisioner.go:174: status, info, err =
machine.Status()
result.Results[i].Status, result.Results[i].Info, err = machine.Status()

?

(it should be fine to rely on the zero values returned by machine.Status on
error).

https://codereview.appspot.com/13501051/diff/1/state/apiserver/provisioner/pr...
state/apiserver/provisioner/provisioner.go:228: // also metadata for each given
machine. Once set, the instance id
s/also//

https://codereview.appspot.com/13501051/diff/1/state/apiserver/provisioner/pr...
state/apiserver/provisioner/provisioner.go:276: result :=
params.StringsWatchResult{}
shouldn't this return ErrPerm if the authenticated entity isn't an environ
manager?

https://codereview.appspot.com/13501051/diff/1/state/apiserver/provisioner/pr...
state/apiserver/provisioner/provisioner.go:284: err = fmt.Errorf("cannot obtain
initial environment machines")
return params.StringsWatchResult{}, fmt.Errorf("cannot obtain initial
environment machines: %v", watcher.MustErr(watch))

?

That way you won't need to call ServerError and you'll see the reason why the
watcher died.

https://codereview.appspot.com/13501051/diff/1/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/13501051/diff/1/state/machine.go#newcode604
state/machine.go:604: if _, ok := err.(*notProvisionedError); ok {
_, ok := err.(*notProvisionedError)
return ok

slightly more concise and obvious, i think.

[fwereade, 2013-09-19 13:24:05]

A few comments further to rog's

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

https://codereview.appspot.com/13501051/diff/1/state/api/params/internal.go#n...
state/api/params/internal.go:296: Info   string
heads up FWIW: there's going to be a data dict as well at some stage. Shouldn't
impact your logic here.

https://codereview.appspot.com/13501051/diff/1/state/apiserver/provisioner/pr...
File state/apiserver/provisioner/provisioner.go (right):

https://codereview.appspot.com/13501051/diff/1/state/apiserver/provisioner/pr...
state/apiserver/provisioner/provisioner.go:174: status, info, err =
machine.Status()
On 2013/09/19 12:48:06, rog wrote:
> result.Results[i].Status, result.Results[i].Info, err = machine.Status()
> 
> ?
> 
> (it should be fine to rely on the zero values returned by machine.Status on
> error).

I'm not sure that line is especially readable. Making the status and info
declarations consistent would be est IMO:

var status params.Status
var info string
status, info, err = ...

https://codereview.appspot.com/13501051/diff/1/state/apiserver/provisioner/pr...
state/apiserver/provisioner/provisioner.go:276: result :=
params.StringsWatchResult{}
On 2013/09/19 12:48:06, rog wrote:
> shouldn't this return ErrPerm if the authenticated entity isn't an environ
> manager?

+1

[dimitern, 2013-09-19 13:58:16]

Please take a look.

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

https://codereview.appspot.com/13501051/diff/1/state/api/params/internal.go#n...
state/api/params/internal.go:124: type Config map[string]interface{}
On 2013/09/19 12:48:06, rog wrote:
> any particular reason for defining this as a type rather than just using
> map[string]interface{} directly?

I prefer it this way, and it's more consistent with the other similar cases,
like Settings.

https://codereview.appspot.com/13501051/diff/1/state/api/params/internal.go#n...
state/api/params/internal.go:296: Info   string
On 2013/09/19 13:24:05, fwereade wrote:
> heads up FWIW: there's going to be a data dict as well at some stage.
Shouldn't
> impact your logic here.

Ok.

https://codereview.appspot.com/13501051/diff/1/state/apiserver/provisioner/pr...
File state/apiserver/provisioner/provisioner.go (right):

https://codereview.appspot.com/13501051/diff/1/state/apiserver/provisioner/pr...
state/apiserver/provisioner/provisioner.go:86: }
On 2013/09/19 12:48:06, rog wrote:
> perhaps a comment here to justify the implicit panic:
> 
> // The authorization function guarantees that the tag
> // represents a machine.
> 
> ?

Done.

https://codereview.appspot.com/13501051/diff/1/state/apiserver/provisioner/pr...
state/apiserver/provisioner/provisioner.go:144: result.Error =
common.ServerError(watcher.MustErr(watch))
On 2013/09/19 12:48:06, rog wrote:
> i'd prefer if the error was returned as the result of the method.
> (that fits with the convention used by other non-bulk calls).
> 
> 

Done.

https://codereview.appspot.com/13501051/diff/1/state/apiserver/provisioner/pr...
state/apiserver/provisioner/provisioner.go:174: status, info, err =
machine.Status()
On 2013/09/19 13:24:05, fwereade wrote:
> On 2013/09/19 12:48:06, rog wrote:
> > result.Results[i].Status, result.Results[i].Info, err = machine.Status()
> > 
> > ?
> > 
> > (it should be fine to rely on the zero values returned by machine.Status on
> > error).
> 
> I'm not sure that line is especially readable. Making the status and info
> declarations consistent would be est IMO:
> 
> var status params.Status
> var info string
> status, info, err = ...

Done.

https://codereview.appspot.com/13501051/diff/1/state/apiserver/provisioner/pr...
state/apiserver/provisioner/provisioner.go:174: status, info, err =
machine.Status()
On 2013/09/19 12:48:06, rog wrote:
> result.Results[i].Status, result.Results[i].Info, err = machine.Status()
> 
> ?
> 
> (it should be fine to rely on the zero values returned by machine.Status on
> error).

I'd prefer not to make the line so long and hard to read. I've defined info as
var string, as fwereade suggested.

https://codereview.appspot.com/13501051/diff/1/state/apiserver/provisioner/pr...
state/apiserver/provisioner/provisioner.go:228: // also metadata for each given
machine. Once set, the instance id
On 2013/09/19 12:48:06, rog wrote:
> s/also//

Done.

https://codereview.appspot.com/13501051/diff/1/state/apiserver/provisioner/pr...
state/apiserver/provisioner/provisioner.go:276: result :=
params.StringsWatchResult{}
On 2013/09/19 12:48:06, rog wrote:
> shouldn't this return ErrPerm if the authenticated entity isn't an environ
> manager?

Done.

https://codereview.appspot.com/13501051/diff/1/state/apiserver/provisioner/pr...
state/apiserver/provisioner/provisioner.go:284: err = fmt.Errorf("cannot obtain
initial environment machines")
On 2013/09/19 12:48:06, rog wrote:
> return params.StringsWatchResult{}, fmt.Errorf("cannot obtain initial
> environment machines: %v", watcher.MustErr(watch))
> 
> ?
> 
> That way you won't need to call ServerError and you'll see the reason why the
> watcher died.

Done.

https://codereview.appspot.com/13501051/diff/1/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/13501051/diff/1/state/machine.go#newcode604
state/machine.go:604: if _, ok := err.(*notProvisionedError); ok {
On 2013/09/19 12:48:06, rog wrote:
> _, ok := err.(*notProvisionedError)
> return ok
> 
> slightly more concise and obvious, i think.

Done.

[rog, 2013-09-19 14:22:25]

LGTM with one final suggestion.

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

https://codereview.appspot.com/13501051/diff/1/state/api/params/internal.go#n...
state/api/params/internal.go:124: type Config map[string]interface{}
On 2013/09/19 13:58:16, dimitern wrote:
> On 2013/09/19 12:48:06, rog wrote:
> > any particular reason for defining this as a type rather than just using
> > map[string]interface{} directly?
> 
> I prefer it this way, and it's more consistent with the other similar cases,
> like Settings.

If it was used in more than one place, I'd tend to agree. As it is, I'm not sure
it adds anything, but fair enough.

https://codereview.appspot.com/13501051/diff/1/state/apiserver/provisioner/pr...
File state/apiserver/provisioner/provisioner.go (right):

https://codereview.appspot.com/13501051/diff/1/state/apiserver/provisioner/pr...
state/apiserver/provisioner/provisioner.go:174: status, info, err =
machine.Status()
On 2013/09/19 13:58:16, dimitern wrote:
> On 2013/09/19 12:48:06, rog wrote:
> > result.Results[i].Status, result.Results[i].Info, err = machine.Status()
> > 
> > ?
> > 
> > (it should be fine to rely on the zero values returned by machine.Status on
> > error).
> 
> I'd prefer not to make the line so long and hard to read. I've defined info as
> var string, as fwereade suggested.

One more suggestion, still 2 lines instead of 7, and
easy to read I think:

r := &result.Results[i]
r.Status, r.Info, err = machine.Status()

[dimitern, 2013-09-19 14:26:46]

Please take a look.

https://codereview.appspot.com/13501051/diff/1/state/apiserver/provisioner/pr...
File state/apiserver/provisioner/provisioner.go (right):

https://codereview.appspot.com/13501051/diff/1/state/apiserver/provisioner/pr...
state/apiserver/provisioner/provisioner.go:174: status, info, err =
machine.Status()
On 2013/09/19 14:22:25, rog wrote:
> On 2013/09/19 13:58:16, dimitern wrote:
> > On 2013/09/19 12:48:06, rog wrote:
> > > result.Results[i].Status, result.Results[i].Info, err = machine.Status()
> > > 
> > > ?
> > > 
> > > (it should be fine to rely on the zero values returned by machine.Status
on
> > > error).
> > 
> > I'd prefer not to make the line so long and hard to read. I've defined info
as
> > var string, as fwereade suggested.
> 
> One more suggestion, still 2 lines instead of 7, and
> easy to read I think:
> 
> r := &result.Results[i]
> r.Status, r.Info, err = machine.Status()

Done.