Code review comment for lp:~rvb/maas/bug-1070765-hostname2

Thanks for the review!

> [1]
>
> +# Non-ambiguous characters (i.e. without 'ilouvz1250').
>
> "s" too.

Good catch. Fixed.

> [2]
>
> +            try:
> +                self.save()
> +                break
> +            except ValidationError:
> +                pass
>
> There's not much to it here, but I think this is better expressed as:
[...]

You'll admit that there is little chance for 'break' to raise a ValidationError ;). But ok, fixed.

> [3]
>
> +    def test_generate_hostname_does_not_contain_ambiguous_chars(self):
> +        ambiguous_chars = 'ilouvz1250'
>
> "s" here too.

Fixed.

> [4]
[...]
> I think this could be simplified to not use Mock:
>
>        hostnames = [existing_node.hostname, "new_hostname"]
>        self.patch(
>            node_module, "generate_hostname",
>            lambda size: hostnames.pop(0))

Indeed, much simpler. Thanks.
>
>
> [5]
>
> I was concerned that the host name could not start with a number, but
> it seems that it's fine, 10gen.com for example. It might be worth
> mentioning that this has been considered in a comment or docstring.
>
> http://en.wikipedia.org/wiki/Hostname#Restrictions_on_valid_host_names

Done.