From 6e9a9bed5f463c38b96b707a40668f6b326b7e48 Mon Sep 17 00:00:00 2001 From: Amol Kahat Date: Mon, 31 May 2021 13:42:03 +0530 Subject: [PATCH] Set extended_hash vale to None instead of 'None' Sometimes extended_hash value is getting set to 'None' instead of None. This leads to commit_hash + distro_hash + extended_hash combination not found error. Added test cases for extended hash. Added hash validation functions, which checks that hash is valid or not. This change forced to add valid hashes to the test cases. Change-Id: I1346ce23daef1f95119b9cc58c26d9165e5298d0 Signed-off-by: Amol Kahat --- ci-scripts/dlrnapi_promoter/dlrn_hash.py | 62 ++++++++++++++---- .../dlrnapi_promoter/test_dlrn_client_unit.py | 43 ++++++------- .../dlrnapi_promoter/test_dlrn_hash_unit.py | 64 ++++++++++++++++--- .../dlrnapi_promoter/test_qcow_client_unit.py | 17 +++-- .../dlrnapi_promoter/test_repo_client_unit.py | 28 ++++---- .../dlrnapi_promoter/test_unit_fixtures.py | 43 +++++++++---- 6 files changed, 185 insertions(+), 72 deletions(-) diff --git a/ci-scripts/dlrnapi_promoter/dlrn_hash.py b/ci-scripts/dlrnapi_promoter/dlrn_hash.py index 1bbea799f..9cdc12289 100644 --- a/ci-scripts/dlrnapi_promoter/dlrn_hash.py +++ b/ci-scripts/dlrnapi_promoter/dlrn_hash.py @@ -11,6 +11,35 @@ import logging +def check_hash(hash_value): + """ + This function will check the hash function properties like + hash size, valid hash etc. + """ + if hash_value is not None: + try: + int(hash_value, 16) + len(hash_value) == 40 + except (TypeError, ValueError): + raise DlrnHashError("Invalid hash format!!") + + +def check_extended_hash(extended_hash): + """ + This function will check valid extended hash. + """ + if extended_hash is not None and extended_hash not in ['None', '']: + try: + e_hash = extended_hash.split("_") + if len(e_hash) == 2: + check_hash(e_hash[0]) + check_hash(e_hash[1]) + else: + raise DlrnHashError("Invalid extended hash format") + except (TypeError, ValueError): + raise DlrnHashError("Invalid extended hash, not a hex hash!") + + class DlrnHashError(Exception): """ Raised on various errors on DlrnHash operations @@ -44,6 +73,10 @@ def __init__(self, commit_hash=None, distro_hash=None, extended_hash=None, :param source: a dictionary with all the parameters as keys :param component: the eventual component of the hash """ + # Make sure extended_hash value should be None not "None" + if extended_hash in ['None', '']: + extended_hash = None + # Load from default values into unified source _source = { 'commit_hash': commit_hash, @@ -107,9 +140,11 @@ def sanity_check(self): Checks if the basic components of the hash are present component and timestamp are optional """ - if self.commit_hash is None or self.distro_hash is None or \ - self.extended_hash == '': - raise DlrnHashError("Invalid commit or distro or extended hash") + if self.commit_hash is None or self.distro_hash is None: + raise DlrnHashError("Invalid commit or distro hash") + check_hash(self.commit_hash) + check_hash(self.distro_hash) + check_extended_hash(self.extended_hash) def __repr__(self): """ @@ -119,8 +154,8 @@ def __repr__(self): """ return ("" - "" % (self.commit_hash, self.distro_hash, self.extended_hash, - self.component, self.timestamp)) + "" % (self.commit_hash, self.distro_hash, self.component, + self.extended_hash, self.timestamp)) def __str__(self): """ @@ -129,9 +164,9 @@ def __str__(self): :return: The string representation of the hash informations """ return ("commit: %s, distro: %s, extended: %s, component: %s," - "timestamp: %s" - "" % (self.commit_hash, self.distro_hash, self.extended_hash, - self.component, self.timestamp)) + "timestamp: %s" % (self.commit_hash, self.distro_hash, + self.extended_hash, self.component, + self.timestamp)) def __ne__(self, other): """ @@ -179,7 +214,7 @@ def full_hash(self): Work only with single norma dlrn haseh :return: The full hash format or None """ - if self.extended_hash not in [None, 'None']: + if self.extended_hash is not None: ext_distro_hash, ext_commit_hash = self.extended_hash.split('_') return '{0}_{1}_{2}_{3}'.format(self.commit_hash, self.distro_hash[:8], @@ -245,9 +280,12 @@ def sanity_check(self): component and timestamp are optional """ if self.commit_hash is None or self.distro_hash is None or \ - self.extended_hash == '' or self.aggregate_hash is None: - raise DlrnHashError("Invalid commit or distro or " - "extended aggregate hash") + self.aggregate_hash is None: + raise DlrnHashError("Invalid commit or distro or aggregate hash") + check_hash(self.commit_hash) + check_hash(self.distro_hash) + check_hash(self.aggregate_hash) + check_extended_hash(self.extended_hash) def __repr__(self): """ diff --git a/ci-scripts/dlrnapi_promoter/test_dlrn_client_unit.py b/ci-scripts/dlrnapi_promoter/test_dlrn_client_unit.py index 8fc5a2c29..f41000e9f 100644 --- a/ci-scripts/dlrnapi_promoter/test_dlrn_client_unit.py +++ b/ci-scripts/dlrnapi_promoter/test_dlrn_client_unit.py @@ -58,19 +58,22 @@ def setUp(self): # Set up some ready to use hashes self.dlrn_hash_commitdistro1 = DlrnCommitDistroExtendedHash( - commit_hash='a', - distro_hash='b', - component="comp1", - timestamp=1) + commit_hash='90633a3785687ddf3d37c0f86f9ad9f93926d639', + distro_hash='d68290fed3d9aa069c95fc16d0d481084adbadc6', + extended_hash='6137f83ab8defe688e70a18ef1c7e5bf3fbf02ef_' + '3945701fc2ae9b1b14e4261e87e203b2a89ccdca', + component="tripleo", + timestamp=1) self.dlrn_hash_commitdistro2 = DlrnCommitDistroExtendedHash( - commit_hash='c', - distro_hash='d', - component="comp2", - timestamp=2) - self.dlrn_hash_aggregate = DlrnAggregateHash(commit_hash='abc', - distro_hash='def', - aggregate_hash='ghjk', - timestamp=1) + commit_hash='4f4774d4e410ce72b024c185d3054cf649e5c578', + distro_hash='fe88530aa04df13ebc63287c819c721740837aae', + component="tempest", + timestamp=2) + self.dlrn_hash_aggregate = DlrnAggregateHash( + commit_hash='98da7b0933a2975598844bf40edec4b61714db40', + distro_hash='c3a41aaf53b9ea10333387b7d40797ba2c1018d2', + aggregate_hash='26b9d4d1d8fd09cdc2b11c7dd0f71f93', + timestamp=1) self.promote_log_header = ("Dlrn promote '{}' from {} to {}:" "".format(self.dlrn_hash_commitdistro1, 'tripleo-ci-testing', @@ -433,13 +436,13 @@ def setUp(self): super(TestNamedHashes, self).setUp() dlrn_start_hash_dict = { 'timestamp': '1528085427', - 'commit_hash': 'd221f4b33cf2763875fc6394902f7923108a34da', - 'distro_hash': '70bdcd40eb5cc62e4762a7db0086e09f6edf2e5c' + 'commit_hash': '326452e5851e8347b15b53c3d6b70e6f5225f3ea', + 'distro_hash': '589b556babb2d0c5c6e79d5c2a505341b70ef370' } dlrn_changed_hash_dict = { 'timestamp': '1528085529', - 'commit_hash': 'e3d9fffbf82ec71deff60ba914f1db0e1625466a', - 'distro_hash': 'Iba78e857267ac771d23919fbd1e3c9fcc5813c9' + 'commit_hash': '6b3bf3bba01055ca8e544ce258b44e4f5da3da34', + 'distro_hash': '6aaa73f4925b38ae77d468257bced8d3baf8dd97' } self.dlrn_changed_hash = DlrnHash(source=dlrn_changed_hash_dict) self.dlrn_start_hash = DlrnHash(source=dlrn_start_hash_dict) @@ -447,7 +450,6 @@ def setUp(self): @patch('logging.Logger.error') @patch('dlrn_client.DlrnClient.fetch_hashes') def test_named_hashes_unchanged(self, mock_fetch_hashes, mock_log_err): - mock_fetch_hashes.side_effect = [self.dlrn_start_hash, self.dlrn_start_hash] # positive test for hashes_unchanged @@ -506,7 +508,7 @@ def test_fetch_current_named_hash_no_store(self, mock_fetch_hashes, @patch('logging.Logger.debug') @patch('dlrn_client.DlrnClient.fetch_hashes') def test_update_current_named_hash( - self, mock_fetch_hashes, mock_log_debug): + self, mock_fetch_hashes, mock_log_debug): mock_fetch_hashes.side_effect = [ self.dlrn_changed_hash, self.dlrn_changed_hash ] @@ -523,7 +525,7 @@ def test_update_current_named_hash( @patch('logging.Logger.debug') @patch('dlrn_client.DlrnClient.fetch_promotions') def test_fetch_current_named_hashes_no_hashes( - self, fetch_promotions_mock, mock_log_debug, mock_log_warn): + self, fetch_promotions_mock, mock_log_debug, mock_log_warn): fetch_promotions_mock.return_value = [] self.client.fetch_current_named_hashes() self.assertFalse(mock_log_debug.called) @@ -543,7 +545,6 @@ def test_get_promotion_aggregate_hashes_success(self, mock_log_debug, mock_log_info, mock_log_error): - self.maxDiff = None delorean_repo_path, tmp_dir = self.get_tmp_delorean_repo() # Extremely important that we ensure this method does not produce @@ -594,7 +595,6 @@ def test_get_promotion_aggregate_hashes_no_components(self, mock_log_debug, mock_log_info, mock_log_error): - delorean_repo_path, tmp_dir = self.get_tmp_delorean_repo(empty=True) with self.assertRaises(PromotionError): self.client.get_promotion_aggregate_hashes("", @@ -614,7 +614,6 @@ def test_get_promotion_aggregate_hashes_no_components(self, def test_get_promotion_aggregate_url_error(self, get_hash_mock, mock_log_error): - self.config.repo_url = "file:///not.exist" with self.assertRaises(PromotionError): self.client.get_promotion_aggregate_hashes("", diff --git a/ci-scripts/dlrnapi_promoter/test_dlrn_hash_unit.py b/ci-scripts/dlrnapi_promoter/test_dlrn_hash_unit.py index dd9924fc5..ceda6c1ff 100644 --- a/ci-scripts/dlrnapi_promoter/test_dlrn_hash_unit.py +++ b/ci-scripts/dlrnapi_promoter/test_dlrn_hash_unit.py @@ -12,6 +12,35 @@ from test_unit_fixtures import hashes_test_cases +def get_commit_dir(hash_dict, hash_type="commitdistro"): + if isinstance(hash_dict, dict): + commit_hash = hash_dict.get("commit_hash") + distro_hash = hash_dict.get("distro_hash") + else: + commit_hash = hash_dict.commit_hash + distro_hash = hash_dict.distro_hash + if hash_type == 'commitdistro': + try: + extended_hash = hash_dict.extended_hash + except (AttributeError): + extended_hash = hash_dict.get('extended_hash') + e_hash = '_' + if extended_hash: + e_d_hash, e_c_hash = extended_hash.split("_") + e_hash += "{}_{}".format(e_d_hash[:8], e_c_hash[:8]) + + return "{}/{}/{}_{}{}".format(commit_hash[:2], commit_hash[2:4], + commit_hash, distro_hash[:8], e_hash) + elif hash_type == "aggregate": + try: + aggregate_hash = hash_dict.aggregate_hash + except (AttributeError): + aggregate_hash = hash_dict.get("aggregate_hash") + + return "{}/{}/{}".format(aggregate_hash[:2], aggregate_hash[2:4], + aggregate_hash) + + class TestDlrnHashSubClasses(unittest.TestCase): def test_build_valid(self): @@ -19,13 +48,16 @@ def test_build_valid(self): values = source_types['dict']['valid'] if hash_type == "commitdistro": dh = DlrnCommitDistroExtendedHash( - commit_hash=values['commit_hash'], - distro_hash=values['distro_hash'], - timestamp=values['timestamp']) + commit_hash=values['commit_hash'], + distro_hash=values['distro_hash'], + extended_hash=values['extended_hash'], + timestamp=values['timestamp']) self.assertEqual(dh.commit_hash, source_types['dict']['valid']['commit_hash']) self.assertEqual(dh.distro_hash, source_types['dict']['valid']['distro_hash']) + self.assertEqual(dh.extended_hash, + source_types['dict']['valid']['extended_hash']) elif hash_type == "aggregate": aggregate_hash = source_types['dict']['valid'][ 'aggregate_hash'] @@ -46,6 +78,8 @@ def test_build_valid_from_source(self): source_types['dict']['valid']['commit_hash']) self.assertEqual(dh.distro_hash, source_types['dict']['valid']['distro_hash']) + self.assertEqual(dh.extended_hash, + source_types['dict']['valid']['extended_hash']) elif hash_type == "aggregate": aggregate_hash = source_types['dict']['valid'][ 'aggregate_hash'] @@ -117,8 +151,10 @@ def test_properties(self): source = source_types['object']['valid'] dh = DlrnHash(source=source) if hash_type == "commitdistro": - full_hash = "{}_{}".format(source.commit_hash, - source.distro_hash[:8]) + e_d_hash, e_c_hash = source.extended_hash.split("_") + full_hash = "{}_{}_{}_{}".format(source.commit_hash, + source.distro_hash[:8], + e_d_hash[:8], e_c_hash[:8]) self.assertEqual(dh.full_hash, full_hash) elif hash_type == "aggregate": self.assertEqual(dh.full_hash, source.aggregate_hash) @@ -139,23 +175,31 @@ def test_commit_dir(self): for hash_type, source_types in hashes_test_cases.items(): dh = DlrnHash(source=source_types['object']['valid']) if hash_type == "commitdistro": - self.assertEqual(dh.commit_dir, "ab/cd/abcd_defg") + commit_dir = get_commit_dir(source_types['object'][ + 'valid']) + self.assertEqual(dh.commit_dir, commit_dir) elif hash_type == "aggregate": dh.label = "label" - self.assertEqual(dh.commit_dir, "label/ab/cd/abcd") + commit_dir = get_commit_dir(source_types['object']['valid'], + "aggregate") + self.assertEqual(dh.commit_dir, "label/{}".format(commit_dir)) def test_commit_dir_no_label(self): for hash_type, source_types in hashes_test_cases.items(): dh = DlrnHash(source=source_types['object']['valid']) if hash_type == "commitdistro": - self.assertEqual(dh.commit_dir, "ab/cd/abcd_defg") + commit_dir = get_commit_dir(source_types['object']['valid']) + self.assertEqual(dh.commit_dir, commit_dir) elif hash_type == "aggregate": - self.assertEqual(dh.commit_dir, "ab/cd/abcd") + commit_dir = get_commit_dir(source_types['object']['valid'], + 'aggregate') + self.assertEqual(dh.commit_dir, commit_dir) def test_commit_dir_component(self): for hash_type, source_types in hashes_test_cases.items(): dh = DlrnHash(source=source_types['object']['valid']) if hash_type == "commitdistro": dh.component = "component1" + commit_dir = get_commit_dir(source_types['object']['valid']) self.assertEqual(dh.commit_dir, - "component/component1/ab/cd/abcd_defg") + "component/component1/{}".format(commit_dir)) diff --git a/ci-scripts/dlrnapi_promoter/test_qcow_client_unit.py b/ci-scripts/dlrnapi_promoter/test_qcow_client_unit.py index c2b4d151c..da61c92c6 100644 --- a/ci-scripts/dlrnapi_promoter/test_qcow_client_unit.py +++ b/ci-scripts/dlrnapi_promoter/test_qcow_client_unit.py @@ -103,12 +103,17 @@ def setUp(self): self.images_root = self.client.root self.images_dir = self.client.images_dir - self.previous_hash_dir = os.path.join(self.images_dir, "efgh") + self.previous_hash_dir = os.path.join( + self.images_dir, + DlrnHash(source=hashes_test_cases['aggregate']['object'][ + 'valid_notimestamp']).full_hash) self.current_hash_dir = os.path.join(self.images_dir, "dunno") - self.candidate_hash_dir = os.path.join(self.images_dir, "abcd") + self.candidate_hash_dir = os.path.join( + self.images_dir, + DlrnHash(source=hashes_test_cases['aggregate']['object'][ + 'valid']).full_hash) self.target_label = "test-label" self.previous_target_label = "previous-{}".format(self.target_label) - try: os.makedirs(self.candidate_hash_dir) except FileExistsError: @@ -138,7 +143,6 @@ def test_promote_success_no_previous(self, mock_log_error): self.client.promote(self.valid_candidate_hash, self.target_label, create_previous=False, validation=False) - promotion_link = os.path.join(self.images_dir, self.target_label) check_links(os, promotion_link, "test", self.candidate_hash_dir) @@ -159,7 +163,10 @@ def test_promote_success_previous(self, promotion_link = os.path.join(self.images_dir, self.target_label) previous_link = os.path.join(self.images_dir, "previous-test-label") - previous_dir = os.path.join(self.images_dir, "efgh") + previous_dir = os.path.join( + self.images_dir, + DlrnHash(source=hashes_test_cases['aggregate']['object'][ + 'valid_notimestamp']).full_hash) check_links(os, promotion_link, "test", self.candidate_hash_dir, previous_link=previous_link, previous_dir=previous_dir) self.assertFalse(mock_log_error.called) diff --git a/ci-scripts/dlrnapi_promoter/test_repo_client_unit.py b/ci-scripts/dlrnapi_promoter/test_repo_client_unit.py index 8e2c2c25a..cdb404b76 100644 --- a/ci-scripts/dlrnapi_promoter/test_repo_client_unit.py +++ b/ci-scripts/dlrnapi_promoter/test_repo_client_unit.py @@ -30,20 +30,24 @@ def setUp(self): os.makedirs(log_d) self.dlrn_hash_commitdistro = DlrnCommitDistroExtendedHash( - commit_hash='abc', - distro_hash='def', - component="comp1", - timestamp=1) + commit_hash='26c19986a58560b90bfa4b39bd3bc1ef43ad57f5', + distro_hash='34d2c7ae120f6c07767c5638f28d4a35515ade67', + extended_hash='e99c5941564f63e6c2f99592c0c6ff5ac815914a_' + '6772f32196d090a6cd303471538fa963727a58e7', + component="security", + timestamp=1) self.dlrn_hash_commitdistro2 = DlrnCommitDistroExtendedHash( - commit_hash='ghj', - distro_hash='klm', - component="comp2", - timestamp=2) + commit_hash='adce3dce8a017f30c2214df4802dd9b6bd79cce6', + distro_hash='030bf177f0394ece3dc912dd22ae7ccee8e4af0b', + extended_hash='3dbc2b82127cdfbbdc5967e57d2acfca2c442978_' + '12a790ca4bbf61f2f4c04a9d2fcd1c12b7615925', + component="octavia", + timestamp=2) self.dlrn_hash_aggregate = DlrnAggregateHash( - commit_hash='abc', - distro_hash='def', - aggregate_hash='ghjk', - timestamp=1) + commit_hash='28fe6735517a32c6b4d6e73b95ad752ed172d9d3', + distro_hash='4acaff5b0437c615562fa2860fdd48abd11117d6', + aggregate_hash='8188496b0d154cf2f22f408851ed418b', + timestamp=1) self.hashes = [self.dlrn_hash_commitdistro, self.dlrn_hash_aggregate] self.temp_dir = tempfile.mkdtemp() diff --git a/ci-scripts/dlrnapi_promoter/test_unit_fixtures.py b/ci-scripts/dlrnapi_promoter/test_unit_fixtures.py index f21a659eb..af1a26f77 100644 --- a/ci-scripts/dlrnapi_promoter/test_unit_fixtures.py +++ b/ci-scripts/dlrnapi_promoter/test_unit_fixtures.py @@ -12,7 +12,6 @@ # Python2 imports from mock import Mock - # Passwordles ssh key SSH_CONTENT = """-----BEGIN RSA PRIVATE KEY----- MIIG5QIBAAKCAYEAoPmVjDOjLAbOfE+8wfWMiaL5II0QQa8sCzgMe5fE8f/jphv6 @@ -56,16 +55,38 @@ # These are preparation for all the types of dlrn_hashes we are going to test # on the following test cases. -valid_commitdistro_kwargs = dict(commit_hash='abcd', distro_hash='defg', - timestamp=1) -valid_commitdistro_notimestamp_kwargs = dict(commit_hash='a', distro_hash='b') -invalid_commitdistro_kwargs = dict(commit='a', distro='b') -different_commitdistro_kwargs = dict(commit_hash='b', distro_hash='c', - timestamp=1) -different_commitdistro_notimestamp_kwargs = dict(commit_hash='a', - distro_hash='b') -valid_aggregate_kwargs = dict(aggregate_hash='abcd', commit_hash='defg', - distro_hash='hijk', timestamp=1) +valid_commitdistro_kwargs = dict( + commit_hash='f6ef2e4e4e4cce030e5ef2f7799bff4e968af03a', + distro_hash='41b4c09cabc2df6450eeb997594077fc7d4ebe6e', + extended_hash='85ddd74e99da98ad07e637d0b42d4692910e4e70_' + '7ecd11cd44cbe7b93e4ff27d0049d937945d3a75', + timestamp=1) +valid_commitdistro_notimestamp_kwargs = dict( + commit_hash='ec60849c773e4649d22d4b976e7c5892596d4d1b', + distro_hash='030bf177f0394ece3dc912dd22ae7ccee8e4af0b', + extended_hash='35e7cd373aa33c4fe9dfc21ff41da81810c69715_' + '86d134f3ea6848af3f1968b8904ba0da1fa7a074') +invalid_commitdistro_kwargs = dict( + commit='26c19986a58560b90bfa4b39bd3bc1ef43ad57f5', + distro='34d2c7ae120f6c07767c5638f28d4a35515ade67', + extended='e99c5941564f63e6c2f99592c0c6ff5ac815914a_' + '6772f32196d090a6cd303471538fa963727a58e7') +different_commitdistro_kwargs = dict( + commit_hash='4f4774d4e410ce72b024c185d3054cf649e5c578', + distro_hash='fe88530aa04df13ebc63287c819c721740837aae', + extended_hash='255baafa43d70533e93abb74a67f97b974ee314c_' + 'eaf928319df5cdeb87e646e940eacc0402a49751', + timestamp=1) +different_commitdistro_notimestamp_kwargs = dict( + commit_hash='96e24bb1dbbac0313cea1f233f8cd918c6d820ac', + distro_hash='c3a41aaf53b9ea10333387b7d40797ba2c1018d2', + extended_hash='29c28719aaf8b6b58a75e9aa9d9307b6248e9717_' + '14078f4c7615d8b21a2a6d7c75afb84b4920f59b') +valid_aggregate_kwargs = dict( + aggregate_hash='d7c3cd922f91d83aacb5d7555de9d5bc', + commit_hash='f21ff147b907701bfbc840a1ee59a8014e77684a', + distro_hash='ea06708141614decfdc4af687a26c9586a23b9ff', + timestamp=1) valid_aggregate_notimestamp_kwargs = dict(aggregate_hash='a', commit_hash='b', distro_hash='c') invalid_aggregate_kwargs = dict(aggregate='a')