From 89a61d168a8b3777573d12c212e387c0fa601722 Mon Sep 17 00:00:00 2001 From: Anthony Lin Date: Wed, 9 May 2018 06:17:55 +0000 Subject: [PATCH] Refactor get_configdocs_status Method This is a follow-up patch set for [0] to address the comments to break up the get_configdocs_status method into smaller methods. Also made a minor update to the check_reformat_versions method based on comments in [0] [0] https://review.gerrithub.io/#/c/att-comdev/shipyard/+/409839/ Change-Id: Id13fcaa25e48cc2f4267a08db1ccb4561b260197 --- .../control/helpers/configdocs_helper.py | 159 ++++++++++-------- .../unit/control/test_configdocs_helper.py | 5 + .../shipyard_client/cli/input_checks.py | 2 +- .../tests/unit/cli/get/test_get_commands.py | 2 +- 4 files changed, 95 insertions(+), 73 deletions(-) diff --git a/src/bin/shipyard_airflow/shipyard_airflow/control/helpers/configdocs_helper.py b/src/bin/shipyard_airflow/shipyard_airflow/control/helpers/configdocs_helper.py index f99f8654..8a5a3099 100644 --- a/src/bin/shipyard_airflow/shipyard_airflow/control/helpers/configdocs_helper.py +++ b/src/bin/shipyard_airflow/shipyard_airflow/control/helpers/configdocs_helper.py @@ -176,78 +176,12 @@ class ConfigdocsHelper(object): """ configdocs_status = [] - # Default ordering - def_order = [SUCCESSFUL_SITE_ACTION, - LAST_SITE_ACTION, - COMMITTED, - BUFFER] + # Get ordered versions + ordered_versions = self._get_ordered_versions(versions) - # Defaults to COMMITTED and BUFFER - if versions is None: - versions = [COMMITTED, BUFFER] - - elif not len(versions) == 2: - raise AppError( - title='Incorrect number of versions for comparison', - description=( - 'User must pass in 2 valid versions for comparison'), - status=falcon.HTTP_400, - retry=False) - - elif versions[0] == versions[1]: - raise AppError( - title='Versions must be different for comparison', - description=( - 'Versions must be unique in order to perform comparison'), - status=falcon.HTTP_400, - retry=False) - - for version in versions: - if version not in def_order: - raise AppError( - title='Invalid version detected', - description=( - '{} is not a valid version, which include: ' - '{}'.format(version, ', '.join(def_order))), - status=falcon.HTTP_400, - retry=False) - - # Higher index in the def_order list will mean that it is a newer - # version. We will swap the order and sort the version if need be. - if def_order.index(versions[0]) > def_order.index(versions[1]): - sorted_versions = list(reversed(versions)) - else: - sorted_versions = versions - - # Get version id - old_version_id = self.get_revision_id(sorted_versions[0]) - new_version_id = self.get_revision_id(sorted_versions[1]) - - # Get revision name - old_version_name = sorted_versions[0] - new_version_name = sorted_versions[1] - - # Check that revision id of LAST_SITE_ACTION and SUCCESSFUL_SITE_ACTION - # is not None - for name, rev_id in [(old_version_name, old_version_id), - (new_version_name, new_version_id)]: - if (name in [LAST_SITE_ACTION, SUCCESSFUL_SITE_ACTION] and - rev_id is None): - raise AppError( - title='Version does not exist', - description='{} version does not exist'.format(name), - status=falcon.HTTP_404, - retry=False) - - # Set to 0 if there is no committed version - if old_version_name == COMMITTED and old_version_id is None: - old_version_id = 0 - new_version_id = self.get_revision_id(BUFFER) or 0 - - # Set new_version_id if None - if new_version_id is None: - new_version_id = ( - self.get_revision_id(BUFFER) or old_version_id or 0) + # Get version name and id + old_version_name, new_version_name, old_version_id, new_version_id = ( + self._get_versions_name_id(ordered_versions)) try: diff = self.deckhand.get_diff( @@ -873,3 +807,86 @@ def _format_validations_to_status(val_msgs, error_count): }, "code": code } + + def _get_ordered_versions(self, versions=None): + """returns a list of ordered versions""" + + # Default ordering + def_order = [SUCCESSFUL_SITE_ACTION, + LAST_SITE_ACTION, + COMMITTED, + BUFFER] + + # Defaults to COMMITTED and BUFFER + if versions is None: + versions = [COMMITTED, BUFFER] + + elif not len(versions) == 2: + raise AppError( + title='Incorrect number of versions for comparison', + description=( + 'User must pass in 2 valid versions for comparison'), + status=falcon.HTTP_400, + retry=False) + + elif versions[0] == versions[1]: + raise AppError( + title='Versions must be different for comparison', + description=( + 'Versions must be unique in order to perform comparison'), + status=falcon.HTTP_400, + retry=False) + + for version in versions: + if version not in def_order: + raise AppError( + title='Invalid version detected', + description=( + '{} is not a valid version, which include: ' + '{}'.format(version, ', '.join(def_order))), + status=falcon.HTTP_400, + retry=False) + + # Higher index in the def_order list will mean that it is a newer + # version. We will swap the order and sort the version if need be. + if def_order.index(versions[0]) > def_order.index(versions[1]): + ordered_versions = list(reversed(versions)) + else: + ordered_versions = versions + + return ordered_versions + + def _get_versions_name_id(self, ordered_versions): + + # Get version id + old_version_id = self.get_revision_id(ordered_versions[0]) + new_version_id = self.get_revision_id(ordered_versions[1]) + + # Get revision name + old_version_name = ordered_versions[0] + new_version_name = ordered_versions[1] + + # Check that revision id of LAST_SITE_ACTION and SUCCESSFUL_SITE_ACTION + # is not None + for name, rev_id in [(old_version_name, old_version_id), + (new_version_name, new_version_id)]: + if (name in [LAST_SITE_ACTION, SUCCESSFUL_SITE_ACTION] and + rev_id is None): + raise AppError( + title='Version does not exist', + description='{} version does not exist'.format(name), + status=falcon.HTTP_404, + retry=False) + + # Set to 0 if there is no committed version + if old_version_name == COMMITTED and old_version_id is None: + old_version_id = 0 + new_version_id = self.get_revision_id(BUFFER) or 0 + + # Set new_version_id if None + if new_version_id is None: + new_version_id = ( + self.get_revision_id(BUFFER) or old_version_id or 0) + + return ( + old_version_name, new_version_name, old_version_id, new_version_id) diff --git a/src/bin/shipyard_airflow/tests/unit/control/test_configdocs_helper.py b/src/bin/shipyard_airflow/tests/unit/control/test_configdocs_helper.py index cb36c7a2..2ebda1d1 100644 --- a/src/bin/shipyard_airflow/tests/unit/control/test_configdocs_helper.py +++ b/src/bin/shipyard_airflow/tests/unit/control/test_configdocs_helper.py @@ -60,6 +60,9 @@ REV_BUFFER_DICT = { DIFF_BUFFER_DICT = {'mop': 'unmodified', 'chum': 'created', 'slop': 'deleted'} +ORDERED_VER = ['committed', 'buffer'] +REV_NAME_ID = ('committed', 'buffer', 3, 5) + REV_BUFF_EMPTY_DICT = { 'committed': { 'id': 3, @@ -297,6 +300,8 @@ def test_is_buffer_valid_for_bucket(): def test_get_configdocs_status(): helper = ConfigdocsHelper(CTX) helper._get_revision_dict = lambda: REV_BUFFER_DICT + helper._get_ordered_versions = lambda versions: ORDERED_VER + helper._get_versions_name_id = lambda ordered_versions: REV_NAME_ID helper.deckhand.get_diff = ( lambda old_revision_id, new_revision_id: DIFF_BUFFER_DICT) result = helper.get_configdocs_status() diff --git a/src/bin/shipyard_client/shipyard_client/cli/input_checks.py b/src/bin/shipyard_client/shipyard_client/cli/input_checks.py index 45c0d2da..a3ba5111 100644 --- a/src/bin/shipyard_client/shipyard_client/cli/input_checks.py +++ b/src/bin/shipyard_client/shipyard_client/cli/input_checks.py @@ -89,7 +89,7 @@ def check_reformat_versions(ctx, buffer, committed, last_site_action, versions.append('successful_site_action') if len(versions) == 0: - return None + return ['committed', 'buffer'] elif len(versions) == 2: return versions diff --git a/src/bin/shipyard_client/tests/unit/cli/get/test_get_commands.py b/src/bin/shipyard_client/tests/unit/cli/get/test_get_commands.py index 05609290..58658003 100644 --- a/src/bin/shipyard_client/tests/unit/cli/get/test_get_commands.py +++ b/src/bin/shipyard_client/tests/unit/cli/get/test_get_commands.py @@ -66,7 +66,7 @@ def test_get_configdocs_without_passing_collection(*args): runner = CliRunner() with patch.object(GetConfigdocsStatus, '__init__') as mock_method: runner.invoke(shipyard, [auth_vars, 'get', 'configdocs']) - mock_method.assert_called_once_with(ANY, None) + mock_method.assert_called_once_with(ANY, ['committed', 'buffer']) def test_get_configdocs_negative(*args):