Code review comment for lp:~axwalk/juju-core/lp1225825-netlookupip-ip

Revision history for this message
Tim Penhey (thumper) wrote :

LGTM with a test added, also consider the logging severity.

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

https://codereview.appspot.com/13504044/diff/1/environs/manual/provisioner.go#newcode80
environs/manual/provisioner.go:80: logger.Warningf("failed to resolve
%v: %v", ip, err)
Is it really a warning if we can't do a reverse lookup? Is it something
that we should be fixing later?

Perhaps just log with info or debug, certainly not really a big deal is
it?

https://codereview.appspot.com/13504044/diff/1/instance/address.go
File instance/address.go (right):

https://codereview.appspot.com/13504044/diff/1/instance/address.go#newcode64
instance/address.go:64: func HostAddresses(host string) (addrs
[]Address, err error) {
How about a test for this?

https://codereview.appspot.com/13504044/

« Back to merge proposal