launchpad-reviewers team mailing list archive

[Julian Edwards <julian.edwards@xxxxxxxxxxxxx>]

Hi Scott,

On 19/04/13 03:09, Scott Moser wrote:
> "doing it properly in python" is a general problem that maas has.   Maas knows too much.  The installer (xinstall) is not maas.  The proper way to do this is to not have the xinstall code in maas at all, and have maas know an interface to it.  We've pushed xinstall here as a temporary landing location.

I don't think maas itself knows too much - all the installer specific
stuff is in templates, exactly where it should be.  However I would love
for the xinstall code to not be in MAAS too.  Fortunately there's an
easy solution, we just use the template feature that MAAS already has.

> What you have above is a sane way to interface between maas an an installer.

With respect, in no way do I think that is sane and you'll get the same
response from the rest of the guys in my team.  I've explained already
where it's faults are and how to do it better, so I won't do so again.

> We've explicitly put this is a preseed as these templates are sold as user-configurable.  Some configuration may need to be done by an end user, and this is how they do that.  Making maas invoke that code internally means a user has to change maas python code to make a change.

Right now with this branch you have a situation like this:
- Maas renders a preseed template, passing in some data to it
- The template shells out to a script, passing it the data, that
generates another template

This can be vastly simplified as:
 - Maas renders a template, passing in some data to it

where the template is the one that the script previously generated.

The same configuration and flexibility is still there, without the
intermediate step.  The template system we chose is very powerful.

> 
>> already in src/maasserver/preseed.py and src/maasserver/compose_preseed.py
>> that calculates preseed_data based on the node state.
>>
>> It is extremely fragile and it is likely to break, horribly, in many ways, as
>> soon as someone changes anything elsewhere.
> 
> We have 2 components.  yes, one component can break another component.  A change to python can break maas.  A change to glibc can break python. a change to the kernel can break glibc.  Thats *good* design.

I think that's a strawman.  It's not good to have things break.  You're
also falsely assuming the likelihood of Python/glibc/kernel breaking is
of the same likelihood as a change here.  This change is fragile and it
will break easily.

> Its an installer. It should be separate.  Please just review this as if that code was not there.  We'll replace or move it at a later date.

The fact that it is here and you know it should be separate is enough of
a warning that this approach is wrong.

>> I realise that you need to get *something* in raring before FF tomorrow.
>> Last-minute rushed changes are never good, but if you can at least add tests
>> and make the change to the preseed template as I suggested then this could
>> land with a view to improving more later.
> 
> I really prefer to use user-editable and modifable template files than maas python files for the interface between 'xinstall' and maas.

I was never suggesting modifiable python files.  The existing preseed
setup is totally geared around customisable templates that will do the
job.  If there is any aspect of that code that doesn't do what you want
then we can fix it, but we will always insist that you never have to
customise Python and that the templates contain all the correct logic.
The Python is just there to organise and render the correct template.

I'd still like to know why we have to have xinstall in the code base (in
particular the maas-make-preseed exe) rather than a template.  Andres
mentioned that it was so that user-supplied post/pre-inst scripts can be
added later, but that is also trivial to add to a template.

Thanks.


-- 
https://code.launchpad.net/~andreserl/maas/trunk-fpi/+merge/152039
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~andreserl/maas/trunk-fpi into lp:maas.