Bug 1535056 - Validate taskgraph parameters using a schema r=tomprince

Validate taskgraph parameters using a schema.

Previously, parameters were verified using handwritten comparison to a sample set of parameters.
Switch to using an explicit schema instead.

Differential Revision: https://phabricator.services.mozilla.com/D23756

--HG--
extra : moz-landing-system : lando

taskcluster/taskgraph/parameters.py

@@ -8,11 +8,19 @@ from __future__ import absolute_import, print_function, unicode_literals
 
 import os.path
 import json
-import time
 from datetime import datetime
 
 from mozbuild.util import ReadOnlyDict, memoize
 from mozversioncontrol import get_repository_object
+from taskgraph.util.schema import validate_schema
+from voluptuous import (
+    ALLOW_EXTRA,
+    Any,
+    Inclusive,
+    PREVENT_EXTRA,
+    Required,
+    Schema,
+)
 
 from . import GECKO
 from .util.attributes import release_level
@@ -45,55 +53,58 @@ def get_app_version(product_dir='browser'):
     return get_contents(app_version_path)
 
 
-# Please keep this list sorted and in sync with taskcluster/docs/parameters.rst
+base_schema = {
-# Parameters are of the form: {name: default} or {name: lambda: default}
+    Required('app_version'): basestring,
-PARAMETERS = {
+    Required('base_repository'): basestring,
-    'app_version': get_app_version(),
+    Required('build_date'): int,
-    'base_repository': 'https://hg.mozilla.org/mozilla-unified',
+    Required('build_number'): int,
-    'build_date': lambda: int(time.time()),
+    Inclusive('comm_base_repository', 'comm'): basestring,
-    'build_number': 1,
+    Inclusive('comm_head_ref', 'comm'): basestring,
-    'do_not_optimize': [],
+    Inclusive('comm_head_repository', 'comm'): basestring,
-    'existing_tasks': {},
+    Inclusive('comm_head_rev', 'comm'): basestring,
-    'filters': ['target_tasks_method'],
+    Required('do_not_optimize'): [basestring],
-    'head_ref': get_head_ref,
+    Required('existing_tasks'): {basestring: basestring},
-    'head_repository': 'https://hg.mozilla.org/mozilla-central',
+    Required('filters'): [basestring],
-    'head_rev': get_head_ref,
+    Required('head_ref'): basestring,
-    'hg_branch': 'default',
+    Required('head_repository'): basestring,
-    'level': '3',
+    Required('head_rev'): basestring,
-    'message': '',
+    Required('hg_branch'): basestring,
-    'moz_build_date': lambda: datetime.now().strftime("%Y%m%d%H%M%S"),
+    Required('level'): basestring,
-    'next_version': None,
+    Required('message'): basestring,
-    'optimize_target_tasks': True,
+    Required('moz_build_date'): basestring,
-    'owner': 'nobody@mozilla.com',
+    Required('next_version'): Any(None, basestring),
-    'project': 'mozilla-central',
+    Required('optimize_target_tasks'): bool,
-    'pushdate': lambda: int(time.time()),
+    Required('owner'): basestring,
-    'pushlog_id': '0',
+    Required('phabricator_diff'): Any(None, basestring),
-    'phabricator_diff': None,
+    Required('project'): basestring,
-    'release_enable_emefree': False,
+    Required('pushdate'): int,
-    'release_enable_partners': False,
+    Required('pushlog_id'): unicode,
-    'release_eta': '',
+    Required('release_enable_emefree'): bool,
-    'release_history': {},
+    Required('release_enable_partners'): bool,
-    'release_partners': None,
+    Required('release_eta'): Any(None, basestring),
-    'release_partner_config': None,
+    Required('release_history'): {basestring: [dict]},
-    'release_partner_build_number': 1,
+    Required('release_partners'): [basestring],
-    'release_type': 'nightly',
+    Required('release_partner_config'): Any(None, dict),
-    'release_product': None,
+    Required('release_partner_build_number'): int,
-    'required_signoffs': [],
+    Required('release_type'): basestring,
-    'signoff_urls': {},
+    Required('release_product'): Any(None, basestring),
-    'target_tasks_method': 'default',
+    Required('required_signoffs'): [basestring],
-    'tasks_for': 'hg-push',
+    Required('signoff_urls'): dict,
-    'try_mode': None,
+    Required('target_tasks_method'): Any(None, basestring),
-    'try_options': None,
+    Required('tasks_for'): Any(None, basestring),
-    'try_task_config': None,
+    Required('try_mode'): Any(None, basestring),
-    'version': get_version(),
+    Required('try_options'): Any(None, dict),
+    Required('try_task_config'): Any(None, dict),
+    Required('version'): basestring,
 }
 
-COMM_PARAMETERS = {
+
-    'comm_base_repository': 'https://hg.mozilla.org/comm-central',
+COMM_PARAMETERS = [
-    'comm_head_ref': None,
+    'comm_base_repository',
-    'comm_head_repository': 'https://hg.mozilla.org/comm-central',
+    'comm_head_ref',
-    'comm_head_rev': None,
+    'comm_head_repository',
-}
+    'comm_head_rev',
+]
 
 
 class Parameters(ReadOnlyDict):
@@ -104,49 +115,74 @@ class Parameters(ReadOnlyDict):
 
         if not self.strict:
             # apply defaults to missing parameters
-            for name, default in PARAMETERS.items():
+            kwargs = Parameters._fill_defaults(**kwargs)
-                if name not in kwargs:
-                    if callable(default):
-                        default = default()
-                    kwargs[name] = default
-
-            if set(kwargs) & set(COMM_PARAMETERS.keys()):
-                for name, default in COMM_PARAMETERS.items():
-                    if name not in kwargs:
-                        if callable(default):
-                            default = default()
-                        kwargs[name] = default
 
         ReadOnlyDict.__init__(self, **kwargs)
 
+    @staticmethod
+    def _fill_defaults(**kwargs):
+        now = datetime.utcnow()
+        epoch = datetime.utcfromtimestamp(0)
+        seconds_from_epoch = int((now - epoch).total_seconds())
+
+        defaults = {
+            'app_version': get_app_version(),
+            'base_repository': 'https://hg.mozilla.org/mozilla-unified',
+            'build_date': seconds_from_epoch,
+            'build_number': 1,
+            'do_not_optimize': [],
+            'existing_tasks': {},
+            'filters': ['target_tasks_method'],
+            'head_ref': get_head_ref(),
+            'head_repository': 'https://hg.mozilla.org/mozilla-central',
+            'head_rev': get_head_ref(),
+            'hg_branch': 'default',
+            'level': '3',
+            'message': '',
+            'moz_build_date': now.strftime("%Y%m%d%H%M%S"),
+            'next_version': None,
+            'optimize_target_tasks': True,
+            'owner': 'nobody@mozilla.com',
+            'phabricator_diff': None,
+            'project': 'mozilla-central',
+            'pushdate': seconds_from_epoch,
+            'pushlog_id': '0',
+            'release_enable_emefree': False,
+            'release_enable_partners': False,
+            'release_eta': '',
+            'release_history': {},
+            'release_partners': [],
+            'release_partner_config': None,
+            'release_partner_build_number': 1,
+            'release_product': None,
+            'release_type': 'nightly',
+            'required_signoffs': [],
+            'signoff_urls': {},
+            'target_tasks_method': 'default',
+            'tasks_for': 'hg-push',
+            'try_mode': None,
+            'try_options': None,
+            'try_task_config': None,
+            'version': get_version(),
+        }
+
+        if set(COMM_PARAMETERS) & set(kwargs):
+            defaults.update({
+                'comm_base_repository': 'https://hg.mozilla.org/comm-central',
+                'comm_head_repository': 'https://hg.mozilla.org/comm-central',
+            })
+
+        for name, default in defaults.items():
+            if name not in kwargs:
+                kwargs[name] = default
+
+        return kwargs
+
     def check(self):
-        names = set(self)
+        schema = Schema(base_schema, extra=PREVENT_EXTRA if self.strict else ALLOW_EXTRA)
-        valid = set(PARAMETERS.keys())
+        validate_schema(schema, self.copy(), 'Invalid parameters:')
-        valid_comm = set(COMM_PARAMETERS.keys())
-        msg = []
-
-        missing = valid - names
-        if missing:
-            msg.append("missing parameters: " + ", ".join(missing))
-
-        extra = names - valid
-
-        if extra & set(valid_comm):
-            # If any comm_* parameters are specified, ensure all of them are specified.
-            missing = valid_comm - extra
-            if missing:
-                msg.append("missing parameters: " + ", ".join(missing))
-            extra = extra - valid_comm
-
-        if extra and self.strict:
-            msg.append("extra parameters: " + ", ".join(extra))
-
-        if msg:
-            raise ParameterMismatch("; ".join(msg))
 
     def __getitem__(self, k):
-        if not (k in PARAMETERS.keys() or k in COMM_PARAMETERS.keys()):
-            raise KeyError("no such parameter {!r}".format(k))
         try:
             return super(Parameters, self).__getitem__(k)
         except KeyError:

taskcluster/taskgraph/test/test_decision.py

@@ -59,11 +59,11 @@ class TestGetDecisionParameters(unittest.TestCase):
             'head_ref': 'ef01',
             'message': '',
             'project': 'mozilla-central',
-            'pushlog_id': 143,
+            'pushlog_id': '143',
             'pushdate': 1503691511,
             'owner': 'nobody@mozilla.com',
             'tasks_for': 'hg-push',
-            'level': 3,
+            'level': '3',
         }
 
     @patch('taskgraph.decision.get_hg_revision_branch')
@@ -71,7 +71,7 @@ class TestGetDecisionParameters(unittest.TestCase):
         mock_get_hg_revision_branch.return_value = 'default'
         with MockedOpen({self.ttc_file: None}):
             params = decision.get_decision_parameters(FAKE_GRAPH_CONFIG, self.options)
-        self.assertEqual(params['pushlog_id'], 143)
+        self.assertEqual(params['pushlog_id'], '143')
         self.assertEqual(params['build_date'], 1503691511)
         self.assertEqual(params['hg_branch'], 'default')
         self.assertEqual(params['moz_build_date'], '20170825200511')
@@ -80,7 +80,8 @@ class TestGetDecisionParameters(unittest.TestCase):
         self.assertEqual(params['try_task_config'], None)
 
     @patch('taskgraph.decision.get_hg_revision_branch')
-    def test_no_email_owner(self, _):
+    def test_no_email_owner(self, mock_get_hg_revision_branch):
+        mock_get_hg_revision_branch.return_value = 'default'
         self.options['owner'] = 'ffxbld'
         with MockedOpen({self.ttc_file: None}):
             params = decision.get_decision_parameters(FAKE_GRAPH_CONFIG, self.options)
@@ -88,8 +89,9 @@ class TestGetDecisionParameters(unittest.TestCase):
 
     @patch('taskgraph.decision.get_hg_revision_branch')
     @patch('taskgraph.decision.get_hg_commit_message')
-    def test_try_options(self, mock_get_hg_commit_message, _):
+    def test_try_options(self, mock_get_hg_commit_message, mock_get_hg_revision_branch):
         mock_get_hg_commit_message.return_value = 'try: -b do -t all'
+        mock_get_hg_revision_branch.return_value = 'default'
         self.options['project'] = 'try'
         with MockedOpen({self.ttc_file: None}):
             params = decision.get_decision_parameters(FAKE_GRAPH_CONFIG, self.options)
@@ -100,8 +102,9 @@ class TestGetDecisionParameters(unittest.TestCase):
 
     @patch('taskgraph.decision.get_hg_revision_branch')
     @patch('taskgraph.decision.get_hg_commit_message')
-    def test_try_task_config(self, mock_get_hg_commit_message, _):
+    def test_try_task_config(self, mock_get_hg_commit_message, mock_get_hg_revision_branch):
         mock_get_hg_commit_message.return_value = 'Fuzzy query=foo'
+        mock_get_hg_revision_branch.return_value = 'default'
         ttc = {'tasks': ['a', 'b'], 'templates': {}}
         self.options['project'] = 'try'
         with MockedOpen({self.ttc_file: json.dumps(ttc)}):

taskcluster/taskgraph/test/test_parameters.py

@@ -8,17 +8,53 @@ import unittest
 
 from taskgraph.parameters import (
     Parameters,
-    ParameterMismatch,
     load_parameters_file,
-    PARAMETERS,
-    COMM_PARAMETERS,
 )
 from mozunit import main, MockedOpen
 
 
 class TestParameters(unittest.TestCase):
 
-    vals = {n: n for n in PARAMETERS.keys()}
+    vals = {
+        'app_version': 'app_version',
+        'base_repository': 'base_repository',
+        'build_date': 0,
+        'build_number': 0,
+        'do_not_optimize': [],
+        'existing_tasks': {},
+        'filters': [],
+        'head_ref': 'head_ref',
+        'head_repository': 'head_repository',
+        'head_rev': 'head_rev',
+        'hg_branch': 'hg_branch',
+        'level': 'level',
+        'message': 'message',
+        'moz_build_date': 'moz_build_date',
+        'next_version': 'next_version',
+        'optimize_target_tasks': False,
+        'owner': 'owner',
+        'phabricator_diff': 'phabricator_diff',
+        'project': 'project',
+        'pushdate': 0,
+        'pushlog_id': 'pushlog_id',
+        'release_enable_emefree': False,
+        'release_enable_partners': False,
+        'release_eta': None,
+        'release_history': {},
+        'release_partners': [],
+        'release_partner_config': None,
+        'release_partner_build_number': 1,
+        'release_type': 'release_type',
+        'release_product': None,
+        'required_signoffs': [],
+        'signoff_urls': {},
+        'target_tasks_method': 'target_tasks_method',
+        'tasks_for': 'tasks_for',
+        'try_mode': 'try_mode',
+        'try_options': None,
+        'try_task_config': None,
+        'version': 'version',
+    }
 
     def test_Parameters_immutable(self):
         p = Parameters(**self.vals)
@@ -33,8 +69,8 @@ class TestParameters(unittest.TestCase):
 
     def test_Parameters_invalid_KeyError(self):
         """even if the value is present, if it's not a valid property, raise KeyError"""
-        p = Parameters(xyz=10, **self.vals)
+        p = Parameters(xyz=10, strict=True, **self.vals)
-        self.assertRaises(KeyError, lambda: p['xyz'])
+        self.assertRaises(Exception, lambda: p.check())
 
     def test_Parameters_get(self):
         p = Parameters(head_ref=10, level=20)
@@ -46,14 +82,14 @@ class TestParameters(unittest.TestCase):
 
     def test_Parameters_check_missing(self):
         p = Parameters()
-        self.assertRaises(ParameterMismatch, lambda: p.check())
+        self.assertRaises(Exception, lambda: p.check())
 
         p = Parameters(strict=False)
         p.check()  # should not raise
 
     def test_Parameters_check_extra(self):
         p = Parameters(xyz=10, **self.vals)
-        self.assertRaises(ParameterMismatch, lambda: p.check())
+        self.assertRaises(Exception, lambda: p.check())
 
         p = Parameters(strict=False, xyz=10, **self.vals)
         p.check()  # should not raise
@@ -91,7 +127,12 @@ class TestParameters(unittest.TestCase):
 
 
 class TestCommParameters(unittest.TestCase):
-    vals = {n: n for n in PARAMETERS.keys() + COMM_PARAMETERS.keys()}
+    vals = dict({
+        'comm_base_repository': 'comm_base_repository',
+        'comm_head_ref': 'comm_head_ref',
+        'comm_head_repository': 'comm_head_repository',
+        'comm_head_rev': 'comm_head_rev',
+    }.items() + TestParameters.vals.items())
 
     def test_Parameters_check(self):
         """
@@ -105,7 +146,7 @@ class TestCommParameters(unittest.TestCase):
         If any of the comm parameters are specified, all of them must be specified.
         """
         vals = self.vals.copy()
-        del vals[next(iter(COMM_PARAMETERS.keys()))]
+        del vals['comm_base_repository']
         p = Parameters(**vals)
         self.assertRaises(Exception, p.check)