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 just reviewed src/maasserver/models/node.py as well (again using inline comments) and I hit one sticking point: I don't think validation of a node's operating system and release against the database belongs on the model. The model can validate for basic sanity of the input string, but I feel the checks for supported systems/releases belongs at the form level.

If these were proper foreign keys then yes, clearly it's a model integrity issue. But then we'd also have model-level protection against osystems being deleted while nodes still refer to them. We would have the full stack of referential-integrity mechanisms to do the work for us, efficiently and reliably. But where possible I would avoid model validation that needs to query the database.

As it is, don't these validators break tests? I would expect factory.make_node to set a random operating system and release, and unless you surprised the caller by also creating boot images (probably breaking other tests), the validators would wreak havoc on the test suite.

« Back to merge proposal