Merge lp:~rvb/maas/maas-basic-model into lp:~maas-committers/maas/trunk

[Raphaël Badin]

Update Node and MACAddress.

This branch updates the model classes: Node and MACAddress and adds model tests.

= Tests =

./bin/test

[Gavin Panella (allenap)]

Looks good. Lots of questions though :)

[1]

+ def save(self):
+ if not self.id:
+ self.created = datetime.date.today()
+ self.updated = datetime.datetime.today()

I assume this needs a sprinkling of pytz? Or does Django mysteriously
DTRT here? I think we should prefer explicit when dealing with dates.

[2]

+def generate_node_system_id():
+ return 'node-' + str(uuid1())

    return 'node-%s' % uuid1()

might be prettier :) No other reason.

[3]

+ def __unicode__(self):
+ return self.system_id

Is this used by Django, or is it for your own information?

Does Django provide a decent repr around this? In other words, it
would be nice to have a decent repr :)

[4]

+mac_re = re.compile(r'^([0-9a-fA-F]{2}(:|$)){6}$')

I'm not sure about the $ in the middle. It will also match
"12:34:56:78:90:12:" even though the max_length setting on mac_address
will catch that.

The following might be clearer:

mac_re = re.compile(r'^([0-9a-fA-F]{2}:){5}[0-9a-fA-F]{2}$')

Or even:

mac_re = re.compile(
   r"^ (?:[0-9a-fA-F]{2}:){5} [0-9a-fA-F]{2} $", re.VERBOSE)

Note also the non-capturing group I added: (?:...)

[5]

+ self.assertTrue(node.system_id.startswith('node-'))

Perhaps this is the time to introduce testtools and use StartsWith?

[6]

Docstrings needed!

[7]

+ mac = MACAddress(mac_address='AA:BB:CCXDD:EE:FF', node=node)
+ self.assertRaises(ValidationError, mac.full_clean)

Wait! What? It doesn't blow up until later?

review: Needs Information

[Raphaël Badin (rvb)]

> [1]
>
> + def save(self):
> + if not self.id:
> + self.created = datetime.date.today()
> + self.updated = datetime.datetime.today()
>
> I assume this needs a sprinkling of pytz? Or does Django mysteriously
> DTRT here? I think we should prefer explicit when dealing with dates.

It does something somewhat right now: it stores the date with the tz. jtv would like all the dates to be stored without a timezone but I'll let him do it :).

> [2]
>
> +def generate_node_system_id():
> + return 'node-' + str(uuid1())
>
> return 'node-%s' % uuid1()
>
> might be prettier :) No other reason.

Sure.

> [3]
>
> + def __unicode__(self):
> + return self.system_id
>
> Is this used by Django, or is it for your own information?

It is used by django when as the default __repr__ for instance when printing an object. It is just convenient to have the system_id.

> Does Django provide a decent repr around this? In other words, it
> would be nice to have a decent repr :)

The default __repr__ is <Node Ox1234>.

> [4]
>
> +mac_re = re.compile(r'^([0-9a-fA-F]{2}(:|$)){6}$')
>
> I'm not sure about the $ in the middle. It will also match
> "12:34:56:78:90:12:" even though the max_length setting on mac_address
> will catch that.
>
> The following might be clearer:
>
> mac_re = re.compile(r'^([0-9a-fA-F]{2}:){5}[0-9a-fA-F]{2}$')
>
> Or even:
>
> mac_re = re.compile(
> r"^ (?:[0-9a-fA-F]{2}:){5} [0-9a-fA-F]{2} $", re.VERBOSE)
>
> Note also the non-capturing group I added: (?:...)

I prefer the former.

> [5]
>
> + self.assertTrue(node.system_id.startswith('node-'))
>
> Perhaps this is the time to introduce testtools and use StartsWith?

ah! I'll skip that one for now.

> [6]
>
> Docstrings needed!

Docstrings added.

>
>
> [7]
>
> + mac = MACAddress(mac_address='AA:BB:CCXDD:EE:FF', node=node)
> + self.assertRaises(ValidationError, mac.full_clean)
>
> Wait! What? It doesn't blow up until later?

That's the way it is, the validation happens when you call full_clean on the model (see https://docs.djangoproject.com/en/dev/ref/models/instances/#validating-objects).

[Gavin Panella (allenap)]

Okay, looks good.

[1]

> It does something somewhat right now: it stores the date with the
> tz.

Yes, but which timezone? It's not specified anywhere.

[3]

>> Does Django provide a decent repr around this? In other words, it
>> would be nice to have a decent repr :)
>
> The default __repr__ is <Node Ox1234>.

Okay, not very exciting. I guess we could introduce our own magical
base class at some point.

[7]

> That's the way it is, the validation happens when you call
> full_clean on the model (see
> https://docs.djangoproject.com/en/dev/ref/models/instances/#validating-objects).

Cool, thanks for the link.

review: Approve

[Raphaël Badin (rvb)]

> Yes, but which timezone? It's not specified anywhere.

Your timezone. Well, what your browser says your timezone is :).

[Gavin Panella (allenap)]

>> Yes, but which timezone? It's not specified anywhere.
>
> Your timezone. Well, what your browser says your timezone is :).

Gulp. That sounds bad, but if Django *guarantees* that it gets it
right every time then maybe it's okay. My instinct is still to store
all dates as UTC.

[Jeroen T. Vermeulen (jtv)]

Why aren't we using the macaddr data type for storing MACs in the database?

[Raphaël Badin (rvb)]

Good point. I've created this model that works and I'll create all the externally exposed methods (like add_mac_address & co) on the Node object in such a way that how we store MACs is a well confined implementation detail. I'll investigate what you suggest (I suspect it only requires wrapping it so that Django can use it) and also consider using an array to store the MAC Addresses.

[Julian Edwards (julian-edwards)]

On Tuesday 17 January 2012 18:05:26 Gavin Panella wrote:
> >> Yes, but which timezone? It's not specified anywhere.
> >
> > Your timezone. Well, what your browser says your timezone is :).
>
> Gulp. That sounds bad, but if Django *guarantees* that it gets it
> right every time then maybe it's okay. My instinct is still to store
> all dates as UTC.

Mine too. The uncertainty here is concerning.

=== modified file 'src/maasserver/models.py'
--- src/maasserver/models.py	2012-01-16 16:25:49 +0000
+++ src/maasserver/models.py	2012-01-17 17:22:24 +0000
@@ -1,6 +1,29 @@
 import datetime
+import re
+from uuid import uuid1
+
 from django.db import models
 from django.contrib import admin
+from django.core.validators import RegexValidator
+
+
+class CommonInfo(models.Model):
+    created = models.DateField(editable=False)
+    updated = models.DateTimeField(editable=False)
+
+    class Meta:
+        abstract = True
+
+    def save(self):
+        if not self.id:
+            self.created = datetime.date.today()
+        self.updated = datetime.datetime.today()
+        super(CommonInfo, self).save()
+
+
+def generate_node_system_id():
+    return 'node-%s' % uuid1()
+
 
 NODE_STATUS_CHOICES = (
     (u'NEW', u'New'),
@@ -11,17 +34,35 @@
 )
 
 
-class Node(models.Model):
-    name = models.CharField(max_length=30)
+class Node(CommonInfo):
+    system_id = models.CharField(
+        max_length=41, unique=True, editable=False,
+        default=generate_node_system_id)
+    hostname = models.CharField(max_length=255)
     status = models.CharField(max_length=10, choices=NODE_STATUS_CHOICES)
-    created = models.DateField(editable=False)
-    updated = models.DateTimeField(editable=False)
-
-    def save(self):
-        if not self.id:
-            self.created = datetime.date.today()
-        self.updated = datetime.datetime.today()
-        super(Node, self).save()
-
-
+
+    def __unicode__(self):
+        return self.system_id
+
+
+mac_re = re.compile(r'^([0-9a-fA-F]{2}:){5}[0-9a-fA-F]{2}$')
+
+
+validate_mac_address = RegexValidator(
+    regex = mac_re,
+    message = u'Enter a valid MAC address (e.g. AA:BB:CC:DD:EE:FF).')
+
+
+class MACAddress(CommonInfo):
+    mac_address = models.CharField(
+        max_length=17, validators=[validate_mac_address])
+    node = models.ForeignKey(Node)
+
+    def __unicode__(self):
+        return self.mac_address
+
+
+# Register the models in the admin site.
 admin.site.register(Node)
+admin.site.register(MACAddress)
+

=== added file 'src/maasserver/tests/test_models.py'
--- src/maasserver/tests/test_models.py	1970-01-01 00:00:00 +0000
+++ src/maasserver/tests/test_models.py	2012-01-17 17:22:24 +0000
@@ -0,0 +1,42 @@
+"""
+Test maasserver models.
+"""
+
+from django.test import TestCase
+from maasserver.models import Node, MACAddress
+from django.core.exceptions import ValidationError
+
+
+class NodeTest(TestCase):
+
+    def test_system_id(self):
+        """
+        The generated system_id looks good.
+
+        """
+        node = Node()
+        self.assertEqual(len(node.system_id), 41)
+        self.assertTrue(node.system_id.startswith('node-'))
+
+
+class MACAddressTest(TestCase):
+
+    def test_mac_address_invalid(self):
+        """
+        An invalid MAC address does not pass the model validation phase.
+
+        """
+        node = Node()
+        node.save()
+        mac = MACAddress(mac_address='AA:BB:CCXDD:EE:FF', node=node)
+        self.assertRaises(ValidationError, mac.full_clean)
+
+    def test_mac_address_valid(self):
+        """
+        A valid MAC address passes the model validation phase.
+
+        """
+        node = Node()
+        node.save()
+        mac = MACAddress(mac_address='AA:BB:CC:DD:EE:FF', node=node)
+        mac.full_clean()  # No exception.