Code review comment for lp:~mpontillo/maas/update-host-maps-cleanup-bug-1440102

Revision history for this message
Mike Pontillo (mpontillo) wrote :

I'm putting this up for review even though there is a little more cleanup to be done, so people can get started looking at it. All the test cases are passing in this iteration, though I'll be doing some manual testing and running it through CI as well, to ensure it's okay.

This change consolidates the logic for updating host maps and DNS zones within the claim_static_ips() and set_static_ip() methods inside macaddress.py. (Sounds easy? Not really, because lots of tests depend on using a mock to forego updating DNS and/or host maps, depending on their particular use case.)

I'm not completely happy with this change, mainly because moving around dozens of mocks makes it extremely hard to refactor this code.

What I'd like to do, given the time, is to change all callers of update_host_maps(), update_dns_zones() (and any related functions) to call via a module, instead of importing a function. This way when we mock the calls, we already know what to mock, and we can refactor as much as we like without impacting the dozens of tests that care about it.

Also on the TODO list is changing the signature of the update_nodegroup_host_maps() function, which is currently an awkward (nodegroup, claims, nodegroups=None). (I extracted the method in a hurry.)

« Back to merge proposal