Code review comment for lp:~blake-rouse/maas/add-osystem-to-node-form-api

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

I'm supposed to be reviewing the review here, I think, but things got so big that I just went back to the code! Anyway, thanks for doing this and for bearing with us when we made you jump through the additional hoops...

Notes about superficialities follow.

.

Doesn't that last line in list_commisioning_choices just return "options" as it already was? If you're trying to force eager evaluation, you could just return list(options).

.

In test_get_osystem_returns_default_osystem, you create a Node and assume that it won't have its osystem set. I think it would make sense to set a random one by default, in which case it would make more sense to set it to '' explicitly. (Yes, Django's handling of blank strings can be infuriating.) Same for test_get_distro_series_returns_default_series just below that.

.

In make_osystem_with_releases, and make_usable_osystem below that, I guess the “purpose” parameter should really be called “purposes” (plural).

.

Nice tests for list_all_usable_osystems, list_all_usable_releases, and the new form validation! Very focused, clearly named, distinct setup / execution / verification phases.

.

It'd be nice to rename _all_ mention of “series” to “releases”... maybe someday.

review: Approve

« Back to merge proposal