Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/initialize-node-nodegroup
into lp:maas.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~jtv/maas/initialize-node-nodegroup/+merge/117377
This may look like a simple change, and perhaps even a good one. In reality it
was neither. The forms that save a new node make up a mesh of related classes.
One of these, a mix-in related to MAC addresses, absolutely needs to save its
model object and so can't support the commit=False argument. And it needs to
come early in the form's base classes, so this is where a node object is really
created. Thus it was in this mixin's overridden save() method that I needed to
ensure initialization of Node.nodegroup. Anywhere else and the node would
already have been written to the database without this field initialized, which
wouldn't let us add a NOT NULL constraint.
I tried reordering the forms' base classes, but this broke tests in a way that
I couldn't fathom at the time. It had come to a point where I urgently needed
to get past the practical problem and get that NOT NULL constraint into place
before I could consider the bigger picture again.
Another approach that might possibly have worked would have been to add yet
another mixin, even earlier in the inheritance chains, that merely ensured
initialization of this field before the MAC-related mixin even came into play.
I shirked from doing that because it would have increased the reliance on
ordering of parent classes even further. Instead, I ended up relying on tests
to ensure the desired behaviour. My next branch will add NOT NULL constraints,
at which point any violation will be swiftly and mercilessly punished. But
yes, it's ugly. Once we get this working I'd be happy to consider nicer
arrangements.
Jeroen
PS — this will conflict with trunk initially, since some of the code changes
adjoin a docstring change I made in another branch that is landing as I write
this.
--
https://code.launchpad.net/~jtv/maas/initialize-node-nodegroup/+merge/117377
Your team Launchpad code reviewers is requested to review the proposed merge of
lp:~jtv/maas/initialize-node-nodegroup into lp:maas.
=== modified file 'src/maasserver/forms.py'
--- src/maasserver/forms.py 2012-07-31 04:43:48 +0000
+++ src/maasserver/forms.py 2012-07-31 06:28:22 +0000
@@ -59,6 +59,7 @@
Config,
MACAddress,
Node,
+ NodeGroup,
SSHKey,
)
from maasserver.node_action import compile_node_actions
@@ -267,6 +268,12 @@
return []
+def initialize_node_group(node):
+ """If `node` is not in a node group yet, enroll it in the master group."""
+ if node.nodegroup is None:
+ node.nodegroup = NodeGroup.objects.ensure_master()
+
+
class WithMACAddressesMixin:
"""A form mixin which dynamically adds a MultipleMACAddressField to the
list of fields. This mixin also overrides the 'save' method to persist
@@ -306,11 +313,20 @@
return data
def save(self):
+<<<<<<< TREE
"""Save the form's data to the database.
This implementation of `save` does not support the `commit` argument.
"""
node = super(WithMACAddressesMixin, self).save()
+=======
+ node = super(WithMACAddressesMixin, self).save(commit=False)
+ # We have to save this node in order to attach MACAddress
+ # records to it. But its nodegroup must be initialized before
+ # we can do that.
+ initialize_node_group(node)
+ node.save()
+>>>>>>> MERGE-SOURCE
for mac in self.cleaned_data['mac_addresses']:
node.add_mac_address(mac)
if self.cleaned_data['hostname'] == "":
=== modified file 'src/maasserver/tests/test_api.py'
--- src/maasserver/tests/test_api.py 2012-07-30 15:42:41 +0000
+++ src/maasserver/tests/test_api.py 2012-07-31 06:28:22 +0000
@@ -54,6 +54,7 @@
DHCPLease,
MACAddress,
Node,
+ NodeGroup,
)
from maasserver.models.user import (
create_auth_token,
@@ -283,6 +284,20 @@
['aa:bb:cc:dd:ee:ff', '22:bb:cc:dd:ee:ff'],
[mac.mac_address for mac in diane.macaddress_set.all()])
+ def test_POST_new_initializes_nodegroup_to_master_by_default(self):
+ hostname = factory.make_name('host')
+ self.client.post(
+ self.get_uri('nodes/'),
+ {
+ 'op': 'new',
+ 'hostname': hostname,
+ 'architecture': factory.getRandomChoice(ARCHITECTURE_CHOICES),
+ 'mac_addresses': [factory.getRandomMACAddress()],
+ })
+ self.assertEqual(
+ NodeGroup.objects.ensure_master(),
+ Node.objects.get(hostname=hostname).nodegroup)
+
def test_POST_with_no_hostname_auto_populates_hostname(self):
architecture = factory.getRandomChoice(ARCHITECTURE_CHOICES)
response = self.client.post(
=== modified file 'src/maasserver/tests/test_forms.py'
--- src/maasserver/tests/test_forms.py 2012-07-31 04:23:03 +0000
+++ src/maasserver/tests/test_forms.py 2012-07-31 06:28:22 +0000
@@ -34,6 +34,7 @@
get_node_create_form,
get_node_edit_form,
HostnameFormField,
+ initialize_node_group,
MACAddressForm,
NewUserCreationForm,
NodeActionForm,
@@ -46,6 +47,8 @@
from maasserver.models import (
Config,
MACAddress,
+ Node,
+ NodeGroup,
)
from maasserver.models.config import DEFAULT_CONFIG
from maasserver.node_action import (
@@ -58,6 +61,22 @@
from testtools.testcase import ExpectedException
+class TestHelpers(TestCase):
+
+ def test_initialize_node_group_initializes_nodegroup_to_master(self):
+ node = Node(
+ NODE_STATUS.DECLARED,
+ architecture=factory.getRandomEnum(ARCHITECTURE))
+ initialize_node_group(node)
+ self.assertEqual(NodeGroup.objects.ensure_master(), node.nodegroup)
+
+ def test_initialize_node_group_leaves_nodegroup_reference_intact(self):
+ preselected_nodegroup = factory.make_node_group()
+ node = factory.make_node(nodegroup=preselected_nodegroup)
+ initialize_node_group(node)
+ self.assertEqual(preselected_nodegroup, node.nodegroup)
+
+
class NodeWithMACAddressesFormTest(TestCase):
def get_QueryDict(self, params):
@@ -69,29 +88,35 @@
query_dict[k] = v
return query_dict
+ def make_params(self, mac_addresses=None, architecture=None):
+ if mac_addresses is None:
+ mac_addresses = [factory.getRandomMACAddress()]
+ if architecture is None:
+ architecture = factory.getRandomEnum(ARCHITECTURE)
+ return self.get_QueryDict({
+ 'mac_addresses': mac_addresses,
+ 'architecture': architecture,
+ })
+
def test_NodeWithMACAddressesForm_valid(self):
-
+ architecture = factory.getRandomEnum(ARCHITECTURE)
form = NodeWithMACAddressesForm(
- self.get_QueryDict({
- 'mac_addresses': ['aa:bb:cc:dd:ee:ff', '9a:bb:c3:33:e5:7f'],
- 'architecture': ARCHITECTURE.i386,
- }))
+ self.make_params(
+ mac_addresses=['aa:bb:cc:dd:ee:ff', '9a:bb:c3:33:e5:7f'],
+ architecture=architecture))
self.assertTrue(form.is_valid())
self.assertEqual(
['aa:bb:cc:dd:ee:ff', '9a:bb:c3:33:e5:7f'],
form.cleaned_data['mac_addresses'])
- self.assertEqual(ARCHITECTURE.i386, form.cleaned_data['architecture'])
+ self.assertEqual(architecture, form.cleaned_data['architecture'])
def test_NodeWithMACAddressesForm_simple_invalid(self):
# If the form only has one (invalid) MAC address field to validate,
# the error message in form.errors['mac_addresses'] is the
# message from the field's validation error.
form = NodeWithMACAddressesForm(
- self.get_QueryDict({
- 'mac_addresses': ['invalid'],
- 'architecture': ARCHITECTURE.i386,
- }))
+ self.make_params(mac_addresses=['invalid']))
self.assertFalse(form.is_valid())
self.assertEqual(['mac_addresses'], list(form.errors))
@@ -104,10 +129,7 @@
# if one or more fields are invalid, a single error message is
# present in form.errors['mac_addresses'] after validation.
form = NodeWithMACAddressesForm(
- self.get_QueryDict({
- 'mac_addresses': ['invalid_1', 'invalid_2'],
- 'architecture': ARCHITECTURE.i386,
- }))
+ self.make_params(mac_addresses=['invalid_1', 'invalid_2']))
self.assertFalse(form.is_valid())
self.assertEqual(['mac_addresses'], list(form.errors))
@@ -118,26 +140,26 @@
def test_NodeWithMACAddressesForm_empty(self):
# Empty values in the list of MAC addresses are simply ignored.
form = NodeWithMACAddressesForm(
- self.get_QueryDict({
- 'mac_addresses': ['aa:bb:cc:dd:ee:ff', ''],
- 'architecture': ARCHITECTURE.i386,
- }))
+ self.make_params(
+ mac_addresses=[factory.getRandomMACAddress(), '']))
self.assertTrue(form.is_valid())
def test_NodeWithMACAddressesForm_save(self):
- form = NodeWithMACAddressesForm(
- self.get_QueryDict({
- 'mac_addresses': ['aa:bb:cc:dd:ee:ff', '9a:bb:c3:33:e5:7f'],
- 'architecture': ARCHITECTURE.i386,
- }))
+ macs = ['aa:bb:cc:dd:ee:ff', '9a:bb:c3:33:e5:7f']
+ form = NodeWithMACAddressesForm(self.make_params(mac_addresses=macs))
node = form.save()
self.assertIsNotNone(node.id) # The node is persisted.
self.assertSequenceEqual(
- ['aa:bb:cc:dd:ee:ff', '9a:bb:c3:33:e5:7f'],
+ macs,
[mac.mac_address for mac in node.macaddress_set.all()])
+ def test_sets_nodegroup_to_master_by_default(self):
+ self.assertEqual(
+ NodeGroup.objects.ensure_master(),
+ NodeWithMACAddressesForm(self.make_params()).save().nodegroup)
+
class TestOptionForm(ConfigForm):
field1 = forms.CharField(label="Field 1", max_length=10)
_______________________________________________
Mailing list: https://launchpad.net/~launchpad-reviewers
Post to : [email protected]
Unsubscribe : https://launchpad.net/~launchpad-reviewers
More help : https://help.launchpad.net/ListHelp
