ProvisioningScript client API

ProvisioningScript client API

This is a new client API that, given the ID and nonce
of a machine injected into state, returns a script
that can be executed on that machine to install and
configure a Juju machine agent.

The existing MachineConfig client API call has been
modified: it was taking a series and arch. Series is
guaranteed to be in state.Machine; the arch is not,
but we make it a requirement for calling MachineConfig.
In the only place where MachineConfig is already called,
this is true. ProvisioningScript is expected to be
used similarly.

I have tested "live" by temporarily modifying
environs/manual/provisioner.go to call ProvisioningScript
and then running it via utils/ssh.Command.

https://code.launchpad.net/~axwalk/juju-core/provisioningscript-client-api/+merge/201893

(do not edit description out of merge proposal)

[axw, 2014-01-16 07:36:55]

Please take a look.

[rog, 2014-01-16 12:02:47]

LGTM

https://codereview.appspot.com/53040043/diff/1/state/statecmd/machineconfig.go
File state/statecmd/machineconfig.go (right):

https://codereview.appspot.com/53040043/diff/1/state/statecmd/machineconfig.g...
state/statecmd/machineconfig.go:92: func FinishMachineConfig(configParameters
params.MachineConfig, machineId, nonce, dataDir string)
(*cloudinit.MachineConfig, error) {
This seems fine, but I don't entirely understand its rationale. Perhaps you
could explain why this needs to be separate from MachineConfig?

[dimitern, 2014-01-16 13:08:18]

Looking good, apart from some grumbles below.

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

https://codereview.appspot.com/53040043/diff/1/state/api/client.go#newcode191
state/api/client.go:191: return result.Script, err
I think it's a bit better like this:
err = ...
if err != nil {
  return "", err
}
return result.Script, nil

(like other similar methods).

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

https://codereview.appspot.com/53040043/diff/1/state/api/params/params.go#old...
state/api/params/params.go:521: Series    string
I'm not sure we can remove these fields just yet (1.16 compatibility).

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

https://codereview.appspot.com/53040043/diff/1/state/api/params/params.go#new...
state/api/params/params.go:545: // ProvisioningSCript client API call.
s/ProvisioningSCript/ProvisioningScript/ (and in the comment above as well)

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

https://codereview.appspot.com/53040043/diff/1/state/apiserver/client/client....
state/apiserver/client/client.go:18: coreCloudinit
"launchpad.net/juju-core/cloudinit"
I'd prefer corecloudinit for consistency sake.

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

https://codereview.appspot.com/53040043/diff/1/state/apiserver/client/client_...
state/apiserver/client/client_test.go:15: coreCloudinit
"launchpad.net/juju-core/cloudinit"
corecloudinit please

https://codereview.appspot.com/53040043/diff/1/state/apiserver/client/client_...
state/apiserver/client/client_test.go:1764: 
I'd like to see a test about using ProvisioningScript against 1.16 API server,
to make sure we're handling it gracefully when it's not implemented.

https://codereview.appspot.com/53040043/diff/1/state/statecmd/machineconfig.go
File state/statecmd/machineconfig.go (right):

https://codereview.appspot.com/53040043/diff/1/state/statecmd/machineconfig.g...
state/statecmd/machineconfig.go:59: return result, fmt.Errorf("arch is not set
for %s", machine.Tag())
s/for %s/for %q/ ?

https://codereview.appspot.com/53040043/diff/1/state/statecmd/machineconfig.g...
state/statecmd/machineconfig.go:92: func FinishMachineConfig(configParameters
params.MachineConfig, machineId, nonce, dataDir string)
(*cloudinit.MachineConfig, error) {
On 2014/01/16 12:02:47, rog wrote:
> This seems fine, but I don't entirely understand its rationale. Perhaps you
> could explain why this needs to be separate from MachineConfig?

I think it's mostly because of the provisioner, which also needs to call this
separately from MachineConfig.

[hazmat, 2014-01-16 13:48:13]

On 2014/01/16 12:02:47, rog wrote:
> LGTM
> 

> This seems fine, but I don't entirely understand its rationale. Perhaps you
> could explain why this needs to be separate from MachineConfig?

Machine config exposes data (mostly internal imo) via the api that a client has
to then format into an install script. This is a fragile error prone process as
it is effectively reimplementing internal juju details and the various cloudinit
modules and options within juju core are subject to change without notice. By
exposing a provisioning script to the client, the client basically gets a blob
that it can use for initialization which is a much cleaner api then exposing
internal details and leaving the burden on the client.

[axw, 2014-01-17 03:22:37]

Please take a look.

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

https://codereview.appspot.com/53040043/diff/1/state/api/client.go#newcode191
state/api/client.go:191: return result.Script, err
On 2014/01/16 13:08:18, dimitern wrote:
> I think it's a bit better like this:
> err = ...
> if err != nil {
>   return "", err
> }
> return result.Script, nil
> 
> (like other similar methods).

Sure, though there's quite a lot that do it the way I did it (I just copied
MachineConfig).

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

https://codereview.appspot.com/53040043/diff/1/state/api/params/params.go#old...
state/api/params/params.go:521: Series    string
On 2014/01/16 13:08:18, dimitern wrote:
> I'm not sure we can remove these fields just yet (1.16 compatibility).

MachineConfig was added in 1.17.0. I may just remove MachineConfig altogether.
It's only used by manual provisioning, which could just use the new API instead.

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

https://codereview.appspot.com/53040043/diff/1/state/api/params/params.go#new...
state/api/params/params.go:545: // ProvisioningSCript client API call.
On 2014/01/16 13:08:18, dimitern wrote:
> s/ProvisioningSCript/ProvisioningScript/ (and in the comment above as well)

Done.

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

https://codereview.appspot.com/53040043/diff/1/state/apiserver/client/client....
state/apiserver/client/client.go:18: coreCloudinit
"launchpad.net/juju-core/cloudinit"
On 2014/01/16 13:08:18, dimitern wrote:
> I'd prefer corecloudinit for consistency sake.

coreCloudinit is consistent with the rest of the code base (there are zero
instances of corecloudinit, a bunch of coreCloudinit).

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

https://codereview.appspot.com/53040043/diff/1/state/apiserver/client/client_...
state/apiserver/client/client_test.go:1764: 
On 2014/01/16 13:08:18, dimitern wrote:
> I'd like to see a test about using ProvisioningScript against 1.16 API server,
> to make sure we're handling it gracefully when it's not implemented.

I was going to do this, but it doesn't make sense to yet*. The only place it's
used is by an external client, where the version is guaranteed to include the
method.

* When I change environs/manual to use ProvisioningScript instead of
MachineConfig, it may make sense. However, I don't see *any* tests for
compatibility, nor do I know how this would even be accomplished. If there
really are tests for the 1dot16 code paths, please point me at one.

https://codereview.appspot.com/53040043/diff/1/state/statecmd/machineconfig.go
File state/statecmd/machineconfig.go (right):

https://codereview.appspot.com/53040043/diff/1/state/statecmd/machineconfig.g...
state/statecmd/machineconfig.go:59: return result, fmt.Errorf("arch is not set
for %s", machine.Tag())
On 2014/01/16 13:08:18, dimitern wrote:
> s/for %s/for %q/ ?

Done.

https://codereview.appspot.com/53040043/diff/1/state/statecmd/machineconfig.g...
state/statecmd/machineconfig.go:92: func FinishMachineConfig(configParameters
params.MachineConfig, machineId, nonce, dataDir string)
(*cloudinit.MachineConfig, error) {
On 2014/01/16 13:08:18, dimitern wrote:
> On 2014/01/16 12:02:47, rog wrote:
> > This seems fine, but I don't entirely understand its rationale. Perhaps you
> > could explain why this needs to be separate from MachineConfig?

This is just how environs/cloudinit works: you create a MachineConfig, tweak it
to your liking, and then call FinishMachineConfig.

> I think it's mostly because of the provisioner, which also needs to call this
> separately from MachineConfig.

Yes, that's why it's in this package. I intend to kill the MachineConfig API
call. When MachineConfig's gone, and when 1.16 compat is no longer required in
environs/manual, both of the functions in this file can be moved into
state/apiserver/client.