cmd/juju/addmachine: compat with 1.16

cmd/juju/addmachine: compat with 1.16

This addresses bug #1253631. It changes the 'juju add-machine ssh:foo'
to fall back to direct DB access when it is unable to make the new API
calls. Some of the code is also refactored a bit to make it clearer
when we are gathering information, vs when we are acting on that
information (recording it in state, etc).

This also required bringing DestroyMachines back somewhere that we
could share the code (instead of only living in
cmd/juju/destroymachine). I went for state/statecmd even though I
didn't end up sharing that code with the API server. (We could, but
we're quite likely to be deleting it soon, and it seemed like busy
work to move it just to move it back.)

https://code.launchpad.net/~jameinel/juju-core/add-machine-manual-compat-1253631/+merge/198503

(do not edit description out of merge proposal)

[jameinel, 2013-12-11 06:49:27]

Please take a look.

[axw, 2013-12-18 08:47:04]

LGTM, just quibbles

https://codereview.appspot.com/40670043/diff/1/environs/manual/provisioner.go
File environs/manual/provisioner.go (right):

https://codereview.appspot.com/40670043/diff/1/environs/manual/provisioner.go...
environs/manual/provisioner.go:192: Constraints:            
machineParams.Constraints, // not used
If they're not used, why initialise them at all?

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

https://codereview.appspot.com/40670043/diff/1/state/statecmd/machineconfig.g...
state/statecmd/machineconfig.go:17: // TODO: Seems like a layering violation for
the API server and CLI to want to
does this deserve a tech-debt bug?

[fwereade, 2013-12-18 08:54:35]

LGTM modulo possible changes re authentication.go

https://codereview.appspot.com/40670043/diff/1/environs/manual/provisioner.go
File environs/manual/provisioner.go (right):

https://codereview.appspot.com/40670043/diff/1/environs/manual/provisioner.go...
environs/manual/provisioner.go:255: nonce := fmt.Sprintf("%s:%s", instanceId,
uuid.String())
This was always the nonce for a manual machine, right? If so, I think we *do*
have a foolproof way to determine what's a manual machine: check for a
manual-style nonce. (I was always twitchy about "manual:" as an instance-id
prefix, but we have complete control over the nonces and don't use "manual:" in
any other situation.)

axw, I think this may be relevant to your interests.

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

https://codereview.appspot.com/40670043/diff/1/state/statecmd/machineconfig.g...
state/statecmd/machineconfig.go:18: // import worker/provisioner code
Strongly agree here. Two ways to look at the problem: (1) manual provisioning
ought to be hiding behind the environ interface and being driven by a
provisioner somehow; or, (2), worker/provisioner/authentication.go is in the
wrong place. I'm inclined to run with (2) for now -- do you think it could live
happily in environs, or does it want its own package? feels a bit too anaemic
for that really, but I'm a touch reluctant to bloat environs. It still feels
like *something* of a violation for this code to be hitting environs(/tools),
but at least it'd be a bit more focused. Thoughts?

[jameinel, 2013-12-18 12:26:34]

Please take a look.

https://codereview.appspot.com/40670043/diff/1/environs/manual/provisioner.go
File environs/manual/provisioner.go (right):

https://codereview.appspot.com/40670043/diff/1/environs/manual/provisioner.go...
environs/manual/provisioner.go:192: Constraints:            
machineParams.Constraints, // not used
On 2013/12/18 08:47:05, axw wrote:
> If they're not used, why initialise them at all?

Done.

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

https://codereview.appspot.com/40670043/diff/1/state/statecmd/machineconfig.g...
state/statecmd/machineconfig.go:17: // TODO: Seems like a layering violation for
the API server and CLI to want to
On 2013/12/18 08:47:05, axw wrote:
> does this deserve a tech-debt bug?

Done. https://bugs.launchpad.net/juju-core/+bug/1262186

https://codereview.appspot.com/40670043/diff/1/state/statecmd/machineconfig.g...
state/statecmd/machineconfig.go:18: // import worker/provisioner code
Manual provisioning and "bootstrap" are the big points where the juju client
acts very differently from the rest of 'juju'. If we didn't have those, then we
wouldn't need any Provider specific details, or cloud-init details in the juju
client. (It could be nice to move all that code into some other bootstrap-juju
sort of CLI, which also has the advantage that then getting support for a new
provider is just a new one of that little bit, and the rest of the CLI can be
API only.)

Anyway, I don't quite think it fits in environs, but some 3rd party location
seems ok.

[jameinel, 2013-12-19 09:47:01]

I went ahead with environs. I couldn't come up with something better, and I
really did want to not import from provisioner.

[jameinel, 2013-12-19 09:48:14]

Please take a look.