Merge lp:~gmb/maas/json-schema-part-1 into lp:~maas-committers/maas/trunk

[Graham Binns]

Convert the definition of power type parameters from straight Python to something that's JSON-schema-validatable. This is the first step in making it possible for hardware enablement drivers to specify what power parameters they accept.

After a lot of dancing around, this is the first part of the work to making power type parameters specifiable in JSON.

In order to do this I've:

 - Defined a JSON schema to describe how power type parameters should look
   (this is split up so that we can validate separate parts of it individually)
 - Created a JSON-validatable version of the existing POWER_TYPE_PARAMETERS dict
 - Created a function, get_power_type_parameters_from_json() to convert the
   JSON power type parameters to something similar to the existing
   POWER_TYPE_PARAMETERS dict.
 - Use get_power_type_parameters_from_json() to create POWER_TYPE_PARAMETERS.
   This is done once at the bottom of maasserver.power_parameters so that we're
   not parsing and validating JSON all the time.

[Julian Edwards (julian-edwards)]

On Friday 07 Feb 2014 09:20:37 you wrote:
> Commit message:
> Convert the definition of power type parameters from straight Python to
> something that's JSON-schema-validatable.

Can you add something about why this is happening please.

> After a lot of dancing around, this is the first part of the work to making
> power type parameters specifiable in JSON.

\o/

> In order to do this I've:
>
> - Defined a JSON schema to describe how power type parameters should look
> (this is split up so that we can validate separate parts of it
> individually) - Created a JSON-validatable version of the existing
> POWER_TYPE_PARAMETERS dict - Created a function,
> get_power_type_parameters_from_json() to convert the JSON power type
> parameters to something similar to the existing POWER_TYPE_PARAMETERS dict.
> - Use get_power_type_parameters_from_json() to create
> POWER_TYPE_PARAMETERS. This is done once at the bottom of
> maasserver.power_parameters so that we're not parsing and validating JSON
> all the time.

This is mostly looking *great*. I'd like to get Gavin to take a peek to make
sure it matches his grand vision, but it looks like it's on the right track to
me.

Some nits:

[1] You're missing :param:, :type: and :return: info on the docstrings for the
new functions.

(FWIW My vim plugin reads the :type: and offers better auto-completions :)

[2] I realise it's early days, but the FIELD_TYPE_MAPPINGS are going to form
part of the API with the drivers. I think putting these in their own module
would be better (like errno), so we can do something like
powerfield.mac_address, for example. This also then forms a firm contract
with the driver, which consumes the API. (It also makes it easier to install
for packaging and is something we should have done for enums so that maas api
clients can use them)

[3] jsonschema. MY EYES!
OK this ones not a nit :)

[4] Missing tests for make_json_field.

cheers.

[Julian Edwards (julian-edwards)]

review: Approve

[Gavin Panella (allenap)]

Looks grand :)

[1]

Couple of minor things:

+def make_json_field(name, label, field_type=None, choices=None, default=None,
+                    required=False):

Dosnae matter much, but try using the following form for indentation:

def make_json_field(
        name, label, field_type=None, choices=None, default=None,
        required=False):
    ... code here ...

+    """Helper function for build a JSON power type parameters field."""

s/for/to/

review: Approve

[Graham Binns (gmb)]

On 10 February 2014 02:24, Julian Edwards <email address hidden> wrote:
> On Friday 07 Feb 2014 09:20:37 you wrote:
>> Commit message:
>> Convert the definition of power type parameters from straight Python to
>> something that's JSON-schema-validatable.
>
> Can you add something about why this is happening please.

Yeah, sorry. I've gotten into a laziness habit with commit messages...

>> After a lot of dancing around, this is the first part of the work to making
>> power type parameters specifiable in JSON.
>
> \o/
>
> This is mostly looking *great*. I'd like to get Gavin to take a peek to make
> sure it matches his grand vision, but it looks like it's on the right track to
> me.

+1. Will bug Gavin.

> Some nits:
>
> [1] You're missing :param:, :type: and :return: info on the docstrings for the
> new functions.
>
> (FWIW My vim plugin reads the :type: and offers better auto-completions :)

Which vim plugin loveliness is it of which you speak? Fixed.

>
> [2] I realise it's early days, but the FIELD_TYPE_MAPPINGS are going to form
> part of the API with the drivers. I think putting these in their own module
> would be better (like errno), so we can do something like
> powerfield.mac_address, for example. This also then forms a firm contract
> with the driver, which consumes the API. (It also makes it easier to install
> for packaging and is something we should have done for enums so that maas api
> clients can use them)

Okay, I'm a bit lost as to what you mean here... I was with you up
until "their own module" but then you started talking about packaging
and I got confused. Are you talking about just adding a module under
src/maasserver (which is what I think you mean)?

Anyway, in the interests of getting this landed I'll do that in a
followup branch once I've got clarity.

> [3] jsonschema. MY EYES!
> OK this ones not a nit :)

DEAR GOD YES.

> [4] Missing tests for make_json_field.
>

Fixed. Cheers for the review.

[Graham Binns (gmb)]

On 10 February 2014 10:25, Gavin Panella <email address hidden> wrote:
> Couple of minor things:
>
> +def make_json_field(name, label, field_type=None, choices=None, default=None,
> + required=False):
>
> Dosnae matter much, but try using the following form for indentation:

Done. Is that the standard form for MAAS? I was doing it the LP way
but this looks better.

> + """Helper function for build a JSON power type parameters field."""

Actually should have been s/build/building/. Fixed.

[Raphaël Badin (rvb)]

This introduces a new dependency: python-jsonschema. This means the packaging branch needs to be updated.

Next time, let's try to do this before landing the branch on lp:maas please. Otherwise it breaks the package, causes the integration tests to fail, and whoever ends up looking into it is wasting his time.

[Julian Edwards (julian-edwards)]

On Monday 10 Feb 2014 12:06:29 you wrote:
> > (FWIW My vim plugin reads the :type: and offers better auto-completions :)
>
> Which vim plugin loveliness is it of which you speak? Fixed.

https://github.com/Valloric/YouCompleteMe

It is seriously *awesome*.

> > [2] I realise it's early days, but the FIELD_TYPE_MAPPINGS are going to
> > form part of the API with the drivers. I think putting these in their
> > own module would be better (like errno), so we can do something like
> > powerfield.mac_address, for example. This also then forms a firm contract
> > with the driver, which consumes the API. (It also makes it easier to
> > install for packaging and is something we should have done for enums so
> > that maas api clients can use them)
>
> Okay, I'm a bit lost as to what you mean here... I was with you up
> until "their own module" but then you started talking about packaging
> and I got confused. Are you talking about just adding a module under
> src/maasserver (which is what I think you mean)?

Ok so these are a bit like the enums in that third party code (drivers) are
going to need access. Putting it in its own module will help.

For packaging, I was thinking more about maas-cli because we could install an
enums.py (for example) along with the cli itself. This doesn't translate too
well for the drivers since they are going to be part and parcel of maas
itself; it's just that we want to make life as easy as possible for driver
writers so I'd prefer if we had all things that drivers will need to import in
a separate and convenience place.

Hope that makes sense :)

> Anyway, in the interests of getting this landed I'll do that in a
> followup branch once I've got clarity.

Naw worries.

It could probably even wait until Gavin starts writing the API layer.

> Fixed. Cheers for the review.

Pleasure.

[Julian Edwards (julian-edwards)]

On Monday 10 Feb 2014 13:17:31 you wrote:
> This introduces a new dependency: python-jsonschema. This means the
> packaging branch needs to be updated.
>
> Next time, let's try to do this before landing the branch on lp:maas please.
> Otherwise it breaks the package, causes the integration tests to fail, and
> whoever ends up looking into it is wasting his time.

Argh, I meant to say something about that when I read the branch and totally
forgot.

=== modified file 'required-packages/base'
--- required-packages/base	2014-01-20 09:39:00 +0000
+++ required-packages/base	2014-02-10 12:07:12 +0000
@@ -24,6 +24,7 @@
 python-docutils
 python-formencode
 python-httplib2
+python-jsonschema
 python-lockfile
 python-netaddr
 python-netifaces

=== modified file 'src/maasserver/power_parameters.py'
--- src/maasserver/power_parameters.py	2014-01-21 06:08:12 +0000
+++ src/maasserver/power_parameters.py	2014-02-10 12:07:12 +0000
@@ -33,6 +33,7 @@
 
 
 from django import forms
+from jsonschema import validate
 from maasserver.config_forms import DictCharField
 from maasserver.fields import MACAddressFormField
 from provisioningserver.enum import (
@@ -40,92 +41,229 @@
     IPMI_DRIVER_CHOICES,
     )
 
-# FIXME: These should come from a hardware driver.
-POWER_TYPE_PARAMETERS = {
-    # Empty type, for the case where nothing is entered in the form yet.
-    '': DictCharField(
-        [], required=False, skip_check=True),
-    'ether_wake': DictCharField(
-        [
-            ('mac_address',
-             MACAddressFormField(label="MAC Address", required=False)),
-        ],
-        required=False,
-        skip_check=True),
-    'virsh': DictCharField(
-        [
-            ('power_address',
-             forms.CharField(label="Address", required=False)),
-            ('power_id',
-             forms.CharField(label="Power ID", required=False)),
-        ],
-        required=False,
-        skip_check=True),
-    'fence_cdu': DictCharField(
-        [
-            ('power_id',
-             forms.CharField(label="Power ID", required=False)),
-            ('power_address',
-             forms.CharField(label="IP Address or Hostname", required=False)),
-            ('power_user',
-             forms.CharField(label="Username", required=False)),
-            ('power_pass',
-             forms.CharField(label="Password", required=False)),
-        ],
-        required=False,
-        skip_check=True),
-    'ipmi': DictCharField(
-        [
-            ('power_driver',
-             forms.ChoiceField(
-                 label="Driver", required=False,
-                 choices=IPMI_DRIVER_CHOICES,
-                 initial=IPMI_DRIVER.DEFAULT)),
-            ('power_address',
-             forms.CharField(label="IP Address or Hostname", required=False)),
-            ('power_user',
-             forms.CharField(label="Username", required=False)),
-            ('power_pass',
-             forms.CharField(label="Password", required=False)),
-        ],
-        required=False,
-        skip_check=True),
-    'moonshot': DictCharField(
-        [
-            ('power_address',
-             forms.CharField(
-                 label="IP Address or Hostname", required=False)),
-            ('power_user',
-             forms.CharField(label="Username", required=False)),
-            ('power_pass',
-             forms.CharField(label="Password", required=False)),
-            ('power_hwaddress',
-             forms.CharField(label="Hardware Address", required=False)),
-        ],
-        required=False,
-        skip_check=True),
-    'sm15k': DictCharField(
-        [
-            ('power_address',
-             forms.CharField(
-                 label="IP Address or Hostname", required=False)),
-            ('power_user',
-             forms.CharField(label="Username", required=False)),
-            ('power_pass',
-             forms.CharField(label="Password", required=False)),
-            ('system_id',
-             forms.CharField(label="System ID", required=False)),
-        ],
-        required=False,
-        skip_check=True),
-    'amt': DictCharField(
-        [
-            ('power_address',
-             forms.CharField(
-                 label="IP Address or Hostname", required=False)),
-            ('power_pass',
-             forms.CharField(label="Password", required=False)),
-        ],
-        required=False,
-        skip_check=True),
-}
+
+# Represent the Django choices format as JSON; an array of 2-item
+# arrays.
+CHOICE_FIELD_SCHEMA = {
+    'type': 'array',
+    'items': {
+        'title': "Power type paramter field choice",
+        'type': 'array',
+        'minItems': 2,
+        'maxItems': 2,
+        'uniqueItems': True,
+        'items': {
+            'type': 'string',
+        }
+    },
+}
+
+
+POWER_TYPE_PARAMETER_FIELD_SCHEMA = {
+    'title': "Power type parameter field",
+    'type': 'object',
+    'properties': {
+        'name': {
+            'type': 'string',
+        },
+        'field_type': {
+            'type': 'string',
+        },
+        'label': {
+            'type': 'string',
+        },
+        'required': {
+            'type': 'boolean',
+        },
+        'choices': CHOICE_FIELD_SCHEMA,
+        'default': {
+            'type': 'string',
+        },
+    },
+    'required': ['field_type', 'label', 'required'],
+}
+
+
+# A basic JSON schema for what power type parameters should look like.
+JSON_POWER_TYPE_PARAMETERS_SCHEMA = {
+    'title': "Power parameters set",
+    'type': 'array',
+    'items': {
+        'title': "Power type parameters",
+        'type': 'object',
+        'properties': {
+            'fields': {
+                'type': 'array',
+                'items': POWER_TYPE_PARAMETER_FIELD_SCHEMA,
+            },
+        },
+        'required': ['fields'],
+    },
+}
+
+
+def make_json_field(
+        name, label, field_type=None, choices=None, default=None,
+        required=False):
+    """Helper function for building a JSON power type parameters field.
+
+    :param name: The name of the field.
+    :type name: string
+    :param label: The label to be presented to the user for this field.
+    :type label: string
+    :param field_type: The type of field to create. Can be one of
+        (string, choice, mac_address). Defaults to string.
+    :type field_type: string.
+    :param choices: The collection of choices to present to the user.
+        Needs to be structured as a list of lists, otherwise
+        make_json_field() will raise a ValidationError.
+    :type list:
+    :param default: The default value for the field.
+    :type default: string
+    :param required: Whether or not a value for the field is required.
+    :type required: boolean
+    """
+    if field_type not in ('string', 'mac_address', 'choice'):
+        field_type = 'string'
+    if choices is None:
+        choices = []
+    validate(choices, CHOICE_FIELD_SCHEMA)
+    if default is None:
+        default = ""
+    field = {
+        'name': name,
+        'label': label,
+        'required': required,
+        'field_type': field_type,
+        'choices': choices,
+        'default': default,
+    }
+    return field
+
+
+# FIXME this should all be produced by hardware drivers, not defined statically
+# like this.
+JSON_POWER_TYPE_PARAMETERS = [
+    {
+        'name': 'ether_wake',
+        'fields': [
+            make_json_field(
+                'mac_address', "MAC Address", field_type='mac_address'),
+        ],
+    },
+    {
+        'name': 'virsh',
+        'fields': [
+            make_json_field('power_address', "Power address"),
+            make_json_field('power_id', "Power ID"),
+        ],
+    },
+    {
+        'name': 'fence_cdu',
+        'fields': [
+            make_json_field('power_address', "Power address"),
+            make_json_field('power_id', "Power ID"),
+            make_json_field('power_user', "Power user"),
+            make_json_field('power_pass', "Power password"),
+        ],
+    },
+    {
+        'name': 'ipmi',
+        'fields': [
+            make_json_field(
+                'power_driver', "Power driver", field_type='choice',
+                choices=IPMI_DRIVER_CHOICES, default=IPMI_DRIVER.DEFAULT),
+            make_json_field('power_address', "Power address"),
+            make_json_field('power_user', "Power user"),
+            make_json_field('power_pass', "Power password"),
+        ],
+    },
+    {
+        'name': 'moonshot',
+        'fields': [
+            make_json_field('power_address', "Power address"),
+            make_json_field('power_user', "Power user"),
+            make_json_field('power_pass', "Power password"),
+            make_json_field('power_hwaddress', "Power hardware address"),
+        ],
+    },
+    {
+        'name': 'sm15k',
+        'fields': [
+            make_json_field('system_id', "System ID"),
+            make_json_field('power_address', "Power address"),
+            make_json_field('power_user', "Power user"),
+            make_json_field('power_pass', "Power password"),
+        ],
+    },
+    {
+        'name': 'amt',
+        'fields': [
+            make_json_field('power_address', "Power address"),
+            make_json_field('power_pass', "Power password"),
+        ],
+    },
+]
+
+
+FIELD_TYPE_MAPPINGS = {
+    'string': forms.CharField,
+    'mac_address': MACAddressFormField,
+    'choice': forms.ChoiceField,
+}
+
+
+def make_form_field(json_field):
+    """Build a Django form field based on the JSON spec.
+
+    :param json_field: The JSON-specified field to convert into a valid
+        Djangoism.
+    :type json_field: dict
+    :return: The correct Django form field for the field type, as
+        specified in FIELD_TYPE_MAPPINGS.
+    """
+    field_class = FIELD_TYPE_MAPPINGS.get(
+        json_field['field_type'], forms.CharField)
+    if json_field['field_type'] == 'choice':
+        extra_parameters = {
+            'choices': json_field['choices'],
+            'initial': json_field['default'],
+            }
+    else:
+        extra_parameters = {}
+    form_field = field_class(
+        label=json_field['label'], required=json_field['required'],
+        **extra_parameters)
+    return form_field
+
+
+def get_power_type_parameters_from_json(json_power_type_parameters):
+    """Return power type parameters.
+
+    :param json_power_type_parameters: Power type parameters expressed
+        as a JSON string or as set of JSONSchema-verifiable objects.
+        Will be validated using jsonschema.validate().
+    :type json_power_type_parameters: JSON string or iterable.
+    :return: A dict of power parameters for all power types, indexed by
+        power type name.
+    """
+    validate(json_power_type_parameters, JSON_POWER_TYPE_PARAMETERS_SCHEMA)
+    power_parameters = {
+        # Empty type, for the case where nothing is entered in the form yet.
+        '': DictCharField(
+            [], required=False, skip_check=True),
+    }
+    for power_type in json_power_type_parameters:
+        fields = []
+        for json_field in power_type['fields']:
+            fields.append((
+                json_field['name'], make_form_field(json_field)))
+        params = DictCharField(fields, required=False, skip_check=True)
+        power_parameters[power_type['name']] = params
+    return power_parameters
+
+
+# We do this once because there's no point re-parsing the JSON every
+# time we need to look up the power type parameters code.
+POWER_TYPE_PARAMETERS = get_power_type_parameters_from_json(
+    JSON_POWER_TYPE_PARAMETERS)

=== modified file 'src/maasserver/tests/test_power_parameters.py'
--- src/maasserver/tests/test_power_parameters.py	2014-01-21 06:08:12 +0000
+++ src/maasserver/tests/test_power_parameters.py	2014-02-10 12:07:12 +0000
@@ -14,13 +14,20 @@
 __metaclass__ = type
 __all__ = []
 
+from django import forms
+import jsonschema
 from maasserver.config_forms import DictCharField
-from maasserver.power_parameters import POWER_TYPE_PARAMETERS
+from maasserver.fields import MACAddressFormField
+from maasserver.power_parameters import (
+    get_power_type_parameters_from_json,
+    make_form_field,
+    make_json_field,
+    POWER_TYPE_PARAMETERS,
+    POWER_TYPE_PARAMETER_FIELD_SCHEMA,
+    )
 from maasserver.testing.factory import factory
 from maasserver.testing.testcase import MAASServerTestCase
-from provisioningserver.enum import (
-    get_power_types,
-    )
+from provisioningserver.enum import get_power_types
 from provisioningserver.power.poweraction import PowerAction
 from testtools.matchers import (
     AllMatch,
@@ -76,3 +83,138 @@
         script = action.render_template(action.get_template(), **params)
         # The real check is that the rendering went fine.
         self.assertIsInstance(script, bytes)
+
+
+class TestGetPowerTypeParametersFromJSON(MAASServerTestCase):
+    """Test that get_power_type_parametrs_from_json."""
+
+    def test_validates_json_power_type_parameters(self):
+        invalid_parameters = [{
+            'name': 'invalid_power_type',
+            'fields': 'nothing to see here',
+        }]
+        self.assertRaises(
+            jsonschema.ValidationError, get_power_type_parameters_from_json,
+            invalid_parameters)
+
+    def test_includes_empty_power_type(self):
+        json_parameters = [{
+            'name': 'something',
+            'fields': [{
+                'name': 'some_field',
+                'label': 'Some Field',
+                'field_type': 'string',
+                'required': False,
+            }],
+        }]
+        power_type_parameters = get_power_type_parameters_from_json(
+            json_parameters)
+        self.assertEqual(['', 'something'], power_type_parameters.keys())
+
+    def test_creates_dict_char_fields(self):
+        json_parameters = [{
+            'name': 'something',
+            'fields': [{
+                'name': 'some_field',
+                'label': 'Some Field',
+                'field_type': 'string',
+                'required': False,
+            }],
+        }]
+        power_type_parameters = get_power_type_parameters_from_json(
+            json_parameters)
+        for name, field in power_type_parameters.items():
+            self.assertIsInstance(field, DictCharField)
+
+
+class TestMakeFormField(MAASServerTestCase):
+    """Test that make_form_field() converts JSON fields to Django."""
+
+    def test_creates_char_field_for_strings(self):
+        json_field = {
+            'name': 'some_field',
+            'label': 'Some Field',
+            'field_type': 'string',
+            'required': False,
+        }
+        django_field = make_form_field(json_field)
+        self.assertIsInstance(django_field, forms.CharField)
+
+    def test_creates_choice_field_for_choices(self):
+        json_field = {
+            'name': 'some_field',
+            'label': 'Some Field',
+            'field_type': 'choice',
+            'choices': [
+                ['choice-one', 'Choice One'],
+                ['choice-two', 'Choice Two'],
+            ],
+            'default': 'choice-one',
+            'required': False,
+        }
+        django_field = make_form_field(json_field)
+        self.assertIsInstance(django_field, forms.ChoiceField)
+        self.assertEqual(json_field['choices'], django_field.choices)
+        self.assertEqual(json_field['default'], django_field.initial)
+
+    def test_creates_mac_address_field_for_mac_addresses(self):
+        json_field = {
+            'name': 'some_field',
+            'label': 'Some Field',
+            'field_type': 'mac_address',
+            'required': False,
+        }
+        django_field = make_form_field(json_field)
+        self.assertIsInstance(django_field, MACAddressFormField)
+
+    def test_sets_properties_on_form_field(self):
+        json_field = {
+            'name': 'some_field',
+            'label': 'Some Field',
+            'field_type': 'string',
+            'required': False,
+        }
+        django_field = make_form_field(json_field)
+        self.assertEqual(
+            (json_field['label'], json_field['required']),
+            (django_field.label, django_field.required))
+
+
+class TestMakeJSONField(MAASServerTestCase):
+    """Test that make_json_field() creates JSON-verifiable fields."""
+
+    def test_returns_json_verifiable_dict(self):
+        json_field = make_json_field('some_field', 'Some Label')
+        jsonschema.validate(json_field, POWER_TYPE_PARAMETER_FIELD_SCHEMA)
+
+    def test_provides_sane_default_values(self):
+        json_field = make_json_field('some_field', 'Some Label')
+        expected_field = {
+            'name': 'some_field',
+            'label': 'Some Label',
+            'required': False,
+            'field_type': 'string',
+            'choices': [],
+            'default': '',
+        }
+        self.assertEqual(expected_field, json_field)
+
+    def test_sets_field_values(self):
+        expected_field = {
+            'name': 'yet_another_field',
+            'label': 'Can I stop writing tests now?',
+            'required': True,
+            'field_type': 'string',
+            'choices': [
+                ['spam', 'Spam'],
+                ['eggs', 'Eggs'],
+            ],
+            'default': 'spam',
+        }
+        json_field = make_json_field(**expected_field)
+        self.assertEqual(expected_field, json_field)
+
+    def test_validates_choices(self):
+        self.assertRaises(
+            jsonschema.ValidationError, make_json_field,
+            'some_field', 'Some Label', choices="Nonsense")

=== modified file 'src/provisioningserver/enum.py'
--- src/provisioningserver/enum.py	2014-01-31 10:46:57 +0000
+++ src/provisioningserver/enum.py	2014-02-10 12:07:12 +0000
@@ -92,11 +92,11 @@
     LAN_2_0 = 'LAN_2_0'
 
 
-IPMI_DRIVER_CHOICES = (
-    (IPMI_DRIVER.DEFAULT, "Auto-detect"),
-    (IPMI_DRIVER.LAN, "LAN (IPMI 1.5)"),
-    (IPMI_DRIVER.LAN_2_0, "LAN_2_0 (IPMI 2.0)"),
-    )
+IPMI_DRIVER_CHOICES = [
+    [IPMI_DRIVER.DEFAULT, "Auto-detect"],
+    [IPMI_DRIVER.LAN, "LAN [IPMI 1.5]"],
+    [IPMI_DRIVER.LAN_2_0, "LAN_2_0 [IPMI 2.0]"],
+    ]
 
 
 class ARP_HTYPE: