PR#21: code refactor and feature of rebuilding modules when rpm spec is updated - freshmaker

freshmaker

#21 code refactor and feature of rebuilding modules when rpm spec is updated

Merged 8 years ago by qwan. Opened 8 years ago by qwan.

qwan/freshmaker rebuild-modules-on-rpm-spec-change into master

Rebuild module when spec file of rpm in module is updated

Qixiang Wan • 8 years ago

af893f8

Refactor pdc.get_modules

Qixiang Wan • 8 years ago

7c81e5e

freshmaker/events.py

file modified

+15

		`@@ -149,6 +149,21 @@`
		`self.branch = branch`


		`+ class RPMSpecUpdated(BaseEvent):`
		`+ """`
		`+ Provides an event object for "RPM spec file in dist-git updated".`
		`+`
		`+ :param rpm: RPM name, also known as the name of component or source package.`
		`+ :param branch: Branch of updated RPM spec.`
		`+ :param rev: revision.`
		`+ """`
		`+ def __init__(self, msg_id, rpm, branch, rev):`
		`+ super(RPMSpecUpdated, self).__init__(msg_id)`
		`+ self.rpm = rpm`
		`+ self.branch = branch`
		`+ self.rev = rev`
		`+`
		`+`
		`class TestingEvent(BaseEvent):`
		`"""`
		`Event useds in unit-tests.`

freshmaker/handlers/mbs.py

file modified

+52 -30

		`@@ -23,9 +23,9 @@`

		`import requests`

		`- from freshmaker import log, conf, pdc, utils`
		`+ from freshmaker import log, conf, utils, pdc`
mjia commented 8 years ago It seems like this is an irrelevant change.
		`from freshmaker.handlers import BaseHandler`
		`- from freshmaker.events import ModuleBuilt, ModuleMetadataUpdated`
		`+ from freshmaker.events import ModuleBuilt, ModuleMetadataUpdated, RPMSpecUpdated`


		`class MBS(BaseHandler):`
		`@@ -42,6 +42,9 @@`
		`if isinstance(event, ModuleMetadataUpdated):`
		`return True`

		`+ if isinstance(event, RPMSpecUpdated):`
		`+ return True`
		`+`
		`return False`

		`def rebuild_module(self, scm_url, branch):`
		`@@ -64,6 +67,23 @@`
		`log.error("Error when triggering rebuild of %s: %s", scm_url, data)`
		`return None`

		`+ def bump_and_rebuild_module(self, name, branch, commit_msg=None):`
		`+ """Bump module repo with an empty commit and submit a module build request to MBS"""`
		`+ commitid = None`
		`+ with utils.temp_dir(prefix='freshmaker-%s-' % name) as repodir:`
		`+ try:`
		`+ utils.clone_module_repo(name, repodir, branch=branch, user=conf.git_user, logger=log)`
		`+ msg = commit_msg or "Bump"`
		`+ utils.add_empty_commit(repodir, msg=msg, logger=log)`
		`+ commitid = utils.get_commit_hash(repodir)`
		`+ utils.push_repo(repodir)`
		`+ except Exception:`
		`+ log.exception("Failed to update module repo of '%s-%s'.", name, branch)`
		`+`
		`+ if commitid is not None:`
		`+ scm_url = conf.git_base_url + '/modules/%s.git?#%s' % (name, commitid)`
		`+ self.rebuild_module(scm_url, branch)`
		`+`
		`def handle_metadata_update(self, event):`
		`log.info("Triggering rebuild of %s, metadata updated", event.scm_url)`
		`self.rebuild_module(event.scm_url, event.branch)`
		`@@ -83,35 +103,35 @@`
		`"in MBS", module_name, module_stream)`

		`pdc_session = pdc.get_client_session(conf)`
		`- depending_modules = pdc.get_modules(pdc_session,`
		`- build_dep_name=module_name,`
		`- build_dep_stream=module_stream,`
		`- active=True)`
		`-`
		`- # only rebuild the latest (by cmp variant_release) modules of`
		`- # (variant_name, variant_version)`
		`- latest_modules = []`
		`- for (name, version) in set([(m.get('variant_name'), m.get('variant_version')) for m in depending_modules]):`
		`- mods = pdc.get_modules(pdc_session, name=name, version=version, active=True)`
		`- latest_modules.append(sorted(mods, key=lambda x: x['variant_release']).pop())`
		`-`
		`- rebuild_modules = list(filter(lambda x: x in latest_modules, depending_modules))`
		`- for mod in rebuild_modules:`
		`+ modules = pdc.get_latest_modules(pdc_session,`
		`+ build_dep_name=module_name,`
		`+ build_dep_stream=module_stream,`
		`+ active='true')`
		`+ for mod in modules:`
		`+ commit_msg = "Bump to rebuild because of %s update" % module_name`
mjia commented 8 years ago s/update/updated
		`+ self.bump_and_rebuild_module(mod['variant_name'],`
		`+ mod['variant_version'],`
		`+ commit_msg=commit_msg)`
		`+ return []`
		`+`
		`+ def handle_rpm_spec_updated(self, event):`
		`+ """`
		`+ Rebuild module when spec file of rpm in module is updated.`
		`+ """`
		`+ rpm = event.rpm`
		`+ branch = event.branch`
		`+ rev = event.rev`
mjia commented 8 years ago It would be good to log a message here for debugging purpose, similar to the one in def handle_metadata_update.
		`+ pdc_session = pdc.get_client_session(conf)`
		`+ modules = pdc.get_latest_modules(pdc_session,`
		`+ component_name=rpm,`
		`+ component_branch=branch,`
		`+ active='true')`
		`+ for mod in modules:`
		`module_name = mod['variant_name']`
		`- module_stream = mod['variant_version']`
		`- commitid = None`
		`- with utils.temp_dir(prefix='freshmaker-%s-' % module_name) as repodir:`
		`- try:`
		`- utils.clone_module_repo(module_name, repodir, branch=module_stream, user=conf.git_user, logger=log)`
		`- utils.add_empty_commit(repodir, msg="Bumped to rebuild because of %s update" % module_name, logger=log)`
		`- commitid = utils.get_commit_hash(repodir)`
		`- utils.push_repo(repodir)`
		`- except Exception:`
		`- log.exception("Failed to update module repo for '%s-%s'.", module_name, module_stream)`
		`-`
		`- if commitid is not None:`
		`- scm_url = conf.git_base_url + '/modules/%s.git?#%s' % (module_name, commitid)`
		`- self.rebuild_module(scm_url, module_stream)`
		`+ module_branch = mod['variant_version']`
		`+ log.info("Going to rebuild module '%s:%s'.", module_name, module_branch)`
		`+ commit_msg = "Bump to rebuild because of %s rpm spec update (%s)." % (rpm, rev)`
		`+ self.bump_and_rebuild_module(module_name, module_branch, commit_msg=commit_msg)`

		`return []`

		`@@ -120,5 +140,7 @@`
		`return self.handle_metadata_update(event)`
		`elif isinstance(event, ModuleBuilt):`
		`return self.handle_module_built(event)`
		`+ elif isinstance(event, RPMSpecUpdated):`
		`+ return self.handle_rpm_spec_updated(event)`

		`return []`

freshmaker/parsers/gitreceive.py

file modified

		`@@ -25,6 +25,7 @@`
		`from freshmaker.parsers import BaseParser`
		`from freshmaker.events import ModuleMetadataUpdated`
		`from freshmaker.events import DockerfileChanged`
		`+ from freshmaker.events import RPMSpecUpdated`


		`class GitReceiveParser(BaseParser):`
		`@@ -78,5 +79,13 @@`
		`repo_url = '{}/{}/{}.git'.format(conf.git_base_url, namespace, repo)`
		`return DockerfileChanged(msg_id, repo_url=repo_url, branch=branch,`
		`namespace=namespace, repo=repo, rev=rev)`
		`+ elif namespace == 'rpms':`
		`+ component = commit.get('repo')`
		`+ branch = commit.get('branch')`
		`+ rev = commit.get('rev')`
		`+ changed_files = commit.get('stats', {}).get('files', {}).keys()`
		`+ has_spec = any([i.endswith('.spec') for i in changed_files])`
		`+ if has_spec:`
		`+ return RPMSpecUpdated(msg_id, component, branch, rev=rev)`

		`return None`

freshmaker/pdc.py

file modified

+25 -15

		`@@ -50,22 +50,32 @@`


		`@freshmaker.utils.retry(wait_on=(requests.ConnectTimeout, requests.ConnectionError), logger=freshmaker.log)`
		`- def get_modules(pdc_session, name=None, version=None, build_dep_name=None, build_dep_stream=None, active=True):`
		`+ def get_modules(pdc_session, **kwargs):`
		`"""`
		`+ Query PDC with specified query parameters and return a list of modules.`
		`+`
		`:param pdc_session: PDCClient instance`
		`- :return: list of modules`
		`+ :param kwargs: query parameters in keyword arguments`
		`+ :return: a list of modules`
		`"""`
		`- query = {}`
		`- if name is not None:`
		`- query['variant_name'] = name`
		`- if version is not None:`
		`- query['variant_version'] = version`
		`- if build_dep_name is not None:`
		`- query['build_dep_name'] = build_dep_name`
		`- if build_dep_stream is not None:`
		`- query['build_dep_stream'] = build_dep_stream`
		`- if active:`
		`- query['active'] = 'true'`
		`-`
		`- modules = pdc_session['unreleasedvariants'](page_size=-1, **query)`
		`+ modules = pdc_session['unreleasedvariants'](page_size=-1, **kwargs)`
		`return modules`
		`+`
		`+`
		`+ def get_latest_modules(pdc_session, **kwargs):`
		`+ """`
		`+ Query PDC with query parameters in kwargs and return a list of modules`
		`+ which contains latest modules of each (module_name, module_version).`
		`+`
		`+ :param pdc_session: PDCClient instance`
		`+ :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`
		`+ """`
		`+ modules = get_modules(pdc_session, **kwargs)`
		`+ active = kwargs.get('active', 'true')`
		`+ latest_modules = []`
		`+ for (name, version) in set([(m.get('variant_name'), m.get('variant_version')) for m in modules]):`
		`+ mods = get_modules(pdc_session, variant_name=name, variant_version=version, active=active)`
		`+ latest_modules.append(sorted(mods, key=lambda x: x['variant_release']).pop())`
		`+ return list(filter(lambda x: x in latest_modules, modules))`

tests/helpers.py

file modified

+63 -7

		`@@ -43,10 +43,13 @@`
		`self.source_name = 'unittest',`
		`self.source_version = '0.1.1',`
		`self.timestamp = time.time()`
		`- self.topic = 'org.fedoraproject.prod.mbs.module.state.change'`
		`+ self.topic = ''`
		`self.username = 'freshmaker'`
		`self.i = random.randint(0, 100)`
		`- self.inner_msg = {}`
		`+`
		`+ @property`
		`+ def inner_msg(self):`
		`+ return {}`

		`def produce(self):`
		`message_body = {`
		`@@ -69,24 +72,27 @@`
		`class ModuleBuiltMessage(FedMsgFactory):`
		`def __init__(self, name, stream, state='ready', build_id=None, args, *kwargs):`
		`super(ModuleBuiltMessage, self).__init__(args, *kwargs)`
		`- states_dict = {}`
		`+ self.topic = 'org.fedoraproject.prod.mbs.module.state.change'`
		`self.name = name`
		`self.stream = stream`
		`self.state = state`
		`self.build_id = build_id if build_id else random.randint(0, 1000)`
		`self.scmurl = "git://pkgs.fedoraproject.org/modules/%s?#%s" % (self.name, '123')`

		`+ self._states_dict = {}`
		`for state, code in six.iteritems(BUILD_STATES):`
		`- states_dict[state] = {'state_name': state, 'state': code}`
		`+ self._states_dict[state] = {'state_name': state, 'state': code}`

		`- inner_msg = {`
		`+ @property`
		`+ def inner_msg(self):`
		`+ return {`
		`'component_builds': [],`
		`'id': self.build_id,`
		`'modulemd': '',`
		`'name': self.name,`
		`'owner': 'freshmaker',`
		`'scmurl': self.scmurl,`
		`- 'state': states_dict[self.state]['state'],`
		`+ 'state': self._states_dict[self.state]['state'],`
		`'state_name': self.state,`
		`'state_reason': None,`
		`'state_trace': [],`
		`@@ -98,7 +104,52 @@`
		`'time_submitted': time.time(),`
		`'version': time.strftime("%Y%m%d%H%M%S"),`
		`}`
		`- self.inner_msg = inner_msg`
		`+`
		`+`
		`+ class DistGitMessage(FedMsgFactory):`
		`+ def __init__(self, namespace, repo, branch, rev, args, *kwargs):`
		`+ super(DistGitMessage, self).__init__(args, *kwargs)`
		`+ self.topic = 'org.fedoraproject.prod.git.receive'`
		`+ self.namespace = namespace`
		`+ self.repo = repo`
		`+ self.branch = branch`
		`+ self.rev = rev`
		`+ self.stats = {'files': {},`
		`+ 'total': {`
		`+ 'additions': 0,`
		`+ 'deletions': 0,`
		`+ 'files': 0,`
		`+ 'lines': 0,`
		`+ }}`
		`+`
		`+ @property`
		`+ def inner_msg(self):`
		`+ return {`
		`+ 'commit': {`
		`+ 'repo': self.repo,`
		`+ 'namespace': self.namespace,`
		`+ 'branch': self.branch,`
		`+ 'rev': self.rev,`
		`+ 'agent': 'freshmaker',`
		`+ 'name': 'freshmaker',`
		`+ 'username': 'freshmaker',`
		`+ 'email': 'freshmaker@example.com',`
		`+ 'message': 'test message',`
		`+ 'summary': 'test',`
		`+ 'path': "/srv/git/repositories/%s/%s.git" % (self.namespace, self.repo),`
		`+ 'seen': False,`
		`+ 'stats': self.stats,`
		`+ }`
		`+ }`
		`+`
		`+ def add_changed_file(self, filename, additions, deletions):`
		`+ self.stats['files'].setdefault(filename, {})['additions'] = additions`
		`+ self.stats['files'][filename]['deletions'] = deletions`
		`+ self.stats['files'][filename]['lines'] = additions + deletions`
		`+ self.stats['total']['additions'] += additions`
		`+ self.stats['total']['deletions'] += deletions`
		`+ self.stats['total']['files'] += 1`
		`+ self.stats['total']['lines'] += self.stats['files'][filename]['lines']`


		`class PDCModuleInfoFactory(object):`
		`@@ -114,6 +165,7 @@`
		`self.build_deps = []`
		`self.runtime_deps = []`
		`self.koji_tag = 'module-%s' % ''.join([random.choice(string.ascii_letters[:6] + string.digits) for n in range(16)])`
		`+ self.rpms = []`

		`def produce(self):`
		`module = {`
		`@@ -128,6 +180,7 @@`
		`'koji_tag': self.koji_tag,`
		`'build_deps': self.build_deps,`
		`'runtime_deps': self.runtime_deps,`
		`+ 'rpms': self.rpms,`
		`}`
		`return module`

		`@@ -135,3 +188,6 @@`
		`class PDCModuleInfo(PDCModuleInfoFactory):`
		`def add_build_dep(self, name, stream):`
		`self.build_deps.append({'dependency': name, 'stream': stream})`
		`+`
		`+ def add_rpm(self, rpm):`
		`+ self.rpms.append(rpm)`

tests/test_mbs_handler.py

file modified

+72 -8

		`@@ -29,11 +29,13 @@`
		`from freshmaker import events`
		`from freshmaker.handlers.mbs import MBS`
		`from freshmaker.parsers.mbsmodule import MBSModuleParser`
		`+ from freshmaker.parsers.gitreceive import GitReceiveParser`


		`class MBSHandlerTest(unittest.TestCase):`
		`def setUp(self):`
		`events.BaseEvent.register_parser(MBSModuleParser)`
		`+ events.BaseEvent.register_parser(GitReceiveParser)`

		`def _get_event(self, message):`
		`event = events.BaseEvent.from_fedmsg(message['body']['topic'], message['body'])`
		`@@ -62,10 +64,10 @@`
		`handler = MBS()`
		`self.assertFalse(handler.can_handle(event))`

		`+ @mock.patch('freshmaker.pdc.get_modules')`
		`@mock.patch('freshmaker.handlers.mbs.utils')`
		`- @mock.patch('freshmaker.handlers.mbs.pdc')`
		`@mock.patch('freshmaker.handlers.mbs.conf')`
		`- def test_rebuild_depending_modules_on_module_built_event(self, conf, pdc, utils):`
		`+ def test_rebuild_depending_modules_on_module_built_event(self, conf, utils, get_modules):`
		`"""`
		`Tests MBS handler can rebuild all modules which depend on the module`
		`in module built event.`
		`@@ -83,7 +85,9 @@`
		`mod3_r1_info.add_build_dep('testmodule', 'master')`
		`mod3_r1 = mod3_r1_info.produce()`

		`- def get_modules(pdc_session, name=None, version=None, build_dep_name=None, build_dep_stream=None, active=True):`
		`+ def mock_get_modules(pdc_session, **kwargs):`
		`+ name = kwargs.get('variant_name', None)`
		`+ version = kwargs.get('variant_version', None)`

		`if name == 'testmodule2' and version == 'master':`
		`return [mod2_r1]`
		`@@ -92,7 +96,7 @@`
		`else:`
		`return [mod2_r1, mod3_r1]`

		`- pdc.get_modules.side_effect = get_modules`
		`+ get_modules.side_effect = mock_get_modules`
		`conf.git_base_url = "git://pkgs.fedoraproject.org"`
		`utils.get_commit_hash.side_effect = [`
		`"fae7848fa47a854f25b782aa64441040a6d86544",`
		`@@ -105,10 +109,10 @@`
		`[mock.call(u'git://pkgs.fedoraproject.org/modules/testmodule2.git?#fae7848fa47a854f25b782aa64441040a6d86544', u'master'),`
		`mock.call(u'git://pkgs.fedoraproject.org/modules/testmodule3.git?#43ec03000d249231bc7135b11b810afc96e90efb', u'master')])`

		`+ @mock.patch('freshmaker.pdc.get_modules')`
		`@mock.patch('freshmaker.handlers.mbs.utils')`
		`- @mock.patch('freshmaker.handlers.mbs.pdc')`
		`@mock.patch('freshmaker.handlers.mbs.conf')`
		`- def test_only_rebuild_latest_depending_modules_on_module_built_event(self, conf, pdc, utils):`
		`+ def test_only_rebuild_latest_depending_modules_on_module_built_event(self, conf, utils, get_modules):`
		`"""`
		`Tests MBS handler only rebuild latest depending modules. If there is a`
		`module only has old release depends on the module, it won't be rebuilt.`
		`@@ -130,7 +134,9 @@`
		`mod3_r2_info.add_build_dep('testmodule', 'master')`
		`mod3_r2 = mod3_r2_info.produce()`

		`- def get_modules(pdc_session, name=None, version=None, build_dep_name=None, build_dep_stream=None, active=True):`
		`+ def mock_get_modules(pdc_session, **kwargs):`
		`+ name = kwargs.get('variant_name', None)`
		`+ version = kwargs.get('variant_version', None)`

		`if name == 'testmodule2' and version == 'master':`
		`return [mod2_r1]`
		`@@ -141,7 +147,7 @@`

		`# query for testmodule3 releases, get mod3_r1 and mod3_r2,`
		`# only mod3_r1 depends on testmodule, and r1 < r2.`
		`- pdc.get_modules.side_effect = get_modules`
		`+ get_modules.side_effect = mock_get_modules`
		`conf.git_base_url = "git://pkgs.fedoraproject.org"`
		`utils.get_commit_hash.side_effect = [`
		`"fae7848fa47a854f25b782aa64441040a6d86544",`
		`@@ -153,6 +159,64 @@`
		`self.assertEqual(handler.rebuild_module.call_args_list,`
		`[mock.call(u'git://pkgs.fedoraproject.org/modules/testmodule2.git?#fae7848fa47a854f25b782aa64441040a6d86544', u'master')])`

		`+ def test_can_handle_rpm_spec_updated_event(self):`
		`+ """`
		`+ Tests MBS handler can handle rpm spec updated event`
		`+ """`
		`+ m = helpers.DistGitMessage('rpms', 'bash', 'master', '123')`
		`+ m.add_changed_file('bash.spec', 1, 1)`
		`+ msg = m.produce()`
		`+`
		`+ event = self._get_event(msg)`
		`+`
		`+ handler = MBS()`
		`+ self.assertTrue(handler.can_handle(event))`
		`+`
		`+ def test_can_not_handle_no_spec_updated_dist_git_event(self):`
		`+ """`
		`+ Tests RPMSpechandler can not handle dist git message that`
		`+ spec file is not updated.`
		`+ """`
		`+`
		`+ m = helpers.DistGitMessage('rpms', 'bash', 'master', '123')`
		`+ m.add_changed_file('test.c', 1, 1)`
		`+ msg = m.produce()`
		`+`
		`+ event = self._get_event(msg)`
		`+`
		`+ handler = MBS()`
		`+ self.assertFalse(handler.can_handle(event))`
		`+`
		`+ @mock.patch('freshmaker.handlers.mbs.utils')`
		`+ @mock.patch('freshmaker.handlers.mbs.pdc')`
		`+ @mock.patch('freshmaker.handlers.mbs.conf')`
		`+ def test_trigger_module_rebuild_when_rpm_spec_updated(self, conf, pdc, utils):`
		`+ """`
		`+ Test RPMSpecHandler can trigger module rebuild when spec`
		`+ file of rpm in module updated.`
		`+ """`
		`+ conf.git_base_url = "git://pkgs.fedoraproject.org"`
		`+`
		`+ m = helpers.DistGitMessage('rpms', 'bash', 'master', '123')`
		`+ m.add_changed_file('bash.spec', 1, 1)`
		`+ msg = m.produce()`
		`+`
		`+ event = self._get_event(msg)`
		`+`
		`+ mod_info = helpers.PDCModuleInfo('testmodule', 'master', '20170412010101')`
		`+ mod_info.add_rpm("bash-1.2.3-4.f26.rpm")`
		`+ mod = mod_info.produce()`
		`+ pdc.get_latest_modules.return_value = [mod]`
		`+`
		`+ commitid = '9287eb8eb4c4c60f73b4a59f228a673846d940c6'`
		`+ utils.get_commit_hash.return_value = commitid`
		`+`
		`+ handler = MBS()`
		`+ handler.rebuild_module = mock.Mock()`
		`+ handler.handle(event)`
		`+ self.assertEqual(handler.rebuild_module.call_args_list,`
		`+ [mock.call('git://pkgs.fedoraproject.org/modules/testmodule.git?#%s' % commitid, 'master')])`
		`+`

		`if __name__ == '__main__':`
		`unittest.main()`

qwan commented 8 years ago

Note: the feature of rebuilding modules when rpm spec is update requires this PR https://github.com/fedora-modularity/product-definition-center/pull/15 in pdc to support filtering modules by rpm name and branch.

cqi commented on line 6 of freshmaker/pdc.py 8 years ago

If change kwargs to something like query_params, it would be more straightforward and easy to understand.

cqi commented on line 11 of freshmaker/events.py 8 years ago

Should these params be the docstring of __init__?

cqi commented on line 9 of freshmaker/events.py 8 years ago

It would be nice to make RPM name more clear?

cqi commented 8 years ago

Generally LGTM

jkaluza commented 8 years ago

rebased

8 years ago

qwan commented 8 years ago

Just realized it's unnecessary to add a new rpm handler, should add such feature which need to trigger MBS rebuild job in mbs handler to keep it simple, updated.

ralph commented 8 years ago

FYI, https://github.com/fedora-modularity/product-definition-center/pull/15 is merged but currently isn't scheduled to be deployed until next sprint (so we can wait for one more change from @mprahl). If having it in place is urgent, let me know and I can fast track deployment.

Pull-Request has been merged by qwan

8 years ago

mjia commented on line 6 of freshmaker/handlers/mbs.py 8 years ago

It seems like this is an irrelevant change.

mjia commented on line 70 of freshmaker/handlers/mbs.py 8 years ago

s/update/updated

mjia commented on line 82 of freshmaker/handlers/mbs.py 8 years ago

It would be good to log a message here for debugging purpose, similar to
the one in def handle_metadata_update.

qwan commented 8 years ago

@mjia Thanks for reviewing, it was merged :( but luckily these issues won't break the functionalities, so I can update them in later patches :)