Merge lp:~jtv/maas/import-from-configured-mirror into lp:~maas-committers/maas/trunk
Status: | Merged |
---|---|
Approved by: | Jeroen T. Vermeulen |
Approved revision: | no longer in the source branch. |
Merged at revision: | 1339 |
Proposed branch: | lp:~jtv/maas/import-from-configured-mirror |
Merge into: | lp:~maas-committers/maas/trunk |
Diff against target: |
231 lines (+90/-11) 8 files modified
etc/maas/import_ephemerals (+8/-1) scripts/maas-import-ephemerals (+8/-5) scripts/maas-import-pxe-files (+2/-1) src/maasserver/models/nodegroup.py (+10/-1) src/maasserver/tests/test_nodegroup.py (+26/-0) src/maasserver/tests/test_views_settings.py (+5/-2) src/provisioningserver/tasks.py (+8/-1) src/provisioningserver/tests/test_tasks.py (+23/-0) |
To merge this branch: | bzr merge lp:~jtv/maas/import-from-configured-mirror |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Julian Edwards (community) | Needs Fixing | ||
Raphaël Badin (community) | Approve | ||
Review via email: mp+133505@code.launchpad.net |
Commit message
Tell the import scripts, when run as a celery task, what archives to use.
Description of the change
This ties in with Raphaël's ongoing work: he's changing the UI to manage these same settings. I cribbed the settings' names from lp:~rvb/maas/mirror-settings-re-2
Archive locations are passed to the script through environment variables. The scripts already obeys these variables. The impact on the scripts themselves is altogether quite minimal, but one name in maas-import-
URLs that have not been configured yet fall back on the defaults that are encoded in the import scripts. Missing archive locations are not passed through.
Tests pass for this branch even though it does not incorporate Raphaël's branches, which haven't landed yet. That's because my tests configure the archive settings themselves. In a separate branch, which relies on the synthesis of this branch and Raphaël's, I will add an integration test to establish that the two sets of changes operate on the same configuration items.
My test code repeats make_archive_url. It might be worth abstracting a single make_url somewhere that's available to all apps. But then the sensible step would be to sanitize all code that constructs URLs. A job best left to a separate branch!
Jeroen
Looks good!
[0]
96 + config = { objects. get_config( name)
97 + name: Config.
98 + for name in config_parameters}
99 + task_kwargs = {
100 + name: value
101 + for name, value in config.items()
102 + if value is not None}
I don't see why we need to filter out None values…? Also, there is probably no need for a second dict comprehension.
[1]
233 + archive_settings = { endswith( '_ARCHIVE' )}
234 + variable: value
235 + for variable, value in env.iteritems()
236 + if variable.
and
99 + task_kwargs = {
100 + name: value
101 + for name, value in config.items()
102 + if value is not None}
A word about style here. You may disagree but I'd be in favor of formatting that kind of statement this way, mainly because I think it's more readable
task_kwargs = {
name: value
for name, value in config.items()
if value is not None
}
With your version I get the feeling that the "if value is not None" statement is inside the loop, which isn't true.
[2]
144 + archive_options = { '_archive' )}
145 + arg: value
146 + for arg, value in kwargs.items()
147 + if arg.endswith(
How about (and this is not only about style ;))
archive_options = {
arg: value
for arg, value in kwargs.items()
if arg in archives
}
[3]
> My test code repeats make_archive_url. It might be worth abstracting a single make_url somewhere that's available
> to all apps. But then the sensible step would be to sanitize all code that constructs URLs. A job best left to a
> separate branch!
My code in lp:~rvb/maas/mirror-settings-re-2 could benefit from that too!