Code review comment for lp:~gmb/maas/label-in-pxe-config

Revision history for this message
Jeroen T. Vermeulen (jtv) wrote :

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.

review: Approve

« Back to merge proposal