Code review comment for ~newell-jensen/maas:no-arch-and-mac-address-validation-with-ipmi-power-parameters

Revision history for this message
Lee Trager (ltrager) wrote :

I'm not sure clearing the errors after they occur is the best path. If I add form.save() to your unit test an exception is raised about the architecture missing. I think MAAS should be smart enough to know when to avoid the errors.

On Django forms whether or not a field is required is controlled by the optional field argument required. On the mac_addresses field this is not set so Django is assuming it always is, you can turn it off by modifying WithMACAddressesMixin.set_up_mac_addresses_field with

self.fields['mac_addresses'] = MultipleMACAddressField(len(macs), required=(self.data.get('power_type') != 'ipmi'))

If you look at MachineForm.set_up_architecture_field you'll see required is already False. The error is coming from Node.clean_architecture. That is why clearing the error won't help, the model save will still raise the error. The form should be validating the architecture already so I think its safe to remove Node.clean_architecture and Device.clean_architecture and rely on the form by making required=True unless the power type is ipmi.

review: Needs Fixing

« Back to merge proposal