<bigjools> I am trying to be constructive on the review but you're going to hate me, sorry :( <roaksoax> hehe :)
Did you guys have any pre-implementation chat with one of us before starting this? I can't approve this as it stands for quite a few fundamental architectural reasons, and also it's an 1100+ line diff without a single test change. I did previously say I won't accept any more branches that have untested code, especially untested walls of shell :( More below. 25 === added file 'contrib/preseeds_v2/preseed_xinstall' 26 --- contrib/preseeds_v2/preseed_xinstall 1970-01-01 00:00:00 +0000 27 +++ contrib/preseeds_v2/preseed_xinstall 2013-04-17 16:47:25 +0000 [snip] 36 +preseed = subprocess.check_output(["/usr/share/maas/xinstall/make-userdata-maas", 37 + "--apt-proxy="+proxy, 38 + "--dpkg-selections="+preseed_data, 39 + "--finished-url="+node_disable_pxe_url, 40 + "--finished-url-data="+node_disable_pxe_data, 41 + "http://"+cluster_host+"/MAAS/static/images/"+node.architecture+"/"+node.distro_series+"/xinstall/root.tar.gz";, 42 + "/dev/sda"]) 43 +}} 44 +{{preseed}} This scares the bejesus out of me. This is just a workaround so that the shell script make-userdata-maas can be used instead of doing it properly in Python, and tested. It has completely sidestepped all of the code that exists 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. At the very least, this template should not be calling out to the shell script. If you can't re-write it in Python and test it in the time available, it should be called from the existing Python code that generates preseeds and returned in preseed_data for the template. It can then be tested so we know the right preseed is getting returned for FPI. 1045 === modified file 'src/maasserver/api.py' 1061 === modified file 'src/maasserver/models/nodegroup.py' 1077 === modified file 'src/maasserver/preseed.py' 1096 === modified file 'src/provisioningserver/kernel_opts.py' 1135 === modified file 'src/provisioningserver/pxe/install_image.py' The changes in all these files need tests. I expect some tests are broken as a result but I can't run the test suite at the moment as I get this: $ make build bin/buildout install database Traceback (most recent call last): File "bin/buildout", line 5, in <module> from pkg_resources import load_entry_point File "/home/ed/canonical/maas/sandbox/local/lib/python2.7/site-packages/distribute-0.6.24-py2.7.egg/pkg_resources.py", line 16, in <module> import sys, os, zipimport, time, re, imp, types File "/home/ed/canonical/maas/sandbox/lib/python2.7/re.py", line 105, in <module> import sre_compile File "/home/ed/canonical/maas/sandbox/lib/python2.7/sre_compile.py", line 14, in <module> import sre_parse File "/home/ed/canonical/maas/sandbox/lib/python2.7/sre_parse.py", line 17, in <module> from sre_constants import * File "/home/ed/canonical/maas/sandbox/lib/python2.7/sre_constants.py", line 18, in <module> from _sre import MAXREPEAT ImportError: cannot import name MAXREPEAT 1109 === added file 'src/provisioningserver/pxe/config.xinstall.template' 1110 --- src/provisioningserver/pxe/config.xinstall.template 1970-01-01 00:00:00 +0000 1111 +++ src/provisioningserver/pxe/config.xinstall.template 2013-04-17 16:47:25 +0000 [snip] 1119 +LABEL amd64 1120 + SAY Booting (amd64) under MAAS direction... [snip] 1127 +LABEL i386 1128 + SAY Booting (i386) under MAAS direction... Don't bother with both SAY commands, they get parsed and executed before choosing the LABEL so both get printed. You could fix the existing templates too. I have not reviewed the changes in the contrib/ directory, shell is not my expertise. Also, no tests :( 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. J -- 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. _______________________________________________ Mailing list: https://launchpad.net/~launchpad-reviewers Post to : [email protected] Unsubscribe : https://launchpad.net/~launchpad-reviewers More help : https://help.launchpad.net/ListHelp
