Simpler startInstance & friends?

[Ian Booth]

The provider code is at the point where in can indeed do with refactoring. The
situation has evolved such that the EC2 provider was written first, and then
much later the Openstack provider was written separately afterwards. We borrowed
much of the EC2 implementation and ended up (for expediency) using cut and paste
in various places. The two main reasons were that we wanted to keep our code
initially separate and not disturb too much the existing code which we were just
coming to understand, the EC2 code worked and we weren't sure enough of how
things hung together to be confident of too many changes, and the decomposition
of the existing environs interface meant that things needed to be done a certain
way.

Now having two working providers, it's much clearer where the various clean ups
and refactorings need to be. And adding a 3rd and 4th provider will provide more
clarity. So *my* view is that nothing that's there currently, especially
unexported business logic methods like startInstance(), is sacred and that if
good software engineering principals suggest changes are required, then go for
it. Having a requirement for 2 reviewers will vet any errors/omissions etc. Just
ensure that any changes make sense across all providers and any optimisations
are implemented in each provider if appropriate.
All IMHO. YMMV.

On 12/03/13 14:17, Jeroen Vermeulen wrote:
> Hi all,
> 
> Raphaël Badin and I have been looking at the startInstance() and
> userData() code in the ec2 and openstack providers.  It was pretty hard
> to figure out what goes on.  But having had to do that anyway, we think
> we have found a way to simplify it and make it more manageable.  I'm
> bringing it up here because somebody may find it useful.
> 
> The only call sites we are aware of for startInstance are Bootstrap()
> and StartInstance().  It looks to us as if this code could be simpler
> with 3 changes:
> 
> 1. The caller passes "tools" to startInstance(), but if tools is nil,
> startInstance() looks for tools.  This can only happen if the caller is
> StartInstance().  Moving the check in there enables other changes below.
> 
> 2. startInstance() can take a user-data blob as a parameter.  The caller
> first creates it by calling userData().
> 
> This takes away a lot of the information going into startInstance().
> The big differences between the two call sites are now at the call site,
> instead of inside startInstance().
> 
> 3. The startInstanceParams struct type is effectively a wrapper for
> cloudinit.MachineConfig.  Once it's no longer shared between
> startInstance() and userData(), it becomes easier just to use MachineConfig.
> 
> I don't know if anybody would be interested in trying this, but since we
> had to figure out the code anyway, we might as well share.
> 
> 
> Jeroen
>