launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #07457
[Merge] lp:~jtv/maas/jsenums into lp:maas
Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/jsenums into lp:maas with lp:~jtv/maas/reusable-map_enum as a prerequisite.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~jtv/maas/jsenums/+merge/104126
We discussed this on Monday's standup call. The change generates a JavaScript version of our python-side enums, so that our JS can make use of it without hard-coding or manual duplication. A Makefile rule keeps the file in sync. The generated file is not under revision control, but roaksoax confirms that given where it is generated, it will automatically be installed with the rest of the files.
Raphaël suggested as a possible alternative the approach we currently use to convey configuration items to the JS client. It's really a question of where to draw the line between the ease and flexibility of run-time passing of data; and the advantages of caching, minification, loader optimizations, and so on. I went for the latter, and once we do this (and may sometimes have to run a “make” after changing an enum) it's possible that we'll want the efficiency gains in more places in the future.
(You may wonder why I don't just dump these as JSON, but I wanted some consistency to avoid pollution from arbitrary ordering changes. I used JSON at a lower level, but int enumeration values stay ints to match existing usage in the code.)
With this change, JavaScript code can acccess enums like:
Y.maas.enums.NODE_STATUS.DECLARED
But of course in most cases you'll want to keep a reference to the enum in a variable:
var NODE_STATUS = Y.maas.enums.NODE_STATUS;
// ...
if (status === NODE_STATUS.DECLARED) {
// ...
}
Note by the way that I called the JS module “enums,” plural, not “enum.” Using “enum” for a module that isn't _about_ enumeration but a container _of_ enums never sat quite right with me, and seeing my editor highlight the word “enum” as a special word made me change it as one of the steps to get all this working. (It's not actually a reserved word though, so I could probably change it back if I had to).
I have only have two real regrets about this branch: the size of the diff (yes, dear reviewer, I care about you) and the lack of unit tests. I can't solve both in the same branch. Yes, enum generation works as proven in end-to-end tests; and yes, more or less anything that happens to work for us is good enough for this particular job. But I'd like to have something a bit more solid when it comes to testing.
So here's what I propose: *in a separate branch* I move the meat of the new script into a maasserver module somewhere, unit-test the hell out of it, and make the script itself jump into that module as soon as possible.
Jeroen
--
https://code.launchpad.net/~jtv/maas/jsenums/+merge/104126
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/maas/jsenums into lp:maas.
=== modified file '.bzrignore'
--- .bzrignore 2012-04-20 16:09:23 +0000
+++ .bzrignore 2012-04-30 16:33:20 +0000
@@ -18,6 +18,7 @@
./parts
./run/*
./services/*/supervise
+src/maasserver/static/js/enums.js
./TAGS
./tags
./twisted/plugins/dropin.cache
=== modified file 'Makefile'
--- Makefile 2012-04-25 13:50:42 +0000
+++ Makefile 2012-04-30 16:33:20 +0000
@@ -5,7 +5,8 @@
bin/maas bin/test.maas \
bin/twistd.pserv bin/test.pserv \
bin/twistd.txlongpoll \
- bin/py bin/ipy
+ bin/py bin/ipy \
+ enums
all: build doc
@@ -13,11 +14,11 @@
$(PYTHON) bootstrap.py --distribute --setup-source distribute_setup.py
@touch --no-create $@ # Ensure it's newer than its dependencies.
-bin/maas: bin/buildout buildout.cfg versions.cfg setup.py
+bin/maas: bin/buildout buildout.cfg versions.cfg setup.py enums
bin/buildout install maas
@touch --no-create $@
-bin/test.maas: bin/buildout buildout.cfg versions.cfg setup.py
+bin/test.maas: bin/buildout buildout.cfg versions.cfg setup.py enums
bin/buildout install maas-test
@touch --no-create $@
@@ -48,7 +49,7 @@
dev-db:
utilities/maasdb start ./db/ disposable
-test: bin/test.maas bin/test.pserv
+test: bin/test.maas bin/test.pserv enums
bin/test.maas
bin/test.pserv
@@ -79,10 +80,20 @@
doc: bin/sphinx docs/api.rst
bin/sphinx
+# JavaScript enums module, generated from python enums modules.
+JSENUMS=src/maasserver/static/js/enums.js
+
+# Generate JavaScript enums from python enums.
+enums: $(JSENUMS)
+
+$(JSENUMS): utilities/convert-enums.py src/*/enum.py
+ utilities/convert-enums.py --src=src >$@
+
clean:
find . -type f -name '*.py[co]' -print0 | xargs -r0 $(RM)
find . -type f -name '*~' -print0 | xargs -r0 $(RM)
$(RM) -r media/demo/* media/development
+ $(RM) $(JSENUMS)
distclean: clean stop
utilities/maasdb delete-cluster ./db/
@@ -110,6 +121,7 @@
dev-db
distclean
doc
+ enums
harness
lint
lint-css
=== modified file 'src/maasserver/static/js/node_views.js'
--- src/maasserver/static/js/node_views.js 2012-04-17 16:27:21 +0000
+++ src/maasserver/static/js/node_views.js 2012-04-30 16:33:20 +0000
@@ -14,6 +14,9 @@
// Only used to mockup io in tests.
module._io = new Y.IO();
+var NODE_STATUS = Y.maas.enums.NODE_STATUS;
+
+
/**
* A base view class to display a set of Nodes (Y.maas.node.Node).
*
@@ -303,6 +306,7 @@
*/
updateStatus: function(action, status) {
var update_chart = false;
+
/* This seems like an ugly way to calculate the change, but it stops
duplication of checking for the action for each status.
*/
@@ -314,28 +318,28 @@
node_counter = -1;
}
- /* TODO: The commissioned status currently doesn't exist, but once it
- does it should be added here too.
- */
- if (status === 0) {
+ switch (status) {
+ case NODE_STATUS.DECLARED:
// Added nodes
this.added_nodes += node_counter;
this.chart.set('added_nodes', this.added_nodes);
update_chart = true;
- }
- else if (status === 1 || status === 2 || status === 3) {
+ break;
+ case NODE_STATUS.COMMISSIONING:
+ case NODE_STATUS.FAILED_TESTS:
+ case NODE_STATUS.MISSING:
// Offline nodes
this.offline_nodes += node_counter;
this.chart.set('offline_nodes', this.offline_nodes);
update_chart = true;
- }
- else if (status === 4) {
+ break;
+ case NODE_STATUS.READY:
// Queued nodes
this.queued_nodes += node_counter;
this.chart.set('queued_nodes', this.queued_nodes);
update_chart = true;
- }
- else if (status === 5) {
+ break;
+ case NODE_STATUS.RESERVED:
// Reserved nodes
this.reserved_nodes += node_counter;
this.setNodeText(
@@ -343,18 +347,19 @@
this.reserved_template,
this.reserved_nodes
);
- }
- else if (status === 6) {
+ break;
+ case NODE_STATUS.ALLOCATED:
// Deployed nodes
this.deployed_nodes += node_counter;
this.chart.set('deployed_nodes', this.deployed_nodes);
update_chart = true;
- }
- else if (status === 7) {
+ break;
+ case NODE_STATUS.RETIRED:
// Retired nodes
this.retired_nodes += node_counter;
this.setNodeText(
this.retiredNode, this.retired_template, this.retired_nodes);
+ break;
}
return update_chart;
@@ -406,6 +411,6 @@
});
}, '0.1', {'requires': [
- 'view', 'io', 'maas.node', 'maas.node_add', 'maas.nodes_chart',
- 'maas.morph', 'anim']}
+ 'view', 'io', 'maas.enums', 'maas.node', 'maas.node_add',
+ 'maas.nodes_chart', 'maas.morph', 'anim']}
);
=== added file 'src/maasserver/static/js/tests/test_enums.html'
--- src/maasserver/static/js/tests/test_enums.html 1970-01-01 00:00:00 +0000
+++ src/maasserver/static/js/tests/test_enums.html 2012-04-30 16:33:20 +0000
@@ -0,0 +1,31 @@
+<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd";>
+<html xmlns="http://www.w3.org/1999/xhtml"; xml:lang="en" lang="en">
+ <head>
+ <title>Test maas.enums</title>
+
+ <!-- YUI and test setup -->
+ <script type="text/javascript" src="../testing/yui_test_conf.js"></script>
+ <script type="text/javascript" src="../../jslibs/yui/tests/build/yui/yui.js"></script>
+ <script type="text/javascript" src="../testing/testrunner.js"></script>
+ <script type="text/javascript" src="../testing/testing.js"></script>
+ <script type="text/javascript" src="../node.js"></script>
+ <!-- The module under test -->
+ <script type="text/javascript" src="../enums.js"></script>
+ <!-- The test suite -->
+ <script type="text/javascript" src="test_enums.js"></script>
+ <script type="text/javascript">
+ <!--
+ var MAAS_config = {
+ uris: {
+ statics: '/static/',
+ nodes_handler: '/api/nodes/'
+ }
+ };
+ // -->
+ </script>
+ </head>
+ <body>
+ <span id="suite">maas.enums.tests</span>
+ <div id="placeholder"></div>
+ </body>
+</html>
=== added file 'src/maasserver/static/js/tests/test_enums.js'
--- src/maasserver/static/js/tests/test_enums.js 1970-01-01 00:00:00 +0000
+++ src/maasserver/static/js/tests/test_enums.js 2012-04-30 16:33:20 +0000
@@ -0,0 +1,37 @@
+/* Copyright 2012 Canonical Ltd. This software is licensed under the
+ * GNU Affero General Public License version 3 (see the file LICENSE).
+ */
+
+YUI({ useBrowserConsole: true }).add('maas.enums.tests', function(Y) {
+
+Y.log('loading maas.enums.tests');
+var namespace = Y.namespace('maas.enums.tests');
+
+var module = Y.maas.enums;
+var suite = new Y.Test.Suite("maas.enums Tests");
+
+suite.add(new Y.maas.testing.TestCase({
+ name: 'test-enums',
+
+ testDefinesEnums: function() {
+ Y.Assert.isObject(Y.maas.enums.NODE_STATUS);
+ },
+
+ testHasEnumValues: function() {
+ Y.Assert.isNotUndefined(Y.maas.enums.NODE_STATUS.READY);
+ Y.Assert.isNotNull(Y.maas.enums.NODE_STATUS.READY);
+ },
+
+ testDistinguishesValues: function() {
+ Y.Assert.areNotEqual(
+ Y.maas.enums.NODE_STATUS.READY,
+ Y.maas.enums.NODE_STATUS.RETIRED,
+ "Different values of an enum were equal somehow.")
+ }
+}));
+
+namespace.suite = suite;
+
+}, '0.1', {'requires': [
+ 'node-event-simulate', 'test', 'maas.testing', 'maas.enums']}
+);
=== modified file 'src/maasserver/static/js/tests/test_node_views.html'
--- src/maasserver/static/js/tests/test_node_views.html 2012-03-21 04:58:53 +0000
+++ src/maasserver/static/js/tests/test_node_views.html 2012-04-30 16:33:20 +0000
@@ -9,6 +9,7 @@
<script type="text/javascript" src="../../jslibs/yui/tests/build/yui/yui.js"></script>
<script type="text/javascript" src="../testing/testrunner.js"></script>
<script type="text/javascript" src="../testing/testing.js"></script>
+ <script type="text/javascript" src="../enums.js"></script>
<script type="text/javascript" src="../node.js"></script>
<script type="text/javascript" src="../node_add.js"></script>
<script type="text/javascript" src="../nodes_chart.js"></script>
=== modified file 'src/maasserver/static/js/tests/test_node_views.js'
--- src/maasserver/static/js/tests/test_node_views.js 2012-04-17 16:27:21 +0000
+++ src/maasserver/static/js/tests/test_node_views.js 2012-04-30 16:33:20 +0000
@@ -463,5 +463,6 @@
namespace.suite = suite;
}, '0.1', {'requires': [
- 'node-event-simulate', 'test', 'maas.testing', 'maas.node_views']}
+ 'node-event-simulate', 'test', 'maas.testing', 'maas.enums',
+ 'maas.node_views']}
);
=== modified file 'src/maasserver/templates/maasserver/index.html'
--- src/maasserver/templates/maasserver/index.html 2012-04-05 06:23:59 +0000
+++ src/maasserver/templates/maasserver/index.html 2012-04-30 16:33:20 +0000
@@ -10,11 +10,12 @@
{% block head %}
<script type="text/javascript" src="{{ STATIC_URL }}jslibs/raphael/raphael-min.js"></script>
<script type="text/javascript" src="{{ STATIC_URL }}js/nodes_chart.js"></script>
+ <script type="text/javascript" src="{{ STATIC_URL }}js/enums.js"></script>
<script type="text/javascript">
<!--
YUI().use(
- 'maas.node_add', 'maas.node','maas.node_views', 'maas.utils',
- 'maas.longpoll',
+ 'maas.enums', 'maas.node_add', 'maas.node','maas.node_views',
+ 'maas.utils', 'maas.longpoll',
function (Y) {
Y.on('load', function() {
// Create Dashboard view.
=== modified file 'src/maastesting/matchers.py'
--- src/maastesting/matchers.py 2012-04-30 16:33:19 +0000
+++ src/maastesting/matchers.py 2012-04-30 16:33:20 +0000
@@ -12,7 +12,7 @@
__metaclass__ = type
__all__ = [
'ContainsAll',
-]
+ ]
from testtools.matchers import (
Contains,
@@ -21,8 +21,8 @@
def ContainsAll(items):
+ """Matches if the matchee contains all the provided items."""
# XXX: rvb 2012-04-30 bug=991743: This matcher has been submitted
# upstream. If it gets included in the next version of testtools, this code
# should be removed.
- """Matches if the matchee contains all the provided items."""
return MatchesAll(*[Contains(item) for item in items], first_only=False)
=== added file 'utilities/convert-enums.py'
--- utilities/convert-enums.py 1970-01-01 00:00:00 +0000
+++ utilities/convert-enums.py 2012-04-30 16:33:20 +0000
@@ -0,0 +1,160 @@
+#!/usr/bin/env python2.7
+# Copyright 2012 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Generate JavaScript enum definitions based on their python definitions.
+
+MAAS defines its enums as simple classes, with the enum items as attributes.
+Running this script produces a source text containing the JavaScript
+equivalents of the same enums, so that JavaScript code can make use of them.
+
+The script takes one option: --src=DIRECTORY. DIRECTORY is where the MAAS
+modules (maasserver, metadataserver, provisioningserver) can be found.
+
+The resulting JavaScript module is printed to standard output.
+"""
+
+from __future__ import (
+ absolute_import,
+ print_function,
+ unicode_literals,
+ )
+
+__metaclass__ = type
+
+from argparse import ArgumentParser
+from datetime import datetime
+from imp import (
+ find_module,
+ load_module,
+ )
+import json
+from operator import itemgetter
+import os.path
+import sys
+from textwrap import dedent
+
+# Header. Will be written on top of the output.
+header = dedent("""\
+ /*
+ Generated file. DO NOT EDIT.
+
+ This file was generated by %(script)s,
+ on %(timestamp)s.
+ */
+
+ YUI.add('maas.enums', function(Y) {
+ Y.log('loading maas.enums');
+ var module = Y.namespace('maas.enums');
+ """
+ % {'script': sys.argv[0], 'timestamp': datetime.now()})
+
+
+footer = dedent("""
+ }, '0.1');
+ """)
+
+
+def get_module(src_path, package, name='enum'):
+ """Attempt to load a given module.
+
+ This makes some assumptions about directory structure: it is assumed
+ that the module is directly in a package of the given name, which in turn
+ should be directly in the search path.
+
+ :param src_path: The path to search in.
+ :param package: The package to load the requested module from.
+ :param name: Name of module to load.
+ :return: The imported module, or None if it was not found.
+ """
+ path = os.path.join(src_path, package)
+ try:
+ found_module = find_module(name, [path])
+ except ImportError:
+ # No enum module here. Ignore this package or directory.
+ return None
+ return load_module(name, *found_module)
+
+
+def find_enum_modules(src_path):
+ """Find MAAS "enum" modules in the packages in src_path.
+
+ This assumes that all MAAS enums can be found in
+ <src_path>/<package>/enum.py.
+
+ :param src_path: The path to search in.
+ :return: A list of "enum" modules found in packages in src_path.
+ """
+ return filter(None, [
+ get_module(src_path, package, 'enum')
+ for package in os.listdir(src_path)])
+
+
+def is_enum(item):
+ """Does the given python item look like an enum?
+
+ :param item: An item imported from a MAAS enum module.
+ :return: Bool.
+ """
+ return isinstance(item, type) and item.__name__ == item.__name__.upper()
+
+
+def get_enum_classes(module):
+ """Collect all enum classes exported from loaded `module`."""
+ return filter(is_enum, (module.__dict__[name] for name in module.__all__))
+
+
+def serialize_value(value):
+ """Represent an enumeration item's value in JavaScript."""
+ if value is None:
+ return 'null'
+ elif isinstance(value, basestring):
+ return json.dumps(value)
+ else:
+ return value
+
+
+def serialize_item(key, value):
+ """Represent one key/value pair as a line in a JavaScript enum."""
+ return " %s: %s" % (key, serialize_value(value))
+
+
+def serialize_dict_items(enum):
+ """Represent the items of a dict as a block of JavaScript."""
+ items = sorted(enum.items(), key=itemgetter(1))
+ return ',\n'.join(serialize_item(key, value) for key, value in items)
+
+
+def serialize_enum(enum):
+ """Represent a MAAS enum class in JavaScript."""
+ # Import lazily to make use of initialized path.
+ from maasserver.utils import map_enum
+
+ head = "module.%s = {\n" % enum.__name__
+ foot = "\n};"
+ return head + serialize_dict_items(map_enum(enum)) + foot
+
+
+def parse_args():
+ """Parse options & arguments."""
+ default_src = os.path.join(os.path.dirname(sys.path[0]), 'src')
+ parser = ArgumentParser(
+ "Generate JavaScript enums based on python enums modules")
+ parser.add_argument(
+ '--src', metavar='SOURCE_DIRECTORY', type=str, default=default_src,
+ help="Look for the MAAS packages in SOURCE_DIRECTORY.")
+ return parser.parse_args()
+
+
+def main(args):
+ enum_modules = find_enum_modules(args.src)
+ enums = sum((get_enum_classes(module) for module in enum_modules), [])
+ dumps = (serialize_enum(enum) for enum in enums)
+ print(header + "\n\n".join(dumps) + footer)
+
+
+if __name__ == "__main__":
+ args = parse_args()
+ # Add src directory so that we can import from MAAS packages.
+ sys.path.append(args.src)
+ main(args)
Follow ups
