Code review comment for lp:~julian-edwards/maas/api-reserve-user-ip

Thanks for the review!

On Tuesday 01 Jul 2014 11:58:12 you wrote:
> Review: Approve
>
> Looks good.
>
> An alternate design would have been to stick the new endpoint on the
> nodegroupinterface but I guess adding a new top level endpoint
> (/ipaddresses/) is a bit simpler.

I explicitly didn't want to do that for several reasons:
 1. addressing the NGI is problematic
 2. it would obviate the desired mechanism of selecting a network - the CIDR
 3. it is not future proof for querying other types of addresses (as I said in
the cover note!)

>
> As I said on IRC, I'd advice refactoring the reserve() API call to use a
> form. A form is just the natural abstraction when doing any kind of
> validation that is longer than two lines. It has several advantages: - the
> code will be easier to test (the form it only doing the validation so
> testing the different error cases will be much simpler) - the tests which
> only exercise the form will be much faster (because they don't have to deal
> with the HTTP layer) - the form can be reused in the UI

I must admit I thought I wouldn't need a form when I started this, and then
after it was done I had some regrets, and also figured you might say that :)

> You'll get a KeyError if it's not there; that's not what I call proper
> testing :).

I think it's fine, the test will stop with an error and it'll be obvious why.

> > + del returned_address['created']
> > + expected = dict(
>
> You could use testtools.matchers.MatchesStructure here.

Ah yes I had forgotten about that, thanks for the reminder!

J