Constructor for cloudinit.MachineConfig.

Constructor for cloudinit.MachineConfig.

This is preparation for a refactoring discussed with William Reade and
outlined in bug 1199847.  Much of the work of creating a
cloudinit.MachineConfig is repetitive between providers, so we might as well
extract it.  This also makes it easier to move the construction from one
function to another, as is likely to happen during the further refactoring.

You'll see that "tools" is not a parameter.  That's because the projected
refactoring moves construction of the MachineConfig ahead of where the tools
are selected.  If these two steps end up fitting together, uniformly across
providers, once the code starts to stabilize again then we can make "tools" a
parameter after all.

https://code.launchpad.net/~jtv/juju-core/newmachineconfig/+merge/174542

(do not edit description out of merge proposal)

[jtv.canonical, 2013-07-13 11:31:51]

Please take a look.

[jameinel, 2013-07-14 07:56:30]

I'm a bit torn.

I like the idea of centralizing stuff that can benefit from it.

However, because Go lacks named parameters, I feel that struct initialization
for things that are only data (no methods) is actually really nice.

I was considering if we could put a params struct to pass into the function, but
I realized we already have a nice params syntax. :)

I won't block this, but I definitely feel it is at best a wash. (slight loss of
readability in exchange for a slight centralization, though the struct we are
creating is already centralized.)

https://codereview.appspot.com/11234043/diff/1/environs/openstack/provider.go
File environs/openstack/provider.go (right):

https://codereview.appspot.com/11234043/diff/1/environs/openstack/provider.go...
environs/openstack/provider.go:638: mcfg.Tools = tools
I honestly prefer the syntax of the Struct more than the syntax of the function
version here.
Mostly because structs let you do named parameters.

It isn't as important here, but in other places you pass nil for a couple
parameters, and 'nil, nil)' isn't very clear what you are passing.

So while I like centrally locating things, I don't quite understand why we need
the NewMachineConfig function, rather than just:

mcfg := &cloudinit.MachineConfig{
...
}


Does this end up conflicting with your /var/lib/juju patch?

[jtv.canonical, 2013-07-15 01:47:20]

I should have gone into more detail about the refactoring that this change helps
prepare for.

The plan involves getting rid of startInstanceParams.  Most or all of the
information will be passed as part of a MachineConfig.  (Not a permanently ideal
solution, but as I believe William put it in our call about this, a step in the
sane direction.)  For that, we need (1) increased freedom to move the
initialization for that struct around, and (2) a clear view of what is identical
and what is different between providers — because the plan also involves
unifying lots more identical code.

Hopefully this branch makes a bit more sense in the perspective of that plan. 
Copying struct initialisations around makes it easier to miss that one crucial
little difference between what two providers put in the struct.

And yes, one of the problems with Go is that you need Parameter Object to get
nice parameter-passing, and then it helps to have a constructor for the
Parameter Object so that you get convenient positional parameters, but it's not
always easy to draw the line between which parameters it should take care of and
which you do through the struct, and before you know it you find yourself
wanting a Parameter Object for the constructor!

[jtv.canonical, 2013-07-15 05:10:29]

Please take a look.

[fwereade, 2013-07-15 11:17:49]

I agree with jam that this particular step is more or less a wash, but I'd
rather see this little step go in to prepare the ground for the next ones than
to roll them all in together -- one of the biiig problems with environs
refectoring is how easy it is for them to run out of control and end up way past
2000 lines of diff. LGTM, thanks.

[julian.edwards, 2013-07-16 03:38:25]

LGTM