juju/go: add cloudinit package.

The cloudinit package encapsulates knowledge about the format
of the cloudinit configuration data.

https://code.launchpad.net/~rogpeppe/juju/go-cloudinit/+merge/83584

(do not edit description out of merge proposal)

[niemeyer, 2011-12-07 21:08:13]

http://codereview.appspot.com/5444043/diff/2001/cloudinit/cloudinit.go
File cloudinit/cloudinit.go (right):

http://codereview.appspot.com/5444043/diff/2001/cloudinit/cloudinit.go#newcode2
cloudinit/cloudinit.go:2: // a cloudinit configuration file.
s/cloudinit conf/cloud-init conf/

Our package is cloudinit, but the actual software is referred to more often as
cloud-init.

http://codereview.appspot.com/5444043/diff/2001/cloudinit/cloudinit.go#newcode22
cloudinit/cloudinit.go:22: // Render returns the cloudinit configuration as a
YAML file.
s/cloudinit/cloud-init/

http://codereview.appspot.com/5444043/diff/2001/cloudinit/cloudinit.go#newcode31
cloudinit/cloudinit.go:31: // Option represents a cloudinit configuration
option.
s/cloudinit/cloud-init/

http://codereview.appspot.com/5444043/diff/2001/cloudinit/cloudinit_test.go
File cloudinit/cloudinit_test.go (right):

http://codereview.appspot.com/5444043/diff/2001/cloudinit/cloudinit_test.go#n...
cloudinit/cloudinit_test.go:60: })},
This Go interface for cloud-init is extremely verbose and low-level, to the
point I'd prefer to use a plain map or string rather than using it.

Can we please have something like this instead:

config := cloudinit.New()
config.AddPublicDsaKey("key2data")
config.AddPackages("juju", "ubuntu")
config.SaveAllOutput("/tmp/output.txt")

etc.

Also note that we don't need _all_ of these options upfront. Our use of
cloud-limit is quite limited, and we can improve this package with more as we
go.

[rog, 2011-12-08 16:49:15]

On 7 December 2011 21:08,  <n13m3y3r@gmail.com> wrote:
> Also note that we don't need _all_ of these options upfront. Our use of
> cloud-limit is quite limited, and we can improve this package with more
> as we go.

my idea was to make this package useful for others too.
now that the options are in (i spent some time going through
the cloud-init source code to find them out), isn't
it ok to leave them there?

[niemeyer, 2011-12-08 17:21:12]

> my idea was to make this package useful for others too.
> now that the options are in (i spent some time going through
> the cloud-init source code to find them out), isn't
> it ok to leave them there?

That was just a minor note. I'm certainly not against making the
package more useful. My real discomfort was related to the API
specifically.

[rog, 2011-12-08 18:26:55]

oops, i never published these!

http://codereview.appspot.com/5444043/diff/2001/cloudinit/cloudinit.go
File cloudinit/cloudinit.go (right):

http://codereview.appspot.com/5444043/diff/2001/cloudinit/cloudinit.go#newcode2
cloudinit/cloudinit.go:2: // a cloudinit configuration file.
On 2011/12/07 21:08:13, niemeyer wrote:
> s/cloudinit conf/cloud-init conf/
> 
> Our package is cloudinit, but the actual software is referred to more often as
> cloud-init.

Done.

http://codereview.appspot.com/5444043/diff/2001/cloudinit/cloudinit.go#newcode22
cloudinit/cloudinit.go:22: // Render returns the cloudinit configuration as a
YAML file.
On 2011/12/07 21:08:13, niemeyer wrote:
> s/cloudinit/cloud-init/

Done.

http://codereview.appspot.com/5444043/diff/2001/cloudinit/cloudinit.go#newcode31
cloudinit/cloudinit.go:31: // Option represents a cloudinit configuration
option.
On 2011/12/07 21:08:13, niemeyer wrote:
> s/cloudinit/cloud-init/

Done.

http://codereview.appspot.com/5444043/diff/2001/cloudinit/cloudinit_test.go
File cloudinit/cloudinit_test.go (right):

http://codereview.appspot.com/5444043/diff/2001/cloudinit/cloudinit_test.go#n...
cloudinit/cloudinit_test.go:60: })},
On 2011/12/07 21:08:13, niemeyer wrote:
> This Go interface for cloud-init is extremely verbose and low-level, to the
> point I'd prefer to use a plain map or string rather than using it.
> 
> Can we please have something like this instead:
> 
> config := cloudinit.New()
> config.AddPublicDsaKey("key2data")
> config.AddPackages("juju", "ubuntu")
> config.SaveAllOutput("/tmp/output.txt")
> 
> etc.
> 
> Also note that we don't need _all_ of these options upfront. Our use of
> cloud-limit is quite limited, and we can improve this package with more as we
> go.

Is config.Append(cloudinit.Packages("juju", "ubuntu")) really that much worse
than what you're suggesting. In general, it doesn't seem too low level to me,
and it means that it's possible to do config.Set as well as config.Append (some
providers may wish to override values set by the juju cloud-init code)

For SSH keys, I agree, the interface could be slicker.
How about this:
config.Append(cloudinit.SSHKeysDSA(true, "key2data"))
where the "true" means "private".

In practice I didn't find it too verbose to use; here's an example:
http://paste.ubuntu.com/763814/

[rog, 2011-12-08 18:30:50]

PTAL

[niemeyer, 2011-12-09 15:34:15]

The interface is still following the same underlying principle, and still feels
more verbose than it has to be.

[rog, 2011-12-09 15:43:11]

it's a little more verbose because it's more regular.
the same operators apply to any option. personally,
i think the trade-off worthwhile, and it makes the interface
more powerful.

that said, given that you still disagree, i'll try it the other way.

On 9 December 2011 15:34,  <n13m3y3r@gmail.com> wrote:
> The interface is still following the same underlying principle, and
> still feels more verbose than it has to be.
>
> http://codereview.appspot.com/5444043/

[niemeyer, 2011-12-09 16:03:59]

> it's a little more verbose because it's more regular.
> the same operators apply to any option. personally,
> i think the trade-off worthwhile, and it makes the interface
> more powerful.

There's no tradeoff.. it's simply more verbose. Anything you do with:

    config.AddOption(cloudinit.NewWhatever(a, b, c))

can be done more comfortably with:

    config.AddWhatever(a, b, c)

-- 
Gustavo Niemeyer
http://niemeyer.net
http://niemeyer.net/plus
http://niemeyer.net/twitter
http://niemeyer.net/blog

-- I'm not absolutely sure of anything.

[rog, 2011-12-09 16:11:21]

On 9 December 2011 16:03, Gustavo Niemeyer <n13m3y3r@gmail.com> wrote:
>> it's a little more verbose because it's more regular.
>> the same operators apply to any option. personally,
>> i think the trade-off worthwhile, and it makes the interface
>> more powerful.
>
> There's no tradeoff.. it's simply more verbose. Anything you do with:
>
>    config.AddOption(cloudinit.NewWhatever(a, b, c))
>
> can be done more comfortably with:
>
>    config.AddWhatever(a, b, c)

it means sacrificing the ability to do:

config.SetOption(cloudinit.Whatever(a, b, c))

(i.e. to allow setting all values of attribute rather than appending)
i think i see a way to get around that though, although it's
not as elegant.

it also makes table-driven setting of options harder.

but it's ok, i'm going your way. shouldn't be long now.

[rog, 2011-12-09 17:55:16]

PTAL

it probably needs a few more comments, but i hope it's more to your taste now.

[niemeyer, 2011-12-09 18:05:05]

> it means sacrificing the ability to do:
>
> config.SetOption(cloudinit.Whatever(a, b, c))

This means that besides being verbose the API is weak, which again
means the advantage on top of managing a map is not so great. Boolean
options and options with a single possible value, for instance, should
only have a Set* method, because it doesn't ever make sense to append
them. Options that make sense to append may have a way to reset their
value, if we ever need it. In both cases, the API is informing and
constraining what is possible, justifying its existence.

> but it's ok, i'm going your way. shouldn't be long now.

Thank you.

-- 
Gustavo Niemeyer
http://niemeyer.net
http://niemeyer.net/plus
http://niemeyer.net/twitter
http://niemeyer.net/blog

-- I'm not absolutely sure of anything.

[niemeyer, 2011-12-13 11:58:07]

Much better now, thank you. A few last details:

https://codereview.appspot.com/5444043/diff/18001/cloudinit/cloudinit_test.go
File cloudinit/cloudinit_test.go (right):

https://codereview.appspot.com/5444043/diff/18001/cloudinit/cloudinit_test.go...
cloudinit/cloudinit_test.go:21: name   string
I think the list below would be more readable if the order of these two fields
were inverted.

https://codereview.appspot.com/5444043/diff/18001/cloudinit/cloudinit_test.go...
cloudinit/cloudinit_test.go:45: "DisableRoot", func(cfg
*cloudinit.Config){cfg.SetDisableRoot(false)}},
The entries below are significantly more readable than the ones above. I suspect
it'd be even more readable if the brackets were on a line by themselves.

https://codereview.appspot.com/5444043/diff/18001/cloudinit/options.go
File cloudinit/options.go (right):

https://codereview.appspot.com/5444043/diff/18001/cloudinit/options.go#newcode3
cloudinit/options.go:3: func (cfg *Config) SetUser(x string) {
All of these methods should have comments describing what they do, so that
people don't have to Google for their meaning (and Google doesn't generally help
either!).

https://codereview.appspot.com/5444043/diff/18001/cloudinit/options.go#newcode5
cloudinit/options.go:5: cfg.set("user", x != "", x)
Please use proper parameter names in all methods. This is part of the
documentation.

https://codereview.appspot.com/5444043/diff/18001/cloudinit/options.go#newcode9
cloudinit/options.go:9: // apt_update_upgrade
Please drop all comments in this style. Besides adding no value for someone
reading the code, some of them are actually wrong.

https://codereview.appspot.com/5444043/diff/18001/cloudinit/options.go#newcode13
cloudinit/options.go:13: func (cfg *Config) SetAptUpdate(yes bool) {
These methods are looking a lot cleaner now. Thanks a lot.

https://codereview.appspot.com/5444043/diff/18001/cloudinit/options.go#newcode54
cloudinit/options.go:54: func (cfg *Config) SetDebConfSelections(x bool) {
s/DebConf/Debconf/ (the application, not the conference)

https://codereview.appspot.com/5444043/diff/18001/cloudinit/options.go#newcod...
cloudinit/options.go:111: // If stderr is "&1" or empty, it will be directed to
the same
No need to present "&1" here, IMO. The zero value is a more idiomatic form.

https://codereview.appspot.com/5444043/diff/18001/cloudinit/options.go#newcod...
cloudinit/options.go:113: func (cfg *Config) SetOutput(kind, stdout, stderr
string) {
This looks like a good case for a typed const:

  type OutputKind string

  const (
      AllOut OutputKind = "all"
      InitOut OutputKind = "init"
      ...
  )

https://codereview.appspot.com/5444043/diff/18001/cloudinit/options.go#newcod...
cloudinit/options.go:133: // note that disable_root defaults to true, so we
include
Ouch.. what a poor choice of naming+default.

[rog, 2011-12-13 16:15:15]

https://codereview.appspot.com/5444043/diff/18001/cloudinit/cloudinit_test.go
File cloudinit/cloudinit_test.go (right):

https://codereview.appspot.com/5444043/diff/18001/cloudinit/cloudinit_test.go...
cloudinit/cloudinit_test.go:21: name   string
On 2011/12/13 11:58:07, niemeyer wrote:
> I think the list below would be more readable if the order of these two fields
> were inverted.

Done.

https://codereview.appspot.com/5444043/diff/18001/cloudinit/cloudinit_test.go...
cloudinit/cloudinit_test.go:45: "DisableRoot", func(cfg
*cloudinit.Config){cfg.SetDisableRoot(false)}},
On 2011/12/13 11:58:07, niemeyer wrote:
> The entries below are significantly more readable than the ones above. I
suspect
> it'd be even more readable if the brackets were on a line by themselves.

Done.

https://codereview.appspot.com/5444043/diff/18001/cloudinit/options.go
File cloudinit/options.go (right):

https://codereview.appspot.com/5444043/diff/18001/cloudinit/options.go#newcode3
cloudinit/options.go:3: func (cfg *Config) SetUser(x string) {
On 2011/12/13 11:58:07, niemeyer wrote:
> All of these methods should have comments describing what they do, so that
> people don't have to Google for their meaning (and Google doesn't generally
help
> either!).

i really want to do this, but i'll need some help, because i don't actually know
what most of this stuff does - i just hacked the options from the source code.
there doesn't seem to be any decent cloud-init documentation.

i don't know, for instance, what "user" means without delving deep into the
source, which is going to be very time consuming. "set the cloud-init user name"
doesn't really help anyone very much. perhaps we could submit this CL and add
more comments later?

https://codereview.appspot.com/5444043/diff/18001/cloudinit/options.go#newcode5
cloudinit/options.go:5: cfg.set("user", x != "", x)
On 2011/12/13 11:58:07, niemeyer wrote:
> Please use proper parameter names in all methods. This is part of the
> documentation.

Done.

https://codereview.appspot.com/5444043/diff/18001/cloudinit/options.go#newcode9
cloudinit/options.go:9: // apt_update_upgrade
On 2011/12/13 11:58:07, niemeyer wrote:
> Please drop all comments in this style. Besides adding no value for someone
> reading the code, some of them are actually wrong.

Done.

https://codereview.appspot.com/5444043/diff/18001/cloudinit/options.go#newcode54
cloudinit/options.go:54: func (cfg *Config) SetDebConfSelections(x bool) {
On 2011/12/13 11:58:07, niemeyer wrote:
> s/DebConf/Debconf/ (the application, not the conference)

Done.

https://codereview.appspot.com/5444043/diff/18001/cloudinit/options.go#newcod...
cloudinit/options.go:111: // If stderr is "&1" or empty, it will be directed to
the same
On 2011/12/13 11:58:07, niemeyer wrote:
> No need to present "&1" here, IMO. The zero value is a more idiomatic form.

Done.

https://codereview.appspot.com/5444043/diff/18001/cloudinit/options.go#newcod...
cloudinit/options.go:113: func (cfg *Config) SetOutput(kind, stdout, stderr
string) {
On 2011/12/13 11:58:07, niemeyer wrote:
> This looks like a good case for a typed const:
> 
>   type OutputKind string
> 
>   const (
>       AllOut OutputKind = "all"
>       InitOut OutputKind = "init"
>       ...
>   )

not entirely sure about this (i don't know if it's an exhaustive list) but done
anyway.

[rog, 2011-12-13 16:42:56]

Please take a look.

[niemeyer, 2011-12-13 21:54:14]

It's looking good. Just one problem I noticed now, plus the comments, and it
should be good to go in.

http://codereview.appspot.com/5444043/diff/20003/cloudinit/options.go
File cloudinit/options.go (right):

http://codereview.appspot.com/5444043/diff/20003/cloudinit/options.go#newcode118
cloudinit/options.go:118: cfg.attrs["ssh_keys"] = append(keys, key{alg, private,
keyData})
This seems to be marshalling incorrectly according to the example:

  http://j.mp/cloud-config  

It seems to be single map with four keys rather than a list.

Also, please implement the interface like this so it's more readable and simpler
implement:
 
type SSHKeyType string

const (
    PrivateRSA SSHKeyType = "private_rsa"
    PrivateDSA SSHKeyType = "private_dsa"
    PublicRSA  SSKKeyType = "public_rsa"
    PublicDSA  SSHKeyType = "public_dsa"
)

func (cfg *Config) AddSSHKey(keyType SSHKeyType, keyData []byte) {

[rog, 2011-12-14 12:56:01]

Please take a look.

[rog, 2011-12-14 12:58:21]

Please take a look.

[rog, 2011-12-14 12:59:14]

PTAL

[niemeyer, 2011-12-14 13:57:57]

Another round.

https://codereview.appspot.com/5444043/diff/25007/cloudinit/options.go
File cloudinit/options.go (right):

https://codereview.appspot.com/5444043/diff/25007/cloudinit/options.go#newcode80
cloudinit/options.go:80: // AddRunCommand adds a command to be executed
s/AddRunCommand/AddRunCmd/

https://codereview.appspot.com/5444043/diff/25007/cloudinit/options.go#newcode83
cloudinit/options.go:83: // command will be interpreted by "sh".
Please clarify this with smoser (first/non-first, and what is "the shell
script").

https://codereview.appspot.com/5444043/diff/25007/cloudinit/options.go#newcode96
cloudinit/options.go:96: // only slightly after a 'boothook' would run.
What is a boothook?

https://codereview.appspot.com/5444043/diff/25007/cloudinit/options.go#newcod...
cloudinit/options.go:130: // arguments will be used as entries for an /etc/fstab
This isn't clear. As _a_ mount point, or as _many_ mount points? If it's _a_
mount point, it can't be _entries_ in fstab. Is it a single line in fstab?

https://codereview.appspot.com/5444043/diff/25007/cloudinit/options.go#newcod...
cloudinit/options.go:155: //	pipes to the given command.
This will format incorrectly.

https://codereview.appspot.com/5444043/diff/25007/cloudinit/options.go#newcod...
cloudinit/options.go:169: // AddSSHKey adds a pre-generated ssh private key to
the
s/ssh private key/ssh key/

https://codereview.appspot.com/5444043/diff/25007/cloudinit/options.go#newcod...
cloudinit/options.go:182: func (cfg *Config) SetDisableRoot(yes bool) {
Missing docs.

[rog, 2011-12-14 15:25:20]

Please take a look.

[rog, 2011-12-14 15:25:52]

https://codereview.appspot.com/5444043/diff/25007/cloudinit/options.go
File cloudinit/options.go (right):

https://codereview.appspot.com/5444043/diff/25007/cloudinit/options.go#newcode80
cloudinit/options.go:80: // AddRunCommand adds a command to be executed
On 2011/12/14 13:57:57, niemeyer wrote:
> s/AddRunCommand/AddRunCmd/

Done.

https://codereview.appspot.com/5444043/diff/25007/cloudinit/options.go#newcode83
cloudinit/options.go:83: // command will be interpreted by "sh".
On 2011/12/14 13:57:57, niemeyer wrote:
> Please clarify this with smoser (first/non-first, and what is "the shell
> script").

Done.

https://codereview.appspot.com/5444043/diff/25007/cloudinit/options.go#newcode96
cloudinit/options.go:96: // only slightly after a 'boothook' would run.
On 2011/12/14 13:57:57, niemeyer wrote:
> What is a boothook?

reference to boothook removed.

https://codereview.appspot.com/5444043/diff/25007/cloudinit/options.go#newcod...
cloudinit/options.go:130: // arguments will be used as entries for an /etc/fstab
On 2011/12/14 13:57:57, niemeyer wrote:
> This isn't clear. As _a_ mount point, or as _many_ mount points? If it's _a_
> mount point, it can't be _entries_ in fstab. Is it a single line in fstab?

you're right - i'd just copied that phrasing from the cloud-init doc.

https://codereview.appspot.com/5444043/diff/25007/cloudinit/options.go#newcod...
cloudinit/options.go:155: //	pipes to the given command.
On 2011/12/14 13:57:57, niemeyer wrote:
> This will format incorrectly.

Done.

https://codereview.appspot.com/5444043/diff/25007/cloudinit/options.go#newcod...
cloudinit/options.go:169: // AddSSHKey adds a pre-generated ssh private key to
the
On 2011/12/14 13:57:57, niemeyer wrote:
> s/ssh private key/ssh key/

Done.

https://codereview.appspot.com/5444043/diff/25007/cloudinit/options.go#newcod...
cloudinit/options.go:182: func (cfg *Config) SetDisableRoot(yes bool) {
On 2011/12/14 13:57:57, niemeyer wrote:
> Missing docs.

Done.

[niemeyer, 2011-12-14 16:07:45]

LGTM