Code review comment for lp:~gmb/maas-test/import-images

Revision history for this message
Raphaël Badin (rvb) wrote :

Looks nice, but I've got a question, see [1].

[0]

32 === modified file 'maastest/maasfixture.py'
33 --- maastest/maasfixture.py 2013-11-15 14:14:29 +0000
34 +++ maastest/maasfixture.py 2013-11-18 09:48:39 +0000
[...]
39 - def import_maas_images(self, admin_user, api_key):
40 + def import_maas_images(self):

It's really fine to have 'RELEASES="precise" and 'ARCHES="amd64/generic i386/generic" hardcoded from now but it would be nice to have them hardcoded in maastest/main.py and passed to MAASFixture() (same as the parameters 'series' and 'architecture' used by KVMFixture). This way, when we will make them configurable, the change will be minimal. This is really just a detail but might be worth doing in a follow-up branch; this way, all the parameters (even those that are hardcoded for now) will be available at the highest level (i.e. in maastest/main.py) and we will have a nice list of all the config values in one place instead of hardcoded values scattered throughout the code.

[1]

50 + # Update the import_pxe_files config. The reason that we do this
51 + # rather than just `export`ing RELEASES and ARCHES is that the
52 + # latter doesn't seem to work stick across SSH sessions. I don't
53 + # know whether that's because # we're doing it over SSH and not
54 + # using shell=True on the Popen calls in run_command, but this,
55 + # on the other hand, definitely does work.

I'm a bit surprised by that, since we define "DEBIAN_FRONTEND=noninteractive" when we install packages in the VM (see maastest/maasfixture.py). Env variables are not carried over by sudo by default but how about defining the variables *inside* the command executed by sudo… something like:
sudo RELEASES="precise" ARCHES="amd64/generic i386/generic" maas-import-pxe-files
?

review: Needs Information

« Back to merge proposal