Code review comment for lp:~gmb/maas/generate-dns-for-dynamic-pool-bug-1382190

Revision history for this message
Raphaƫl Badin (rvb) wrote :

I added a bunch of comments on top of what Gavin already said.

I've tested that this works as expected on my NUCs and testing went great (but I didn't test the corner cases, just that this addresses the main issue); The main thing I suggested was the removal of the 'dynamic' prefix in the GENERATE records to preserve exactly the hostnames we had before in order to avoid upgrade nightmares; thanks for addressing this quickly gmb.

fwiw, I really wonder if this isn't a bit overly complicated. I understand this was done this way to limit the number of GENERATE records as much as possible but the end result is code that will be a bit painful to maintain.

Same as Gavin (in his comment that starts with "There's a *lot* of machinery in here to come up with expected output."), I think that the testing would benefit from using the expected string output when possible; additionally, this will help us be confident that the corner cases are covered.

review: Approve

« Back to merge proposal