Code review comment for lp:~axwalk/juju-core/lp1300264-manual-unresolvable-address

Revision history for this message
Ian Booth (wallyworld) wrote :

LGTM but please consider the suggestion to improve the test

https://codereview.appspot.com/82930044/diff/1/environs/manual/addresses_test.go
File environs/manual/addresses_test.go (right):

https://codereview.appspot.com/82930044/diff/1/environs/manual/addresses_test.go#newcode22
environs/manual/addresses_test.go:22: func (s *addressesSuite)
TestHostAddress(c *gc.C) {
I'd prefer this test split up into different tests for IP address vs
hostname etc. Also, it would be good if the mocked NetLookupHost
function would error based on the input arg eg if host were "valid.host"
it would resolve, if it were "invalid.host", it would raise an error. I
think that's cleaner than the approach taken and removes the extra
variables.

https://codereview.appspot.com/82930044/diff/1/environs/manual/provisioner.go
File environs/manual/provisioner.go (right):

https://codereview.appspot.com/82930044/diff/1/environs/manual/provisioner.go#newcode236
environs/manual/provisioner.go:236: addrs = append(addrs, addr)
Did we want any logging here?

https://codereview.appspot.com/82930044/

« Back to merge proposal