Code review comment for lp:~jtv/maas/bug-1300059

> [0]
>
> > If the hostname is not valid according to RFCs 952 and 1123.
>
> I know a hostname is a hostname (as in "A Big Mac is a Big Mac") but I think
> this is worth repeating in a place that makes it to the documentation, i.e. in
> ":ivar hostname: This `Node`'s hostname." in
> src/maasserver/models/node.py:Node

OK, I put it in the ivar documentation.

> [1]
>
> 41 + if len(label) > 63:
> 42 + raise ValidationError("Label is too long: '%s'." % label)
>
> The error would be more informative if it mentioned the limit.

Made the message look much like the one for the overall length.

> [2]
>
> 40 + raise ValidationError("Hostname contains empty label.")
> 41 + if len(label) > 63:
> 42 + raise ValidationError("Label is too long: '%s'." % label)
>
> Do you think it's obvious to people what a "label", in this context, means? I
> must admit it was not obvious to me before I read your comments. Maybe "part"?

It's not obvious, no. I changed it to say "name."

> [3]
>
> 44 + raise ValidationError(
> 45 + "Label cannot start or end with hyphen: '%s'." %
> label)
>
> You can use %r here, this saves you from having to include the single quotes
> around the string.

I never liked that, because it introduces magic characters that are neither in the format string nor in the argument. But if you prefer that, I'll change it.

> [4]
>
> 59 + hostname = CharField(
> 60 + max_length=255, default='', blank=True, unique=True,
> 61 + validators=[validate_hostname])
>
> Once more, maybe mention the RFCs in the help_text.

There is no help_text here, or anywhere in our models that I know of. I think that's because we normally have the help text in the form. Wouldn't that one override any help_text from the model? Anyway, the text we have in the form is already quite long and goes into the relationship between the hostname and the FQDN, which I think is much more important and potentially surprising. So I left this as it was.

Thanks for the review.