launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #08866
[Merge] lp:~jtv/maas/migrate-node into lp:maas
Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/migrate-node into lp:maas.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~jtv/maas/migrate-node/+merge/110455
As per the migration plan. But, this is the odd one out: to minimize spurious painful conflicts from code being removed in one place and identical code being added in another — what the plan calls “airstrikes” — I renamed src/maasserver/models/__init__.py to src/maasserver/models/node.py and then moved everything that doesn't belong there into a new src/maasserver/models/__init__.py. That way most of the code in Node and NodeManager, which is some of our most intense code maintenance-wise, sees only minimal changes.
Since this is a long diff, let me run you through the changes and especially the non-changes that you don't need to pore over in detail!
* now() was only used in one non-test module: timestampedmodel. Yes it _could_ be used more generically but we always assumed there are probably more performance-effective alternatives out there. For now I just encapsulated it in with TimestampedModel. It's not exported. (In flagrant violation of my no-other-changes policy for these branches, I added a docstring.)
* get_db_state() is now in maasserver.utils. Arguably this belongs in a separate maasserver.models.utils, but I didn't feel that would carry its weight right now.
* Some imports moved into methods, from the module level. This is as per the migration plan. I may have done more of it than is strictly needed right now, but circular imports are costly to diagnose and resolve so prevention is valuable.
* All the other boilerplate from __init__.py is just moved, but not changed. There are no changes in MAASAuthorizationBackend, for instance.
* There's no need to import NODE_TRANSITIONS into, and then re-export from, __init__.py so I kept it in node.py. Code that imports it must import it from maasserver.models.node.
* Pretty much everything else is imports. Boring, but probably easily reviewed!
Jeroen
--
https://code.launchpad.net/~jtv/maas/migrate-node/+merge/110455
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/maas/migrate-node into lp:maas.
=== added file 'src/maasserver/models/__init__.py'
--- src/maasserver/models/__init__.py 1970-01-01 00:00:00 +0000
+++ src/maasserver/models/__init__.py 2012-06-15 05:19:22 +0000
@@ -0,0 +1,106 @@
+# Copyright 2012 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Model helpers and state for maasserver."""
+
+from __future__ import (
+ absolute_import,
+ print_function,
+ unicode_literals,
+ )
+
+__metaclass__ = type
+__all__ = [
+ 'Config',
+ 'FileStorage',
+ 'logger',
+ 'MACAddress',
+ 'Node',
+ 'NodeGroup',
+ 'SSHKey',
+ 'UserProfile',
+ ]
+
+from logging import getLogger
+
+from django.contrib import admin
+from django.contrib.auth.backends import ModelBackend
+from django.contrib.auth.models import User
+from django.db.models.signals import post_save
+from maasserver.enum import NODE_PERMISSION
+from maasserver.models.config import Config
+from maasserver.models.filestorage import FileStorage
+from maasserver.models.macaddress import MACAddress
+from maasserver.models.node import Node
+from maasserver.models.nodegroup import NodeGroup
+from maasserver.models.sshkey import SSHKey
+from maasserver.models.user import create_user
+from maasserver.models.userprofile import UserProfile
+from maasserver.utils import ignore_unused
+from piston.models import Consumer
+
+
+logger = getLogger('maasserver')
+
+
+# Suppress warning about symbols being imported, but only used for
+# export in __all__.
+ignore_unused(Config, FileStorage, MACAddress, NodeGroup, SSHKey, UserProfile)
+
+
+# Connect the 'create_user' method to the post save signal of User.
+post_save.connect(create_user, sender=User)
+
+
+# Monkey patch django.contrib.auth.models.User to force email to be unique.
+User._meta.get_field('email')._unique = True
+
+
+# Register the models in the admin site.
+admin.site.register(Consumer)
+admin.site.register(Config)
+admin.site.register(FileStorage)
+admin.site.register(MACAddress)
+admin.site.register(Node)
+admin.site.register(SSHKey)
+
+
+class MAASAuthorizationBackend(ModelBackend):
+
+ supports_object_permissions = True
+
+ def has_perm(self, user, perm, obj=None):
+ # Note that a check for a superuser will never reach this code
+ # because Django will return True (as an optimization) for every
+ # permission check performed on a superuser.
+ if not user.is_active:
+ # Deactivated users, and in particular the node-init user,
+ # are prohibited from accessing maasserver services.
+ return False
+
+ # Only Nodes can be checked. We also don't support perm checking
+ # when obj = None.
+ if not isinstance(obj, Node):
+ raise NotImplementedError(
+ 'Invalid permission check (invalid object type).')
+
+ if perm == NODE_PERMISSION.VIEW:
+ return obj.owner in (None, user)
+ elif perm == NODE_PERMISSION.EDIT:
+ return obj.owner == user
+ elif perm == NODE_PERMISSION.ADMIN:
+ # 'admin_node' permission is solely granted to superusers.
+ return False
+ else:
+ raise NotImplementedError(
+ 'Invalid permission check (invalid permission name: %s).' %
+ perm)
+
+
+# 'provisioning' is imported so that it can register its signal handlers early
+# on, before it misses anything.
+from maasserver import provisioning
+ignore_unused(provisioning)
+
+from maasserver import messages
+ignore_unused(messages)
=== renamed file 'src/maasserver/models/__init__.py' => 'src/maasserver/models/node.py'
--- src/maasserver/models/__init__.py 2012-06-14 06:53:15 +0000
+++ src/maasserver/models/node.py 2012-06-15 05:19:22 +0000
@@ -14,37 +14,21 @@
)
__metaclass__ = type
-# Scheduled for model migration on 2012-06-15
__all__ = [
- "create_auth_token",
- "generate_node_system_id",
- "get_auth_tokens",
- "get_db_state",
- "logger",
- "Config",
- "FileStorage",
"NODE_TRANSITIONS",
"Node",
- "NodeGroup",
- "MACAddress",
- "SSHKey",
- "UserProfile",
]
-from logging import getLogger
import os
from string import whitespace
from uuid import uuid1
from django.conf import settings
-from django.contrib import admin
-from django.contrib.auth.backends import ModelBackend
from django.contrib.auth.models import User
from django.core.exceptions import (
PermissionDenied,
ValidationError,
)
-from django.db import connection
from django.db.models import (
CharField,
ForeignKey,
@@ -52,7 +36,6 @@
Manager,
Q,
)
-from django.db.models.signals import post_save
from django.shortcuts import get_object_or_404
from maasserver import DefaultMeta
from maasserver.enum import (
@@ -69,40 +52,15 @@
from maasserver.fields import JSONObjectField
from maasserver.models.cleansave import CleanSave
from maasserver.models.config import Config
-from maasserver.models.filestorage import FileStorage
-from maasserver.models.macaddress import MACAddress
-from maasserver.models.nodegroup import NodeGroup
-from maasserver.models.sshkey import SSHKey
from maasserver.models.timestampedmodel import TimestampedModel
-from maasserver.models.user import create_user
-from maasserver.models.userprofile import UserProfile
-from maasserver.utils import ignore_unused
-from piston.models import (
- Consumer,
- Token,
- )
+from maasserver.utils import get_db_state
+from piston.models import Token
from provisioningserver.enum import (
POWER_TYPE,
POWER_TYPE_CHOICES,
)
from provisioningserver.tasks import power_on
-# Scheduled for model migration on 2012-06-15
-# Suppress warning about symbols being imported, but only used for
-# export in __all__.
-ignore_unused(NodeGroup, UserProfile)
-
-
-# Scheduled for model migration on 2012-06-15
-logger = getLogger('maasserver')
-
-
-# Scheduled for model migration on 2012-06-15
-def now():
- cursor = connection.cursor()
- cursor.execute("select now()")
- return cursor.fetchone()[0]
-
def generate_node_system_id():
return 'node-%s' % uuid1()
@@ -359,22 +317,6 @@
return processed_nodes
-# Scheduled for model migration on 2012-06-15
-def get_db_state(instance, field_name):
- """Get the persisted state of the given field for the given instance.
-
- :param instance: The model instance to consider.
- :type instance: :class:`django.db.models.Model`
- :param field_name: The name of the field to return.
- :type field_name: basestring
- """
- try:
- return getattr(
- instance.__class__.objects.get(pk=instance.pk), field_name)
- except instance.DoesNotExist:
- return None
-
-
class Node(CleanSave, TimestampedModel):
"""A `Node` represents a physical machine used by the MAAS Server.
@@ -488,6 +430,8 @@
docs.djangoproject.com/en/dev/ref/exceptions/
#django.core.exceptions.ValidationError
"""
+ # Avoid circular imports
+ from maasserver.models import MACAddress
mac = MACAddress(mac_address=mac_address, node=self)
mac.save()
@@ -500,6 +444,9 @@
:type mac_address: str
"""
+ # Avoid circular imports
+ from maasserver.models import MACAddress
+
mac = MACAddress.objects.get(mac_address=mac_address, node=self)
if mac:
mac.delete()
@@ -629,67 +576,3 @@
self.owner = None
self.token = None
self.save()
-
-
-# Scheduled for model migration on 2012-06-15
-# Connect the 'create_user' method to the post save signal of User.
-post_save.connect(create_user, sender=User)
-
-
-# Scheduled for model migration on 2012-06-15
-# Monkey patch django.contrib.auth.models.User to force email to be unique.
-User._meta.get_field('email')._unique = True
-
-
-# Scheduled for model migration on 2012-06-15
-# Register the models in the admin site.
-admin.site.register(Consumer)
-admin.site.register(Config)
-admin.site.register(FileStorage)
-admin.site.register(MACAddress)
-admin.site.register(Node)
-admin.site.register(SSHKey)
-
-
-# Scheduled for model migration on 2012-06-15
-class MAASAuthorizationBackend(ModelBackend):
-
- supports_object_permissions = True
-
- def has_perm(self, user, perm, obj=None):
- # Note that a check for a superuser will never reach this code
- # because Django will return True (as an optimization) for every
- # permission check performed on a superuser.
- if not user.is_active:
- # Deactivated users, and in particular the node-init user,
- # are prohibited from accessing maasserver services.
- return False
-
- # Only Nodes can be checked. We also don't support perm checking
- # when obj = None.
- if not isinstance(obj, Node):
- raise NotImplementedError(
- 'Invalid permission check (invalid object type).')
-
- if perm == NODE_PERMISSION.VIEW:
- return obj.owner in (None, user)
- elif perm == NODE_PERMISSION.EDIT:
- return obj.owner == user
- elif perm == NODE_PERMISSION.ADMIN:
- # 'admin_node' permission is solely granted to superusers.
- return False
- else:
- raise NotImplementedError(
- 'Invalid permission check (invalid permission name: %s).' %
- perm)
-
-
-# Scheduled for model migration on 2012-06-15
-# 'provisioning' is imported so that it can register its signal handlers early
-# on, before it misses anything.
-from maasserver import provisioning
-ignore_unused(provisioning)
-
-# Scheduled for model migration on 2012-06-15
-from maasserver import messages
-ignore_unused(messages)
=== modified file 'src/maasserver/models/timestampedmodel.py'
--- src/maasserver/models/timestampedmodel.py 2012-05-08 02:57:56 +0000
+++ src/maasserver/models/timestampedmodel.py 2012-06-15 05:19:22 +0000
@@ -15,6 +15,7 @@
]
+from django.db import connection
from django.db.models import (
DateTimeField,
Model,
@@ -22,6 +23,13 @@
from maasserver import DefaultMeta
+def now():
+ """Current database time (as per start of current transaction)."""
+ cursor = connection.cursor()
+ cursor.execute("select now()")
+ return cursor.fetchone()[0]
+
+
class TimestampedModel(Model):
"""Abstract base model with creation/update timestamps.
@@ -38,9 +46,6 @@
updated = DateTimeField(editable=False)
def save(self, *args, **kwargs):
- # Avoid circular imports.
- from maasserver.models import now
-
current_time = now()
if self.id is None:
self.created = current_time
=== modified file 'src/maasserver/testing/factory.py'
--- src/maasserver/testing/factory.py 2012-06-14 06:53:15 +0000
+++ src/maasserver/testing/factory.py 2012-06-15 05:19:22 +0000
@@ -27,10 +27,10 @@
FileStorage,
MACAddress,
Node,
- NODE_TRANSITIONS,
NodeGroup,
SSHKey,
)
+from maasserver.models.node import NODE_TRANSITIONS
from maasserver.models.user import create_auth_token
from maasserver.testing import (
get_data,
=== modified file 'src/maasserver/tests/test_models.py'
--- src/maasserver/tests/test_models.py 2012-06-14 06:53:15 +0000
+++ src/maasserver/tests/test_models.py 2012-06-15 05:19:22 +0000
@@ -32,12 +32,11 @@
from maasserver.exceptions import NodeStateViolation
from maasserver.models import (
Config,
- get_db_state,
MACAddress,
Node,
- NODE_TRANSITIONS,
- now,
)
+from maasserver.models.node import NODE_TRANSITIONS
+from maasserver.models.timestampedmodel import now
from maasserver.models.user import (
create_auth_token,
get_auth_tokens,
@@ -50,7 +49,10 @@
TestModelTestCase,
)
from maasserver.tests.models import TimestampedModelTestModel
-from maasserver.utils import map_enum
+from maasserver.utils import (
+ get_db_state,
+ map_enum,
+ )
from maastesting.djangotestcase import (
TestModelTransactionalTestCase,
TransactionTestCase,
=== modified file 'src/maasserver/utils.py'
--- src/maasserver/utils.py 2012-06-07 17:58:23 +0000
+++ src/maasserver/utils.py 2012-06-15 05:19:22 +0000
@@ -11,11 +11,27 @@
__metaclass__ = type
__all__ = [
+ 'get_db_state',
'ignore_unused',
'map_enum',
]
+def get_db_state(instance, field_name):
+ """Get the persisted state of a given field for a given model instance.
+
+ :param instance: The model instance to consider.
+ :type instance: :class:`django.db.models.Model`
+ :param field_name: The name of the field to return.
+ :type field_name: basestring
+ """
+ try:
+ return getattr(
+ instance.__class__.objects.get(pk=instance.pk), field_name)
+ except instance.DoesNotExist:
+ return None
+
+
def ignore_unused(*args):
"""Suppress warnings about unused variables.
Follow ups
