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

Revision history for this message
Blake Rouse (blake-rouse) wrote :

> My general take on how validation should be performed is that we should use

> The result is not quite the same, no. Doing this kind of application-level
> integrity enforcement at the model level reduces our ability to deal
> gracefully with inconsistencies. Consider for instance the consequence of
> having an OS removed: one would be incapable of modifying the nodes
> referencing this OS unless he also changes the OS (because the validation
> would fail at the model level); if the validation happens at the form level,
> at least we could write a migration to deal with these nodes.

This makes since, it would fail to save the node if an os was removed. I will refactor the code to remove the validation from the model.

> But then all your tests operate with a very special case (osystem=blank).
> It's much better to have the tests operate on data that is as similar to
> production data as possible. This is also why we chose to use randomness when
> creating test objects.

That is not a special case, it is the default case for all created nodes. Even nodes created by the user, use that value. Its the same implementation that was there for distro_series. If there was no issue the way it was before, why is there now?

« Back to merge proposal