Code review comment for lp:~julian-edwards/maas/enlist-nodegroup-bug-1274926

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

Sensible change. I assume autodetection itself is already tested, in a test that needed no change. One thing that's a bit awkward (and I do see the XXX about it) is that the semantics of the autodetect_nodegroup parameter are only explained in the test. I think the representation of booleans is not very highly standardised, so it may be worth a note in the docstring even if the docstring does not cover all parameters.

For the error message in api.py, saying that the nodegroup parameter is missing may be too strong. There's a choice to be made. How about "one of nodegroup or autodetect_nodegroup must be specified"?

review: Approve

« Back to merge proposal