skeleton provider

skeleton provider

...and rudimentary initial implementation notes.

https://code.launchpad.net/~fwereade/juju-core/provider-skeleton/+merge/189638

(do not edit description out of merge proposal)

[fwereade, 2013-10-07 15:26:42]

Please take a look.

[rog, 2013-10-09 08:01:30]

This is great, thanks.
LGTM modulo the below comments and suggestions.

https://codereview.appspot.com/14494043/diff/1/doc/hacking-providers.txt
File doc/hacking-providers.txt (right):

https://codereview.appspot.com/14494043/diff/1/doc/hacking-providers.txt#newc...
doc/hacking-providers.txt:9: "provider" to juju; this basically involves
defining how the substrate can be
s/basically//

https://codereview.appspot.com/14494043/diff/1/doc/hacking-providers.txt#newc...
doc/hacking-providers.txt:28: ...which roughly correspond to the
environs.InstanceBroker interface.
I wonder if it might be nice to add some godoc links,
although it wouldn't improve text-only readability.

For example http://godoc.org/launchpad.net/juju-core/environs#InstanceBroker

https://codereview.appspot.com/14494043/diff/1/doc/hacking-providers.txt#newc...
doc/hacking-providers.txt:84: support library be exposed as interfaces, to
enable easy mocking and hence allow
I'm not sure that exposing the support library as an interface
necessarily makes it much easier to mock. The interfaces can usually
be defined in the calling package.

https://codereview.appspot.com/14494043/diff/1/doc/hacking-providers.txt#newc...
doc/hacking-providers.txt:103: So we have "dependencies.tsv" in the root of
juju-core. Tooling support is spotty
Perhaps you could mention launchpad.net/godeps here - it is used to generate the
file and can be used to update the dependencies too.

https://codereview.appspot.com/14494043/diff/1/doc/hacking-providers.txt#newc...
doc/hacking-providers.txt:113: and replace every instance of the characters
"skeleton" with your new provider name.
s/characters/word/ ?

https://codereview.appspot.com/14494043/diff/1/provider/skeleton/config.go
File provider/skeleton/config.go (right):

https://codereview.appspot.com/14494043/diff/1/provider/skeleton/config.go#ne...
provider/skeleton/config.go:16: const boilerplateConfig = `skeleton:
All the other providers put this on a new line and use [1:] at the end.
We should keep them consistent.

https://codereview.appspot.com/14494043/diff/1/provider/skeleton/config.go#ne...
provider/skeleton/config.go:19: # this exists to demonstrate how to deal with
sensitive values.
Let's use full sentences here, to fit with the changes made in
https://codereview.appspot.com/14426046/

and four-space indents likewise.

https://codereview.appspot.com/14494043/diff/1/provider/skeleton/config.go#ne...
provider/skeleton/config.go:33: var configFields = schema.Fields{
Please let's have doc comments on all variable and function
definitions, as it's important that people understand the
purpose of each part of the skeleton they're supposed to
be adapting.

https://codereview.appspot.com/14494043/diff/1/provider/skeleton/config.go#ne...
provider/skeleton/config.go:75: // present in validated, and defaults will be
inserted if necessary. If the
s/validated/newAttrs/ ?

https://codereview.appspot.com/14494043/diff/1/provider/skeleton/config.go#ne...
provider/skeleton/config.go:77: // whatever checks you need here, before
continuing.
s/,//

https://codereview.appspot.com/14494043/diff/1/provider/skeleton/config.go#ne...
provider/skeleton/config.go:78: // In particular, if you want to extract (say)
credentials from the user's
s/In particular, /For example /

https://codereview.appspot.com/14494043/diff/1/provider/skeleton/provider.go
File provider/skeleton/provider.go (right):

https://codereview.appspot.com/14494043/diff/1/provider/skeleton/provider.go#...
provider/skeleton/provider.go:21: var _ environs.EnvironProvider =
providerInstance
This isn't necessary - it's checked in the call to RegisterProvider.

https://codereview.appspot.com/14494043/diff/1/provider/skeleton/provider.go#...
provider/skeleton/provider.go:35: // to completely configure an environment,
regardless of the initial state.
This comment reads awkwardly.

https://codereview.appspot.com/14494043/diff/1/provider/skeleton/provider.go#...
provider/skeleton/provider.go:90: "secret %q field must have a string value; got
%v",
%T %v ?

so that we can actually see the offending type