Code review comment for lp:~roadmr/checkbox/1260507-config-files

Revision history for this message
Jeff Lane  (bladernr) wrote :

Heh... so...
1: If I read that right, now the configs for virtualization and network will live in /etc/xdg/plainbox.conf. Did you add the default configs there? (I don't see it in this diff)
2: Personally, I think the separate files is a bit more intuitive as it says (Hey Tester, edit these files here for network and virtualization tests)
3: /etc/xdg/plainbox.conf is harder to find than /etc/checkbox.d was but that's a minor quibble
4: What's the order of precedence for environment variables now? they don't override command line, yes, but do pre-set env vars override the ones read from plainbox.conf? (e.g. if I have TARGET set already via the shell, will running plainbox overwrite TARGET with whatever is in plainbox.conf?)
5: has this been tested with the contents of the network.cfg and virtualization.cfg included in plainbox.conf?
6: the pep8 fixes made the diff rather hard to read, it's hard to tell what is related to this fix and what is pep8 adding spaces or doing something that doesn't really affect the script.

« Back to merge proposal