From 36000e4524338cc27b00bd6456001b0b65eb5778 Mon Sep 17 00:00:00 2001 From: Johan Lorenzo Date: Mon, 29 Jan 2018 14:39:15 +0100 Subject: [PATCH] Allow a single instance to talk to multiple ship-it roots --- examples/config.example.json | 4 +- shipitscript/exceptions.py | 5 ++ shipitscript/script.py | 5 +- shipitscript/task.py | 51 +++++++++++-------- .../integration/test_integration_script.py | 4 +- shipitscript/test/test_task.py | 26 +++++++--- shipitscript/test/test_utils.py | 36 ++++++++++++- shipitscript/utils.py | 14 +++++ 8 files changed, 108 insertions(+), 37 deletions(-) diff --git a/examples/config.example.json b/examples/config.example.json index dc0eed6..7aafff8 100644 --- a/examples/config.example.json +++ b/examples/config.example.json @@ -2,11 +2,11 @@ "work_dir": "/path/to/scriptworker/tmp/work", "schema_file": "/path/to/shipitscript/shipitscript/data/pushapk_task_schema.json", - "ship_it_instance": { + "ship_it_instances": [{ "username": "some@user.name", "password": "50mep@ssword", "api_root": "http://ship-it.tld/" - }, + }], "verbose": true } diff --git a/shipitscript/exceptions.py b/shipitscript/exceptions.py index de7f7bb..37a8d11 100644 --- a/shipitscript/exceptions.py +++ b/shipitscript/exceptions.py @@ -5,3 +5,8 @@ class TaskVerificationError(ScriptWorkerTaskException): def __init__(self, msg): super().__init__(msg, exit_code=STATUSES['malformed-payload']) + + +class BadInstanceConfigurationError(ScriptWorkerTaskException): + def __init__(self, msg): + super().__init__(msg, exit_code=STATUSES['internal-error']) diff --git a/shipitscript/script.py b/shipitscript/script.py index de1c101..74aa9f5 100644 --- a/shipitscript/script.py +++ b/shipitscript/script.py @@ -13,7 +13,7 @@ from scriptworker.exceptions import ScriptWorkerTaskException from shipitscript.ship_actions import mark_as_shipped -from shipitscript.task import validate_task_schema, validate_task_scope +from shipitscript.task import validate_task_schema, get_ship_it_instance_config_from_scope from shipitscript.utils import load_json @@ -24,9 +24,8 @@ async def async_main(context): context.task = scriptworker.client.get_task(context.config) log.info('Validating task definition...') validate_task_schema(context) - validate_task_scope(context) + ship_it_instance_config = get_ship_it_instance_config_from_scope(context) - ship_it_instance_config = context.config['ship_it_instance'] release_name = context.task['payload']['release_name'] mark_as_shipped(ship_it_instance_config, release_name) log.info('Done!') diff --git a/shipitscript/task.py b/shipitscript/task.py index 5d40baf..c58af49 100644 --- a/shipitscript/task.py +++ b/shipitscript/task.py @@ -3,7 +3,8 @@ import scriptworker.client -from shipitscript.exceptions import TaskVerificationError +from shipitscript.exceptions import TaskVerificationError, BadInstanceConfigurationError +from shipitscript.utils import get_single_item_from_list log = logging.getLogger(__name__) @@ -28,29 +29,37 @@ def validate_task_schema(context): scriptworker.client.validate_json_schema(context.task, task_schema) -def validate_task_scope(context): +def get_ship_it_instance_config_from_scope(context): scope = _get_scope(context.task) allowed_api_root = ALLOWED_API_ROOT_PER_VALID_SCOPE[scope] - api_root_of_this_instance = context.config['ship_it_instance']['api_root'] - api_root_of_this_instance = api_root_of_this_instance.rstrip('/') - if allowed_api_root != api_root_of_this_instance: - if allowed_api_root in PROTECTED_API_ROOTS or api_root_of_this_instance in PROTECTED_API_ROOTS: - instance_type = scope.split(':')[-1] - raise TaskVerificationError( - 'This instance is now allowed to talk to the "{}" instance ({})'.format(instance_type, allowed_api_root) - ) + configured_instances = context.config['ship_it_instances'] + instance_type = scope.split(':')[-1] + if allowed_api_root in PROTECTED_API_ROOTS: + def condition(instance): + return instance['api_root'].rstrip('/') == allowed_api_root + no_item_error_message = 'This instance is now allowed to talk to the "{}" instance ({})'.format( + instance_type, allowed_api_root + ) + else: + def condition(instance): + return instance['api_root'].rstrip('/') not in PROTECTED_API_ROOTS + no_item_error_message = "Couldn't find an API root that is not a protected one", -def _get_scope(task): - given_scopes = task['scopes'] - scopes = [scope for scope in given_scopes if scope in VALID_SCOPES] + return get_single_item_from_list( + configured_instances, + condition=condition, + ErrorClass=BadInstanceConfigurationError, + no_item_error_message=no_item_error_message, + too_many_item_error_message='Too many "{}" instances configured'.format(instance_type, allowed_api_root), + ) - number_of_scopes = len(scopes) - if number_of_scopes == 0: - raise TaskVerificationError( - 'No valid scope found. Task must have one of these: {}. Given scopes: {}'.format(VALID_SCOPES, given_scopes) - ) - elif number_of_scopes > 1: - raise TaskVerificationError('More than one valid scope given: {}'.format(scopes)) - return scopes[0] +def _get_scope(task): + return get_single_item_from_list( + task['scopes'], + condition=lambda scope: scope in VALID_SCOPES, + ErrorClass=TaskVerificationError, + no_item_error_message='No valid scope found. Task must have one of these: {}'.format(VALID_SCOPES), + too_many_item_error_message='More than one valid scope given', + ) diff --git a/shipitscript/test/integration/test_integration_script.py b/shipitscript/test/integration/test_integration_script.py index 16f409a..fe2cec4 100644 --- a/shipitscript/test/integration/test_integration_script.py +++ b/shipitscript/test/integration/test_integration_script.py @@ -35,12 +35,12 @@ "schema_file": "{project_data_dir}/shipit_task_schema.json", "verbose": true, - "ship_it_instance": {{ + "ship_it_instances": [{{ "api_root": "http://some.ship-it.tld/api/root", "timeout_in_seconds": 1, "username": "some-username", "password": "some-password" - }} + }}] }}''' diff --git a/shipitscript/test/test_task.py b/shipitscript/test/test_task.py index 6017014..97051c4 100644 --- a/shipitscript/test/test_task.py +++ b/shipitscript/test/test_task.py @@ -3,16 +3,21 @@ from scriptworker.context import Context from scriptworker.exceptions import ScriptWorkerTaskException -from shipitscript.exceptions import TaskVerificationError +from shipitscript.exceptions import TaskVerificationError, BadInstanceConfigurationError from shipitscript.script import get_default_config -from shipitscript.task import validate_task_schema, validate_task_scope, _get_scope +from shipitscript.task import validate_task_schema, get_ship_it_instance_config_from_scope, _get_scope @pytest.fixture def context(): context = Context() context.config = get_default_config() - context.config['ship_it_instance'] = {} + context.config['ship_it_instances'] = [{ + 'api_root': '', + 'timeout_in_seconds': 1, + 'username': 'some-username', + 'password': 'some-password' + }] context.task = { 'dependencies': ['someTaskId'], 'payload': { @@ -47,15 +52,20 @@ def test_validate_task(context): ('https://ship-it-dev.allizom.org', 'project:releng:ship-it:dev', True), ('https://ship-it.mozilla.org', 'project:releng:ship-it:dev', True), )) -def test_validate_scope(context, api_root, scope, raises): - context.config['ship_it_instance']['api_root'] = api_root +def test_get_ship_it_instance_config_from_scope(context, api_root, scope, raises): + context.config['ship_it_instances'][0]['api_root'] = api_root context.task['scopes'] = [scope] if raises: - with pytest.raises(TaskVerificationError): - validate_task_scope(context) + with pytest.raises(BadInstanceConfigurationError): + get_ship_it_instance_config_from_scope(context) else: - validate_task_scope(context) + assert get_ship_it_instance_config_from_scope(context) == { + 'api_root': api_root, + 'timeout_in_seconds': 1, + 'username': 'some-username', + 'password': 'some-password' + } @pytest.mark.parametrize('scopes, raises', ( diff --git a/shipitscript/test/test_utils.py b/shipitscript/test/test_utils.py index 02118e5..a172de3 100644 --- a/shipitscript/test/test_utils.py +++ b/shipitscript/test/test_utils.py @@ -1,8 +1,9 @@ import os +import pytest import tempfile import json -from shipitscript.utils import load_json +from shipitscript.utils import load_json, get_single_item_from_list def test_load_json_from_file(): @@ -14,3 +15,36 @@ def test_load_json_from_file(): json.dump(json_object, f) assert load_json(output_file) == json_object + + +@pytest.mark.parametrize('list_, condition, expected', ( + (['a', 'b', 'c'], lambda item: item == 'b', 'b'), + ([{'some_key': 1}, {'some_key': 2}, {'some_key': 3}], lambda item: item['some_key'] == 1, {'some_key': 1}), +)) +def test_get_single_item_from_list(list_, condition, expected): + assert get_single_item_from_list(list_, condition) == expected + + +class SomeCustomError(Exception): + pass + + +@pytest.mark.parametrize( + 'list_, condition, ErrorClass, no_item_error_message, too_many_item_error_message, has_all_params, expected_message', + ( + (['a', 'b', 'c'], lambda item: item == 'z', SomeCustomError, 'NO ITEM', 'TOO MANY', True, "NO ITEM. Given: ['a', 'b', 'c']"), + (['a', 'b', 'b'], lambda item: item == 'b', SomeCustomError, 'NO ITEM', 'TOO MANY', True, "TOO MANY. Given: ['a', 'b', 'b']"), + (['a', 'b', 'c'], lambda _: False, ValueError, None, None, False, "No item matched condition. Given: ['a', 'b', 'c']"), + (['a', 'b', 'c'], lambda _: True, ValueError, None, None, False, "Too many items matched condition. Given: ['a', 'b', 'c']"), + ) +) +def test_fail_(list_, condition, ErrorClass, no_item_error_message, too_many_item_error_message, has_all_params, expected_message): + with pytest.raises(ErrorClass) as exec_info: + if has_all_params: + get_single_item_from_list( + list_, condition, ErrorClass, no_item_error_message, too_many_item_error_message + ) + else: + get_single_item_from_list(list_, condition) + + assert str(exec_info.value) == expected_message diff --git a/shipitscript/utils.py b/shipitscript/utils.py index 7b375a8..703e0ef 100644 --- a/shipitscript/utils.py +++ b/shipitscript/utils.py @@ -4,3 +4,17 @@ def load_json(path): with open(path, "r") as fh: return json.load(fh) + + +def get_single_item_from_list( + list_, condition, ErrorClass=ValueError, no_item_error_message='No item matched condition', + too_many_item_error_message='Too many items matched condition' +): + filtered_list = [item for item in list_ if condition(item)] + number_of_items_in_filtered_list = len(filtered_list) + if number_of_items_in_filtered_list == 0: + raise ErrorClass('{}. Given: {}'.format(no_item_error_message, list_)) + elif number_of_items_in_filtered_list > 1: + raise ErrorClass('{}. Given: {}'.format(too_many_item_error_message, list_)) + + return filtered_list[0]