Bug 1326547: replace get_keyed_by with resolve_keyed_by; r=Callek

MozReview-Commit-ID: FS1nbVyykXV

--HG--
extra : rebase_source : 6cddb3ecb188f2c9c6dafee98c3b0ed5bfdf1955

taskcluster/docs/transforms.rst

@@ -73,7 +73,7 @@ This is a simple but powerful way to encode business rules in the items
 provided as input to the transforms, rather than expressing those rules in the
 transforms themselves.  If you are implementing a new business rule, prefer
 this mode where possible.  The structure is easily resolved to a single value
-using :func:`taskgraph.transform.base.get_keyed_by`.
+using :func:`taskgraph.transform.base.resolve_keyed_by`.
 
 Organization
 -------------

taskcluster/taskgraph/test/test_transforms_base.py

@@ -8,7 +8,7 @@ import unittest
 from mozunit import main
 from taskgraph.transforms.base import (
     validate_schema,
-    get_keyed_by,
+    resolve_keyed_by,
     TransformSequence
 )
 from voluptuous import Schema
@@ -59,85 +59,69 @@ class TestValidateSchema(unittest.TestCase):
             self.failUnless(str(e).startswith("pfx\n"))
 
 
-class TestKeyedBy(unittest.TestCase):
+class TestResolveKeyedBy(unittest.TestCase):
 
-    def test_simple_value(self):
-        test = {
-            'test-name': 'tname',
-            'option': 10,
-        }
-        self.assertEqual(get_keyed_by(test, 'option', 'x'), 10)
+    def test_no_by(self):
+        self.assertEqual(
+            resolve_keyed_by({'x': 10}, 'z', 'n'),
+            {'x': 10})
 
-    def test_by_value(self):
-        test = {
-            'test-name': 'tname',
-            'option': {
-                'by-other-value': {
-                    'a': 10,
-                    'b': 20,
-                },
-            },
-            'other-value': 'b',
-        }
-        self.assertEqual(get_keyed_by(test, 'option', 'x'), 20)
+    def test_no_by_dotted(self):
+        self.assertEqual(
+            resolve_keyed_by({'x': {'y': 10}}, 'x.z', 'n'),
+            {'x': {'y': 10}})
 
-    def test_by_value_regex(self):
-        test = {
-            'test-name': 'tname',
-            'option': {
-                'by-test-platform': {
-                    'macosx64/.*': 10,
-                    'linux64/debug': 20,
-                    'default': 5,
-                },
-            },
-            'test-platform': 'macosx64/debug',
-        }
-        self.assertEqual(get_keyed_by(test, 'option', 'x'), 10)
+    def test_no_by_not_dict(self):
+        self.assertEqual(
+            resolve_keyed_by({'x': 10}, 'x.y', 'n'),
+            {'x': 10})
 
-    def test_by_value_default(self):
-        test = {
-            'test-name': 'tname',
-            'option': {
-                'by-other-value': {
-                    'a': 10,
-                    'default': 30,
-                },
-            },
-            'other-value': 'xxx',
-        }
-        self.assertEqual(get_keyed_by(test, 'option', 'x'), 30)
+    def test_no_by_not_by(self):
+        self.assertEqual(
+            resolve_keyed_by({'x': {'a': 10}}, 'x', 'n'),
+            {'x': {'a': 10}})
 
-    def test_by_value_dict(self):
-        test = {
-            'test-name': 'tname',
-            'option': {
-                'by-something-else': {},
-                'by-other-value': {},
-            },
-        }
-        self.assertEqual(get_keyed_by(test, 'option', 'x'), test['option'])
+    def test_no_by_empty_dict(self):
+        self.assertEqual(
+            resolve_keyed_by({'x': {}}, 'x', 'n'),
+            {'x': {}})
 
-    def test_by_value_invalid_no_default(self):
-        test = {
-            'test-name': 'tname',
-            'option': {
-                'by-other-value': {
-                    'a': 10,
-                },
-            },
-            'other-value': 'b',
-        }
-        self.assertRaises(Exception, get_keyed_by, test, 'option', 'x')
+    def test_no_by_not_only_by(self):
+        self.assertEqual(
+            resolve_keyed_by({'x': {'by-y': True, 'a': 10}}, 'x', 'n'),
+            {'x': {'by-y': True, 'a': 10}})
+
+    def test_match_nested_exact(self):
+        self.assertEqual(
+            resolve_keyed_by(
+                {'f': 'shoes', 'x': {'y': {'by-f': {'shoes': 'feet', 'gloves': 'hands'}}}},
+                'x.y', 'n'),
+            {'f': 'shoes', 'x': {'y': 'feet'}})
+
+    def test_match_regexp(self):
+        self.assertEqual(
+            resolve_keyed_by(
+                {'f': 'shoes', 'x': {'by-f': {'s?[hH]oes?': 'feet', 'gloves': 'hands'}}},
+                'x', 'n'),
+            {'f': 'shoes', 'x': 'feet'})
+
+    def test_match_default(self):
+        self.assertEqual(
+            resolve_keyed_by(
+                {'f': 'shoes', 'x': {'by-f': {'hat': 'head', 'default': 'anywhere'}}},
+                'x', 'n'),
+            {'f': 'shoes', 'x': 'anywhere'})
+
+    def test_no_match(self):
+        self.assertRaises(
+            Exception, resolve_keyed_by,
+            {'f': 'shoes', 'x': {'by-f': {'hat': 'head'}}}, 'x', 'n')
+
+    def test_multiple_matches(self):
+        self.assertRaises(
+            Exception, resolve_keyed_by,
+            {'f': 'hats', 'x': {'by-f': {'hat.*': 'head', 'ha.*': 'hair'}}}, 'x', 'n')
 
-    def test_by_value_no_by(self):
-        test = {
-            'test-name': 'tname',
-            'option': {
-                'other-value': {},
-            },
-        }
-        self.assertEqual(get_keyed_by(test, 'option', 'x'), test['option'])
 
 if __name__ == '__main__':
     main()

taskcluster/taskgraph/transforms/base.py

@@ -93,57 +93,70 @@ def optionally_keyed_by(*arguments):
     return voluptuous.Any(*options)
 
 
-def get_keyed_by(item, field, item_name, subfield=None):
+def resolve_keyed_by(item, field, item_name):
     """
     For values which can either accept a literal value, or be keyed by some
-    other attribute of the item, perform that lookup.  For example, this supports
+    other attribute of the item, perform that lookup and replacement in-place
+    (modifying `item` directly).  The field is specified using dotted notation
+    to traverse dictionaries.
 
-        chunks:
-            by-test-platform:
-                macosx-10.11/debug: 13
-                win.*: 6
-                default: 12
+    For example, given item
+
+        job:
+            chunks:
+                by-test-platform:
+                    macosx-10.11/debug: 13
+                    win.*: 6
+                    default: 12
+
+    a call to `resolve_keyed_by(item, 'job.chunks', item['thing-name'])
+    would mutate item in-place to
+
+        job:
+            chunks: 12
 
     The `item_name` parameter is used to generate useful error messages.
-    The `subfield` parameter, if specified, allows access to a second level
-    of the item dictionary: item[field][subfield]. For example, this supports
-
-        mozharness:
-            config:
-                by-test-platform:
-                    default: ...
     """
-    value = item[field]
-    if not isinstance(value, dict):
-        return value
-    if subfield:
-        value = item[field][subfield]
-        if not isinstance(value, dict):
-            return value
+    # find the field, returning the item unchanged if anything goes wrong
+    container, subfield = item, field
+    while '.' in subfield:
+        f, subfield = subfield.split('.', 1)
+        if f not in container:
+            return item
+        container = container[f]
+        if not isinstance(container, dict):
+            return item
 
-    keyed_by = value.keys()[0]
-    if len(value) > 1 or not keyed_by.startswith('by-'):
-        return value
+    if subfield not in container:
+        return item
+    value = container[subfield]
+    if not isinstance(value, dict) or len(value) != 1 or not value.keys()[0].startswith('by-'):
+        return item
 
-    values = value[keyed_by]
-    keyed_by = keyed_by[3:]  # strip 'by-' off the keyed-by field name
-    if item[keyed_by] in values:
-        return values[item[keyed_by]]
+    keyed_by = value.keys()[0][3:]  # strip off 'by-' prefix
+    key = item[keyed_by]
+    alternatives = value.values()[0]
 
-    matches = [(k, v) for k, v in values.iteritems() if re.match(k, item[keyed_by])]
+    # exact match
+    if key in alternatives:
+        container[subfield] = alternatives[key]
+        return item
+
+    # regular expression match
+    matches = [(k, v) for k, v in alternatives.iteritems() if re.match(k, key)]
     if len(matches) > 1:
         raise Exception(
             "Multiple matching values for {} {!r} found while determining item {} in {}".format(
-                keyed_by, item[keyed_by], field, item_name))
+                keyed_by, key, field, item_name))
     elif matches:
-        return matches[0][1]
+        container[subfield] = matches[0][1]
+        return item
 
-    if 'default' in values:
-        return values['default']
-    for k in item[keyed_by], 'default':
-        if k in values:
-            return values[k]
-    else:
-        raise Exception(
-            "No {} matching {!r} nor 'default' found while determining item {} in {}".format(
-                keyed_by, item[keyed_by], field, item_name))
+    # default
+    if 'default' in alternatives:
+        container[subfield] = alternatives['default']
+        return item
+
+    raise Exception(
+        "No {} matching {!r} nor 'default' found while determining item {} in {}".format(
+            keyed_by, key, field, item_name))

taskcluster/taskgraph/transforms/job/__init__.py

@@ -15,7 +15,7 @@ import copy
 import logging
 import os
 
-from taskgraph.transforms.base import get_keyed_by, validate_schema, TransformSequence
+from taskgraph.transforms.base import resolve_keyed_by, validate_schema, TransformSequence
 from taskgraph.transforms.task import task_description_schema
 from voluptuous import (
     Any,
@@ -104,7 +104,7 @@ def expand_platforms(config, jobs):
 
 
 @transforms.add
-def resolve_keyed_by(config, jobs):
+def handle_keyed_by(config, jobs):
     fields = [
         'worker-type',
         'worker',
@@ -112,7 +112,7 @@ def resolve_keyed_by(config, jobs):
 
     for job in jobs:
         for field in fields:
-            job[field] = get_keyed_by(item=job, field=field, item_name=job['name'])
+            resolve_keyed_by(job, field, item_name=job['name'])
         yield job
 
 

taskcluster/taskgraph/transforms/tests.py

@@ -19,7 +19,7 @@ for example - use `all_tests.py` instead.
 
 from __future__ import absolute_import, print_function, unicode_literals
 
-from taskgraph.transforms.base import TransformSequence, get_keyed_by
+from taskgraph.transforms.base import TransformSequence, resolve_keyed_by
 from taskgraph.util.treeherder import split_symbol, join_symbol
 from taskgraph.transforms.job.common import (
     docker_worker_support_vcs_checkout,
@@ -296,12 +296,10 @@ def validate(config, tests):
 
 
 @transforms.add
-def resolve_keyed_by_mozharness(config, tests):
+def handle_keyed_by_mozharness(config, tests):
     """Resolve a mozharness field if it is keyed by something"""
     for test in tests:
-        test['mozharness'] = get_keyed_by(
-            item=test, field='mozharness',
-            item_name=test['test-name'])
+        resolve_keyed_by(test, 'mozharness', item_name=test['test-name'])
         yield test
 
 
@@ -408,7 +406,7 @@ def set_tier(config, tests):
     specify a tier otherwise."""
     for test in tests:
         if 'tier' in test:
-            test['tier'] = get_keyed_by(item=test, field='tier', item_name=test['test-name'])
+            resolve_keyed_by(test, 'tier', item_name=test['test-name'])
 
         # only override if not set for the test
         if 'tier' not in test or test['tier'] == 'default':
@@ -454,7 +452,7 @@ def set_download_symbols(config, tests):
 
 
 @transforms.add
-def resolve_keyed_by(config, tests):
+def handle_keyed_by(config, tests):
     """Resolve fields that can be keyed by platform, etc."""
     fields = [
         'instance-size',
@@ -465,17 +463,12 @@ def resolve_keyed_by(config, tests):
         'suite',
         'run-on-projects',
         'os-groups',
-    ]
-    mozharness_fields = [
-        'config',
-        'extra-options',
+        'mozharness.config',
+        'mozharness.extra-options',
     ]
     for test in tests:
         for field in fields:
-            test[field] = get_keyed_by(item=test, field=field, item_name=test['test-name'])
-        for subfield in mozharness_fields:
-            test['mozharness'][subfield] = get_keyed_by(
-                item=test, field='mozharness', subfield=subfield, item_name=test['test-name'])
+            resolve_keyed_by(test, field, item_name=test['test-name'])
         yield test
 
 
@@ -499,10 +492,6 @@ def split_e10s(config, tests):
             if group != '?':
                 group += '-e10s'
             test['treeherder-symbol'] = join_symbol(group, symbol)
-            test['mozharness']['extra-options'] = get_keyed_by(item=test,
-                                                               field='mozharness',
-                                                               subfield='extra-options',
-                                                               item_name=test['test-name'])
             test['mozharness']['extra-options'].append('--e10s')
         yield test