From 86b582751f818c3cddabb65ccc22c7330c94220e Mon Sep 17 00:00:00 2001 From: Qixiang Wan Date: Feb 26 2018 09:50:58 +0000 Subject: Refator PDC.get_modules Refactor the code to be more readable. --- diff --git a/freshmaker/handlers/git/rpm_spec_change.py b/freshmaker/handlers/git/rpm_spec_change.py index 1ce5c5c..cba7d8c 100644 --- a/freshmaker/handlers/git/rpm_spec_change.py +++ b/freshmaker/handlers/git/rpm_spec_change.py @@ -47,9 +47,10 @@ class GitRPMSpecChangeHandler(BaseHandler): rpm, branch, rev) pdc = PDC(conf) - modules = pdc.get_latest_modules(component_name=rpm, - component_branch=branch, - active='true') + modules = pdc.get_modules(latest_only=True, + component_name=rpm, + component_branch=branch, + active=True) for module in modules: name = module['name'] diff --git a/freshmaker/handlers/mbs/module_state_change.py b/freshmaker/handlers/mbs/module_state_change.py index 243bb97..984dca7 100644 --- a/freshmaker/handlers/mbs/module_state_change.py +++ b/freshmaker/handlers/mbs/module_state_change.py @@ -84,9 +84,10 @@ class MBSModuleStateChangeHandler(BaseHandler): return [] pdc = PDC(conf) - modules = pdc.get_latest_modules(build_dep_name=module_name, - build_dep_stream=module_stream, - active='true') + modules = pdc.get_modules(latest_only=True, + build_dep_name=module_name, + build_dep_stream=module_stream, + active=True) for mod in modules: name = mod['name'] diff --git a/freshmaker/pdc.py b/freshmaker/pdc.py index 306e19b..e307592 100644 --- a/freshmaker/pdc.py +++ b/freshmaker/pdc.py @@ -52,32 +52,62 @@ class PDC(object): insecure=self.config.pdc_insecure, ) - def get_latest_modules(self, **kwargs): + def get_latest_module(self, name, stream, active=True): """ - Query PDC with query parameters in kwargs and return a list of modules - which contains latest modules of each (module_name, module_version). + Query PDC to get the latest module which has the newest version. - :param kwargs: query parameters in keyword arguments, should only provide - valid query parameters supported by PDC's module query API. - :return: a list of modules + :param name: module name + :param stream: module stream + :param active: active module or not + :return: a dict of module info """ - modules = self.get_modules(**kwargs) - active = kwargs.get('active', 'true') - latest_modules = [] - for (name, stream) in set([(m.get('name'), m.get('stream')) for m in modules]): - mods = self.get_modules(name=name, stream=stream, active=active) - latest_modules.append(sorted(mods, key=lambda x: x['version']).pop()) - return list(filter(lambda x: x in latest_modules, modules)) + modules = self.get_modules(name=name, stream=stream, active=active) + if modules: + return sorted(modules, key=lambda x: x['version']).pop() + return None @freshmaker.utils.retry(wait_on=(requests.Timeout, requests.ConnectionError), logger=freshmaker.log) - def get_modules(self, **kwargs): + def get_modules(self, latest_only=False, **kwargs): """ Query PDC with specified query parameters and return a list of modules. + :param latest_only: if this is True, only the latest modules which has + the newest version for each (name, stream) will be + included in the returned result :param kwargs: query parameters in keyword arguments :return: a list of modules """ modules = self.session['modules'](page_size=-1, **kwargs) + + # latest_only means if a module is not the latest one for (name, stream), + # it will be excluded from the result. + # + # For example, if we have the following modules in PDC: + # + # [{'name': 'testmodule', 'stream': 'master', 'version': '123'}, + # {'name': 'testmodule', 'stream': 'master', 'version': '124'}, + # {'name': 'testmodule', 'stream': 'master', 'version': '125'}, + # {'name': 'testmodule2', 'stream': 'master', 'version': '222'}, + # {'name': 'testmodule2', 'stream': 'master', 'version': '333'}] + # + # And we have returned result from above call: + # [{'name': 'testmodule', 'stream': 'master', 'version': '123'}, + # {'name': 'testmodule', 'stream': 'master', 'version': '124'}, + # {'name': 'testmodule2', 'stream': 'master', 'version': '222'}, + # {'name': 'testmodule2', 'stream': 'master', 'version': '333'}] + # + # Then if latest_only is True, we will have: + # + # [{'name': 'testmodule2', 'stream': 'master', 'version': '333'}] + # + # as the final result. + + if latest_only: + active = kwargs.get('active', True) + latest_modules = [] + for (name, stream) in set([(m.get('name'), m.get('stream')) for m in modules]): + latest_modules.append(self.get_latest_module(name, stream, active=active)) + modules = list(filter(lambda x: x in latest_modules, modules)) return modules @freshmaker.utils.retry(wait_on=(requests.Timeout, requests.ConnectionError), logger=freshmaker.log) diff --git a/tests/test_git_rpm_spec_change_handler.py b/tests/test_git_rpm_spec_change_handler.py index 568e21c..73435aa 100644 --- a/tests/test_git_rpm_spec_change_handler.py +++ b/tests/test_git_rpm_spec_change_handler.py @@ -90,7 +90,7 @@ class GitRPMSpecChangeHandlerTest(helpers.ModelsTestCase): mod_info.add_rpm("bash-1.2.3-4.f26.rpm") mod = mod_info.produce() pdc = PDC.return_value - pdc.get_latest_modules.return_value = [mod] + pdc.get_modules.return_value = [mod] commitid = '9287eb8eb4c4c60f73b4a59f228a673846d940c6' utils.bump_distgit_repo.return_value = commitid diff --git a/tests/test_mbs_module_state_change_handler.py b/tests/test_mbs_module_state_change_handler.py index 6edd674..ddf301f 100644 --- a/tests/test_mbs_module_state_change_handler.py +++ b/tests/test_mbs_module_state_change_handler.py @@ -75,7 +75,7 @@ class MBSModuleStateChangeHandlerTest(helpers.ModelsTestCase): mod3_r1 = mod3_r1_info.produce() pdc = PDC.return_value - pdc.get_latest_modules.return_value = [mod2_r1, mod3_r1] + pdc.get_modules.return_value = [mod2_r1, mod3_r1] conf.git_base_url = "git://pkgs.fedoraproject.org" utils.bump_distgit_repo.side_effect = [ @@ -128,7 +128,7 @@ class MBSModuleStateChangeHandlerTest(helpers.ModelsTestCase): mod2 = mod2_info.produce() pdc = PDC.return_value - pdc.get_latest_modules.return_value = [mod2] + pdc.get_modules.return_value = [mod2] handler = MBSModuleStateChangeHandler() handler.build_module = mock.Mock() @@ -190,7 +190,7 @@ class MBSModuleStateChangeHandlerTest(helpers.ModelsTestCase): # we received module built event of module1 msg = helpers.ModuleStateChangeMessage('module1', 'master', state='ready', build_id=123).produce() event = self.get_event_from_msg(msg) - pdc.get_latest_modules.return_value = [mod2] + pdc.get_modules.return_value = [mod2] handler.build_module = mock.Mock() handler.build_module.return_value = 124 @@ -201,7 +201,7 @@ class MBSModuleStateChangeHandlerTest(helpers.ModelsTestCase): # we received module built event of module2 msg = helpers.ModuleStateChangeMessage('module2', 'master', state='ready', build_id=124).produce() event = self.get_event_from_msg(msg) - pdc.get_latest_modules.return_value = [mod3] + pdc.get_modules.return_value = [mod3] handler.build_module = mock.Mock() handler.build_module.return_value = 125 @@ -212,7 +212,7 @@ class MBSModuleStateChangeHandlerTest(helpers.ModelsTestCase): # we received module built event of module3 msg = helpers.ModuleStateChangeMessage('module3', 'master', state='ready', build_id=125).produce() event = self.get_event_from_msg(msg) - pdc.get_latest_modules.return_value = [mod1] + pdc.get_modules.return_value = [mod1] handler.build_module = mock.Mock() handler.build_module.return_value = 126 @@ -223,7 +223,7 @@ class MBSModuleStateChangeHandlerTest(helpers.ModelsTestCase): # we received module built event of module1 msg = helpers.ModuleStateChangeMessage('module1', 'master', state='ready', build_id=126).produce() event = self.get_event_from_msg(msg) - pdc.get_latest_modules.return_value = [mod2] + pdc.get_modules.return_value = [mod2] handler.build_module = mock.Mock() # but this time we should not rebuild module2 diff --git a/tests/test_pdc.py b/tests/test_pdc.py index a49c520..b6d8362 100644 --- a/tests/test_pdc.py +++ b/tests/test_pdc.py @@ -22,78 +22,75 @@ import unittest -from mock import call, patch +from mock import patch from freshmaker import conf from freshmaker.pdc import PDC -class TestGetLatestModules(unittest.TestCase): - """Test PDC.get_latest_modules""" - - @patch('freshmaker.pdc.PDC.get_modules') - def test_exclude_modules_that_doesnt_depend_on_built_module(self, get_modules): - get_modules.side_effect = [ - # modules returned from first call - [{'name': '389-ds', 'stream': '1.2', 'version': '20171009091843'}, - {'name': '389-ds', 'stream': '1.2', 'version': '20171012150041'}, - {'name': 'apache-commons', 'stream': 'f27', 'version': '20171010111836'}], - - # modules returned from call for name 386-ds and stream 1.2 - [{'name': '389-ds', 'stream': '1.2', 'version': '20171009105405'}, - {'name': '389-ds', 'stream': '1.2', 'version': '20171012150041'}, - - # *** This is a new version module that already depends on other module. - {'name': '389-ds', 'stream': '1.2', 'version': '20171120124934'}], - - # modules returned from call for name apache-commons and stream f27 - [{'name': 'apache-commons', 'stream': 'f27', 'version': '20171010111836'}] - ] - +class TestPDC(unittest.TestCase): + """Test PDC """ + def setUp(self): + super(TestPDC, self).setUp() + self._fake_pdc_session = {} + self._fake_pdc_session['modules'] = self._pdc_modules_side_effect + self.patch_pdc_session = patch('freshmaker.pdc.PDC.get_client_session') + self.patched_pdc_session = self.patch_pdc_session.start() + self.patched_pdc_session.return_value = self._fake_pdc_session + + def tearDown(self): + super(TestPDC, self).tearDown() + self.patch_pdc_session.stop() + + def _pdc_modules_side_effect(self, **kwargs): + name = kwargs.get('name', None) + stream = kwargs.get('stream', None) + build_dep_name = kwargs.get('build_dep_name', None) + build_dep_stream = kwargs.get('build_dep_stream', None) + + if name == 'testmodule2' and stream == 'master': + return [ + {'name': 'testmodule2', 'stream': 'master', 'version': '123'}, + {'name': 'testmodule2', 'stream': 'master', 'version': '124'}, + {'name': 'testmodule2', 'stream': 'master', 'version': '125'}, + ] + + if name == 'testmodule3' and stream == 'master': + return [ + {'name': 'testmodule3', 'stream': 'master', 'version': '233'}, + {'name': 'testmodule3', 'stream': 'master', 'version': '234'}, + ] + + if build_dep_name == 'testmodule' and build_dep_stream == 'master': + return [ + {'name': 'testmodule2', 'stream': 'master', 'version': '123'}, + {'name': 'testmodule2', 'stream': 'master', 'version': '124'}, + {'name': 'testmodule3', 'stream': 'master', 'version': '233'}, + {'name': 'testmodule3', 'stream': 'master', 'version': '234'}, + ] + + def test_get_latest_module(self): pdc = PDC(conf) - modules = pdc.get_latest_modules(build_dep_name='rebuilt module', - build_dep_stream='1.7', - active=True) + module = pdc.get_latest_module(name='testmodule2', + stream='master') - expected_modules = [ - {'name': 'apache-commons', 'stream': 'f27', 'version': '20171010111836'}, - ] - self.assertEqual(expected_modules, modules) + expected_module = {'name': 'testmodule2', 'stream': 'master', 'version': '125'} + self.assertEqual(expected_module, module) - @patch('freshmaker.pdc.PDC.get_modules') - def test_found_latest_modules(self, get_modules): - get_modules.side_effect = [ - # modules returned from first call - [{'name': '389-ds', 'stream': '1.2', 'version': '20171009091843'}, - {'name': '389-ds', 'stream': '1.2', 'version': '20171012150041'}, - {'name': '389-ds', 'stream': '1.2', 'version': '20171120124934'}, - {'name': 'apache-commons', 'stream': 'f27', 'version': '20171010111836'}], - - # modules returned from call for name 386-ds and stream 1.2 - [{'name': '389-ds', 'stream': '1.2', 'version': '20171009105405'}, - {'name': '389-ds', 'stream': '1.2', 'version': '20171012150041'}, - {'name': '389-ds', 'stream': '1.2', 'version': '20171120124934'}], - - # modules returned from call for name apache-commons and stream f27 - [{'name': 'apache-commons', 'stream': 'f27', 'version': '20171010111836'}] - ] + module = pdc.get_latest_module(name='testmodule3', + stream='master') + + expected_module = {'name': 'testmodule3', 'stream': 'master', 'version': '234'} + self.assertEqual(expected_module, module) + def test_get_latest_only_modules(self): pdc = PDC(conf) - modules = pdc.get_latest_modules(build_dep_name='rebuilt module', - build_dep_stream='1.7', - active=True) + modules = pdc.get_modules(latest_only=True, + build_dep_name='testmodule', + build_dep_stream='master', + active=True) - modules = sorted(modules, key=lambda m: m['name']) expected_modules = [ - {'name': '389-ds', 'stream': '1.2', 'version': '20171120124934'}, - {'name': 'apache-commons', 'stream': 'f27', 'version': '20171010111836'}, + {'name': 'testmodule3', 'stream': 'master', 'version': '234'}, ] self.assertEqual(expected_modules, modules) - - get_modules.assert_has_calls([ - call(build_dep_name='rebuilt module', - build_dep_stream='1.7', - active=True), - call(name='389-ds', stream='1.2', active=True), - call(name='apache-commons', stream='f27', active=True), - ], any_order=True)