From 3025d26ab8d70061b9b7c92814dff2f10380037b Mon Sep 17 00:00:00 2001 From: Mitchell Hentges Date: Tue, 26 Mar 2019 02:09:14 +0000 Subject: [PATCH] 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 | 206 ++++++++++-------- taskcluster/taskgraph/test/test_decision.py | 15 +- taskcluster/taskgraph/test/test_parameters.py | 61 +++++- 3 files changed, 181 insertions(+), 101 deletions(-) diff --git a/taskcluster/taskgraph/parameters.py b/taskcluster/taskgraph/parameters.py index dfbfb7e5fbce..765a7d1ebbc9 100644 --- a/taskcluster/taskgraph/parameters.py +++ b/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 -# Parameters are of the form: {name: default} or {name: lambda: default} -PARAMETERS = { - 'app_version': get_app_version(), - 'base_repository': 'https://hg.mozilla.org/mozilla-unified', - 'build_date': lambda: int(time.time()), - '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': lambda: datetime.now().strftime("%Y%m%d%H%M%S"), - 'next_version': None, - 'optimize_target_tasks': True, - 'owner': 'nobody@mozilla.com', - 'project': 'mozilla-central', - 'pushdate': lambda: int(time.time()), - 'pushlog_id': '0', - 'phabricator_diff': None, - 'release_enable_emefree': False, - 'release_enable_partners': False, - 'release_eta': '', - 'release_history': {}, - 'release_partners': None, - 'release_partner_config': None, - 'release_partner_build_number': 1, - 'release_type': 'nightly', - 'release_product': None, - '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(), +base_schema = { + Required('app_version'): basestring, + Required('base_repository'): basestring, + Required('build_date'): int, + Required('build_number'): int, + Inclusive('comm_base_repository', 'comm'): basestring, + Inclusive('comm_head_ref', 'comm'): basestring, + Inclusive('comm_head_repository', 'comm'): basestring, + Inclusive('comm_head_rev', 'comm'): basestring, + Required('do_not_optimize'): [basestring], + Required('existing_tasks'): {basestring: basestring}, + Required('filters'): [basestring], + Required('head_ref'): basestring, + Required('head_repository'): basestring, + Required('head_rev'): basestring, + Required('hg_branch'): basestring, + Required('level'): basestring, + Required('message'): basestring, + Required('moz_build_date'): basestring, + Required('next_version'): Any(None, basestring), + Required('optimize_target_tasks'): bool, + Required('owner'): basestring, + Required('phabricator_diff'): Any(None, basestring), + Required('project'): basestring, + Required('pushdate'): int, + Required('pushlog_id'): unicode, + Required('release_enable_emefree'): bool, + Required('release_enable_partners'): bool, + Required('release_eta'): Any(None, basestring), + Required('release_history'): {basestring: [dict]}, + Required('release_partners'): [basestring], + Required('release_partner_config'): Any(None, dict), + Required('release_partner_build_number'): int, + Required('release_type'): basestring, + Required('release_product'): Any(None, basestring), + Required('required_signoffs'): [basestring], + Required('signoff_urls'): dict, + Required('target_tasks_method'): Any(None, basestring), + Required('tasks_for'): Any(None, basestring), + Required('try_mode'): Any(None, basestring), + 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_head_ref': None, - 'comm_head_repository': 'https://hg.mozilla.org/comm-central', - 'comm_head_rev': None, -} + +COMM_PARAMETERS = [ + 'comm_base_repository', + 'comm_head_ref', + '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(): - 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 + kwargs = Parameters._fill_defaults(**kwargs) 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) - valid = set(PARAMETERS.keys()) - 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)) + schema = Schema(base_schema, extra=PREVENT_EXTRA if self.strict else ALLOW_EXTRA) + validate_schema(schema, self.copy(), 'Invalid parameters:') 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: diff --git a/taskcluster/taskgraph/test/test_decision.py b/taskcluster/taskgraph/test/test_decision.py index 1e4bcd715869..05aab4900e78 100644 --- a/taskcluster/taskgraph/test/test_decision.py +++ b/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)}): diff --git a/taskcluster/taskgraph/test/test_parameters.py b/taskcluster/taskgraph/test/test_parameters.py index 5d3ea3895793..9623cacb8b31 100644 --- a/taskcluster/taskgraph/test/test_parameters.py +++ b/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) - self.assertRaises(KeyError, lambda: p['xyz']) + p = Parameters(xyz=10, strict=True, **self.vals) + 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)