Merge lp:~gmb/maas/label-in-pxe-config into lp:~maas-committers/maas/trunk
Proposed by
Graham Binns
Status: | Merged |
---|---|
Approved by: | Graham Binns |
Approved revision: | no longer in the source branch. |
Merged at revision: | 2139 |
Proposed branch: | lp:~gmb/maas/label-in-pxe-config |
Merge into: | lp:~maas-committers/maas/trunk |
Diff against target: |
134 lines (+96/-1) 4 files modified
src/maasserver/api.py (+16/-1) src/maasserver/models/bootimage.py (+8/-0) src/maasserver/models/tests/test_bootimage.py (+59/-0) src/maasserver/tests/test_api_pxeconfig.py (+13/-0) |
To merge this branch: | bzr merge lp:~gmb/maas/label-in-pxe-config |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Jeroen T. Vermeulen (community) | Approve | ||
Review via email: mp+211480@code.launchpad.net |
Commit message
Use the label of the latest available image for (nodegroup, arch, subarch, release, purpose) as the default label in pxeconfig(). Previously we always fell back to 'release' as the default label if one wasn't specified.
Description of the change
This was a fairly simple change but I'm not entirely happy with the if...else at line 15-18 of the diff. I put this in to avoid several pserv tests blowing up, but it now feels slightly lazy... Please tell me if I'm right about that and I'll go and fix up the tests and take that block out.
To post a comment you must log in.
Thanks for filing the bug. In these hectic days it's good to know we're keeping track.
The division of work here strikes me as very sensible.
I think the indentation in the test is missing something: the list comprehension for actual_images does not indent the arguments to get_latest_image.
No test for the case where you return no-such-image?
The structure of the test there is seems like it tries to do too many things, and the result is a bit muddled. I had to read it a few times to figure out your intention. Why not do a bit more to pry apart the behaviours you're trying to establish? “Returns the most recent matching image” is an easier test than what you have here — one purpose, one result. “Does not return image with different purpose” can also be simpler and clearer: create matching image, create newer image that matches except it's for a different purpose, show that the older (but matching) image is returned. And of course it'd be easy to throw in other newer images that differ from the matching one only in arch, subarch, or release — so that the test actually covers selection by the criteria other than purpose.