Code review comment for lp:~allenap/maas/remove-ip-from-amt

Revision history for this message
Julian Edwards (julian-edwards) wrote :

Mostly fine but a question and a nit! Setting to approved though as I don't want to block you.

[1]
49 + if 'mac_address' in kwargs:
50 + kwargs['ip_address'] = find_ip_via_arp(kwargs['mac_address'])

If ip_address is not set, then the template will blow up. Why would mac_address not be set in kwargs here?

[2]
You're using a bit of a testing antipattern by initialising sentinel.ip_address in the setup and then depending on it in the test assertions. I think it would be better if each test patched out this separately, despite the lack of factoring.

BTW I think we should use more sentinel, it's much easier on the eye that the factory.makeRandomXXX stuff!

review: Approve

« Back to merge proposal