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

[1,2] Fixed

[3] Why fail a node from working? What is the downside of using install instead of xinstall, you still end up with a system that is similar after finish installation, its just speed. So if the operating system installs using fast-path great, if not then use the old way. Also since fast-path booting is only set by a tag, then it makes it even harder to catch. As you wouldn't see the error until you go to turn on the node, at which case it has already been acquired and it will fail, causing JuJu to fail.

[4,5] Comments fixed.
[5] You need to grab the purpose first only because the boot label is unknown on the first call. So first it gets the purpose without the label, then once the correct label is select. Just to check the label supports that purpose we call again. Its just a good check to make sure that the final selected boot image can handle this method.

[6] Early out used!
[6] It needs to handle two form inputs as the OS selection is used with NodeForm, and DeployForm. Didn't want to duplicate the code, when it would be the same so I split it out into a method that can handle multiple forms.

[7,8] Fixed

[9] Merge conflict resolved

[10] Its always a good check to make sure that it is not None, so I check. It really would only be a blank string, but doesn't hurt to just make sure. Being explicit.

[11] Split into two tests.

[12] This is testing that the set method is actually still being called. So if the code was to change where it wasn't this would break the API and the test would fail.

[13] This actually uses a different method on Node, release. The release call resets the osystem and distro_series. Also a good check to make sure that the release call is called.

[14] Belief that it makes it easier to see that the test is testing it multiple times.

[15] Differ in one tests that the items are there, and the other tests that the items are sorted. This is a copy of how the architecture code is tested.

[16]
Added test for maasserver.api.get_boot_purpose() on return "install" when os doesn't support "xinstall".

NodeForm.set_up_osystem_and_distro_series_fields() and maasserver.forms.clean_distro_series_field() is being tested in maasserver.tests.test_forms.TestNodeForm by the following:
  - test_contains_limited_set_of_fields
  - test_accepts_osystem
  - test_rejects_invalid_osystem
  - test_starts_with_default_osystem
  - test_accepts_osystem_distro_series
  - test_rejects_invalid_osystem_distro_series
  - test_starts_with_default_distro_series
  - test_rejects_mismatch_osystem_distro_series

maasserver.models.node.validate_osystem() and maasserver.models.node.validate_distro_series() are getting tested with the TestNodeForm and the start API call.

These are getting covered by the usage of the forms, views, and api throughout the testing code base, as you stated.
  - maasserver.forms.list_release_choices()
  - maasserver.forms.list_osystem_choices()
  - NodeForm.clean_distro_series()
  - maasserver.form_settings.list_osystem_choices()
  - maasserver.form_settings.list_release_choices()
  - maasserver.form_settings.list_commissioning_choices()
  - maasserver.models.node.get_all_releases()
  - maasserver.models.node.get_osystem_from_release()