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

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

Looks good. As discussed on IRC, it means people with existing nodes with invalid labels will need to rename their nodes before they can do any modifications to them but:
a) the problem will be quite easy to identify (the validation error will be pretty clear)
b) I don't expect many people will have to face this problem

[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

[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.

[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"?

[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.

[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.

review: Approve

« Back to merge proposal