PR#17: Rebuild depending modules when receives module built message - freshmaker

freshmaker

#17 Rebuild depending modules when receives module built message

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

qwan/freshmaker rebuild-modules-on-modulebuilt-event into master

Rebuild depending modules when receives module built message

Qixiang Wan • 8 years ago

0dee476

Fix consumer's topics are not subscribed

Qixiang Wan • 8 years ago

3bbdf98

conf/config.py

file modified

		`@@ -39,12 +39,21 @@`
		`# Base URL of git repository with source artifacts.`
		`GIT_BASE_URL = "git://pkgs.fedoraproject.org"`

		`+ # SSH base URL of git repository`
		`+ GIT_SSH_BASE_URL = "ssh://%s@pkgs.fedoraproject.org/"`
		`+`
		`+ # GIT user for cloning and pushing repo`
		`+ GIT_USER = ""`
		`+`
		`# Base URL of Module Build Service.`
		`MBS_BASE_URL = "https://mbs.fedoraproject.org"`

		`# Authorization token to use when communicating with MBS.`
		`MBS_AUTH_TOKEN = ""`

		`+ # PDC API URL`
		`+ PDC_URL = 'http://pdc.fedoraproject.org/rest_api/v1'`
		`+`
		`# Read Koji configuration from profile instead of reading them from`
		`# configuration file directly. For staging Koji, it is stg.`
		`KOJI_PROFILE = 'koji'`

freshmaker/config.py

file modified

+12

		`@@ -139,6 +139,18 @@`
		`'type': str,`
		`'default': "git://pkgs.fedoraproject.org",`
		`'desc': 'Dist-git base URL.'},`
		`+ 'git_ssh_base_url': {`
		`+ 'type': str,`
		`+ 'default': "ssh://%s@pkgs.fedoraproject.org/",`
		`+ 'desc': 'Dist-git ssh base URL.'},`
		`+ 'git_user': {`
		`+ 'type': str,`
		`+ 'default': '',`
		`+ 'desc': 'User for git operations.'},`
		`+ 'git_author': {`
		`+ 'type': str,`
		`+ 'default': 'Freshmaker <freshmaker-owner@fedoraproject.org>',`
		`+ 'desc': 'Author for git commit.'},`
		`'mbs_base_url': {`
		`'type': str,`
		`'default': "https://mbs.fedoraproject.org",`

freshmaker/consumer.py

file modified

+2 -2

		`@@ -42,10 +42,10 @@`
		`config_key = 'freshmakerconsumer'`

		`def __init__(self, hub):`
		`- super(FreshmakerConsumer, self).__init__(hub)`
		`-`
		`+ # set topic before super, otherwise topic will not be subscribed`
		`self.handlers = list(freshmaker.handlers.load_handlers())`
		`self.register_parsers()`
		`+ super(FreshmakerConsumer, self).__init__(hub)`

		`# These two values are typically provided either by the unit tests or`
		`# by the local build command. They are empty in the production environ`

freshmaker/events.py

file modified

+3 -1

		`@@ -129,10 +129,12 @@`
		`:param module_build_id: the id of the module build`
		`:param module_build_state: the state of the module build`
		`"""`
		`- def __init__(self, msg_id, module_build_id, module_build_state):`
		`+ def __init__(self, msg_id, module_build_id, module_build_state, name, stream):`
		`super(ModuleBuilt, self).__init__(msg_id)`
		`self.module_build_id = module_build_id`
		`self.module_build_state = module_build_state`
		`+ self.module_name = name`
		`+ self.module_stream = stream`


		`class ModuleMetadataUpdated(BaseEvent):`

freshmaker/handlers/mbs.py

file modified

+44 -6

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

		`import requests`

		`- from freshmaker import log, conf`
		`+ from freshmaker import log, conf, pdc, utils`
		`from freshmaker.handlers import BaseHandler`
		`- from freshmaker.events import ModuleBuilt, TestingEvent, ModuleMetadataUpdated`
		`+ from freshmaker.events import ModuleBuilt, ModuleMetadataUpdated`


		`class MBS(BaseHandler):`
		`@@ -71,11 +71,49 @@`
		`return []`

		`def handle_module_built(self, event):`
		`- log.info("Triggering rebuild of modules depending on %r "`
		`- "in MBS" % event)`
		`+ """`
		`+ When there is any module built and state is 'ready', query PDC to get`
		`+ all modules that depends on this module, rebuild all these depending`
		`+ modules.`
		`+ """`
		`+ module_name = event.module_name`
		`+ module_stream = event.module_stream`
		`+`
		`+ log.info("Triggering rebuild of modules depending on %s:%s "`
		`+ "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]):`
cqi commented 8 years ago For Python 2.7+, this set can be simplified set((m.get('variant_name'), m.get('variant_version')) for m in depending_modules) I'm also curious why need to remove duplicate pair of `(name, version)`?
qwan commented 8 years ago We need to get the unique result of `(name, version)` and then query PDC to the latest `(name, version)` to filter out any `(name, version)` in `depending_modules` that is not the latest one.
		`+ 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:`
		`+ 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)`

		`- # TODO: Just for initial testing of consumer`
		`- return [TestingEvent("ModuleBuilt handled")]`
		`+ return []`

		`def handle(self, event):`
		`if isinstance(event, ModuleMetadataUpdated):`

freshmaker/parsers/mbsmodule.py

file modified

+5 -2

		`@@ -50,5 +50,8 @@`
		`'topic "{0}"').format(topic))`
		`return None`

		`- return ModuleBuilt(msg_id, msg_inner_msg.get('id'),`
		`- msg_inner_msg.get('state'))`
		`+ return ModuleBuilt(msg_id,`
		`+ msg_inner_msg.get('id'),`
		`+ msg_inner_msg.get('state'),`
		`+ msg_inner_msg.get('name'),`
		`+ msg_inner_msg.get('stream'))`

freshmaker/pdc.py

file added

+71

		`@@ -0,0 +1,71 @@`
		`+ # -- coding: utf-8 --`
		`+ # Copyright (c) 2017 Red Hat, Inc.`
		`+ #`
		`+ # Permission is hereby granted, free of charge, to any person obtaining a copy`
dcallagh commented 8 years ago This looks like an MIT license header, but freshmaker is actually GPLv2+ (like all our other projects...)
dcallagh commented 8 years ago In fact it seems that all the files in freshmaker have the wrong license header, I guess you just copy-pasted this one from one of the others.
		`+ # of this software and associated documentation files (the "Software"), to deal`
		`+ # in the Software without restriction, including without limitation the rights`
		`+ # to use, copy, modify, merge, publish, distribute, sublicense, and/or sell`
		`+ # copies of the Software, and to permit persons to whom the Software is`
		`+ # furnished to do so, subject to the following conditions:`
		`+ #`
		`+ # The above copyright notice and this permission notice shall be included in all`
		`+ # copies or substantial portions of the Software.`
		`+ #`
		`+ # THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR`
		`+ # IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,`
		`+ # FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE`
		`+ # AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER`
		`+ # LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,`
		`+ # OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE`
		`+ # SOFTWARE.`
		`+ #`
		`+`
		`+ import inspect`
		`+ import requests`
		`+ from pdc_client import PDCClient`
		`+`
		`+ import freshmaker`
		`+ import freshmaker.utils`
		`+`
		`+`
		`+ def get_client_session(config):`
		`+ """`
		`+ :param config: instance of freshmaker.config.Config`
		`+ :return: pdc_client.PDCClient instance`
		`+ """`
		`+ if 'ssl_verify' in inspect.getargspec(PDCClient.__init__).args:`
		`+ # New API`
		`+ return PDCClient(`
		`+ server=config.pdc_url,`
		`+ develop=config.pdc_develop,`
		`+ ssl_verify=not config.pdc_insecure,`
		`+ )`
		`+ else:`
		`+ # Old API`
		`+ return PDCClient(`
		`+ server=config.pdc_url,`
		`+ develop=config.pdc_develop,`
		`+ insecure=config.pdc_insecure,`
		`+ )`
		`+`
		`+`
		`+ @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):`
		`+ """`
		`+ :param pdc_session: PDCClient instance`
		`+ :return: 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)`
		`+ return modules`

freshmaker/utils.py

file added

+132

		`@@ -0,0 +1,132 @@`
		`+ # -- coding: utf-8 --`
		`+ # Copyright (c) 2017 Red Hat, Inc.`
		`+ #`
		`+ # Permission is hereby granted, free of charge, to any person obtaining a copy`
		`+ # of this software and associated documentation files (the "Software"), to deal`
		`+ # in the Software without restriction, including without limitation the rights`
		`+ # to use, copy, modify, merge, publish, distribute, sublicense, and/or sell`
		`+ # copies of the Software, and to permit persons to whom the Software is`
		`+ # furnished to do so, subject to the following conditions:`
		`+ #`
		`+ # The above copyright notice and this permission notice shall be included in all`
		`+ # copies or substantial portions of the Software.`
		`+ #`
		`+ # THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR`
		`+ # IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,`
		`+ # FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE`
		`+ # AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER`
		`+ # LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,`
		`+ # OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE`
		`+ # SOFTWARE.`
		`+ #`
		`+`
		`+ import contextlib`
		`+ import errno`
		`+ import functools`
		`+ import getpass`
		`+ import os`
		`+ import shutil`
		`+ import subprocess`
		`+ import tempfile`
		`+ import time`
		`+`
		`+ from freshmaker import conf`
		`+`
		`+`
		`+ def retry(timeout=conf.net_timeout, interval=conf.net_retry_interval, wait_on=Exception, logger=None):`
		`+ """A decorator that allows to retry a section of code until success or timeout."""`
		`+ def wrapper(function):`
		`+ @functools.wraps(function)`
		`+ def inner(args, *kwargs):`
		`+ start = time.time()`
		`+ while True:`
		`+ if (time.time() - start) >= timeout:`
		`+ raise # This re-raises the last exception.`
		`+ try:`
		`+ return function(args, *kwargs)`
		`+ except wait_on as e:`
		`+ if logger is not None:`
		`+ logger.warn("Exception %r raised from %r. Retry in %rs",`
		`+ e, function, interval)`
		`+ time.sleep(interval)`
		`+ return inner`
		`+ return wrapper`
		`+`
		`+`
		`+ def makedirs(path, mode=0o775):`
		`+ try:`
		`+ os.makedirs(path, mode=mode)`
		`+ except OSError as ex:`
		`+ if ex.errno != errno.EEXIST:`
		`+ raise`
		`+`
		`+`
		`+ @contextlib.contextmanager`
		`+ def temp_dir(logger=None, args, *kwargs):`
		`+ """Create a temporary directory and ensure it's deleted."""`
		`+ if kwargs.get('dir'):`
		`+ # If we are supposed to create the temp dir in a particular location,`
		`+ # ensure the location already exists.`
		`+ makedirs(kwargs['dir'])`
		`+ dir = tempfile.mkdtemp(args, *kwargs)`
		`+ try:`
		`+ yield dir`
		`+ finally:`
		`+ try:`
		`+ shutil.rmtree(dir)`
		`+ except OSError as exc:`
		`+ # Okay, we failed to delete temporary dir.`
		`+ if logger:`
		`+ logger.warn('Error removing %s: %s', dir, exc.strerror)`
		`+`
		`+`
		`+ def clone_module_repo(name, dest, branch='master', user=None, logger=None):`
		`+ """Clone a module repo"""`
		`+ if user is None:`
		`+ user = getpass.getuser()`
		`+ cmd = ['git', 'clone', '-b', branch, os.path.join(conf.git_ssh_base_url % user, 'modules', name), dest]`
		`+ _run_command(cmd, logger=logger)`
		`+`
		`+`
		`+ def add_empty_commit(repo, msg="bump", author=None, logger=None):`
		`+ """Commit an empty commit to repo"""`
		`+ if author is None:`
		`+ author = conf.git_author`
		`+ cmd = ['git', 'commit', '--allow-empty', '-m', msg, '--author={}'.format(author)]`
		`+ _run_command(cmd, logger=logger, rundir=repo)`
		`+`
		`+`
		`+ def push_repo(repo, user=None, logger=None):`
		`+ """Push repo"""`
		`+ if user is None:`
		`+ user = getpass.getuser()`
		`+ cmd = ['git', 'push']`
		`+ _run_command(cmd, logger=logger, rundir=repo)`
		`+`
		`+`
		`+ def get_commit_hash(repo, revision='HEAD'):`
		`+ """Get commit hash from revision"""`
		`+ cmd = ['git', 'rev-parse', revision]`
		`+ return _run_command(cmd, rundir=repo, return_output=True).strip()`
		`+`
		`+`
		`+ def _run_command(command, logger=None, rundir='/tmp', output=subprocess.PIPE, error=subprocess.PIPE, env=None, return_output=False):`
		`+ """Run a command, return output if return_output is True. Error out if command exit with non-zero code."""`
		`+`
		`+ if logger:`
		`+ logger.info("Running %s", subprocess.list2cmdline(command))`
		`+`
		`+ p1 = subprocess.Popen(command, cwd=rundir, stdout=output, stderr=error, universal_newlines=True, env=env,`
		`+ close_fds=True)`
		`+ (out, err) = p1.communicate()`
		`+`
		`+ if out and logger:`
		`+ logger.debug(out)`
		`+`
		`+ if p1.returncode != 0:`
		`+ if logger:`
		`+ logger.error("Got an error from %s", command[0])`
		`+ logger.error(err)`
		`+ raise OSError("Got an error (%d) from %s: %s" % (p1.returncode, command[0], err))`
		`+ if return_output:`
		`+ return out`

tests/helpers.py

file added

+137

		`@@ -0,0 +1,137 @@`
		`+ # Copyright (c) 2017 Red Hat, Inc.`
		`+ #`
		`+ # Permission is hereby granted, free of charge, to any person obtaining a copy`
		`+ # of this software and associated documentation files (the "Software"), to deal`
		`+ # in the Software without restriction, including without limitation the rights`
		`+ # to use, copy, modify, merge, publish, distribute, sublicense, and/or sell`
		`+ # copies of the Software, and to permit persons to whom the Software is`
		`+ # furnished to do so, subject to the following conditions:`
		`+ #`
		`+ # The above copyright notice and this permission notice shall be included in all`
		`+ # copies or substantial portions of the Software.`
		`+ #`
		`+ # THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR`
		`+ # IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,`
		`+ # FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE`
		`+ # AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER`
		`+ # LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,`
		`+ # OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE`
		`+ # SOFTWARE.`
		`+`
		`+ import random`
		`+ import six`
		`+ import string`
		`+ import time`
		`+ import uuid`
		`+`
		`+`
		`+ BUILD_STATES = {`
		`+ "init": 0,`
		`+ "wait": 1,`
		`+ "build": 2,`
		`+ "done": 3,`
		`+ "failed": 4,`
		`+ "ready": 5,`
		`+ }`
		`+`
		`+`
		`+ class FedMsgFactory(object):`
		`+ def __init__(self, args, *kwargs):`
		`+ self.msg_id = "%s-%s" % (time.strftime("%Y"), uuid.uuid4())`
		`+ self.msg = {}`
		`+ self.signature = '123'`
		`+ self.source_name = 'unittest',`
		`+ self.source_version = '0.1.1',`
		`+ self.timestamp = time.time()`
		`+ self.topic = 'org.fedoraproject.prod.mbs.module.state.change'`
		`+ self.username = 'freshmaker'`
		`+ self.i = random.randint(0, 100)`
		`+ self.inner_msg = {}`
		`+`
		`+ def produce(self):`
		`+ message_body = {`
		`+ 'i': self.i,`
		`+ 'msg_id': self.msg_id,`
		`+ 'topic': self.topic,`
		`+ 'username': self.username,`
		`+ 'timestamp': self.timestamp,`
		`+ 'signature': self.signature,`
		`+ 'source_name': self.source_name,`
		`+ 'source_version': self.source_version,`
		`+ 'msg': self.inner_msg,`
		`+ }`
		`+ return {`
		`+ 'body': message_body,`
		`+ 'topic': self.topic`
		`+ }`
		`+`
		`+`
		`+ class ModuleBuiltMessage(FedMsgFactory):`
		`+ def __init__(self, name, stream, state='ready', build_id=None, args, *kwargs):`
		`+ super(ModuleBuiltMessage, self).__init__(args, *kwargs)`
		`+ states_dict = {}`
		`+ 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')`
		`+`
		`+ for state, code in six.iteritems(BUILD_STATES):`
		`+ states_dict[state] = {'state_name': state, 'state': code}`
		`+`
		`+ inner_msg = {`
		`+ 'component_builds': [],`
		`+ 'id': self.build_id,`
		`+ 'modulemd': '',`
		`+ 'name': self.name,`
		`+ 'owner': 'freshmaker',`
		`+ 'scmurl': self.scmurl,`
		`+ 'state': states_dict[self.state]['state'],`
		`+ 'state_name': self.state,`
		`+ 'state_reason': None,`
		`+ 'state_trace': [],`
		`+ 'state_url': u'/module-build-service/1/module-builds/%s' % self.build_id,`
		`+ 'stream': u'master',`
		`+ 'tasks': {},`
		`+ 'time_completed': None,`
		`+ 'time_modified': None,`
		`+ 'time_submitted': time.time(),`
		`+ 'version': time.strftime("%Y%m%d%H%M%S"),`
		`+ }`
		`+ self.inner_msg = inner_msg`
		`+`
		`+`
		`+ class PDCModuleInfoFactory(object):`
		`+ def __init__(self, name, version, release, active=True):`
		`+ self.variant_name = name`
		`+ self.variant_version = version`
		`+ self.variant_release = release`
		`+ self.active = active`
		`+ self.variant_uid = "%s-%s-%s" % (name, version, release)`
		`+ self.variant_id = name`
		`+ self.variant_type = 'module'`
		`+ self.modulemd = ''`
		`+ 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)])`
		`+`
		`+ def produce(self):`
		`+ module = {`
		`+ 'active': self.active,`
		`+ 'variant_type': self.variant_type,`
		`+ 'variant_id': self.variant_id,`
		`+ 'variant_name': self.variant_name,`
		`+ 'variant_version': self.variant_version,`
		`+ 'variant_release': self.variant_release,`
		`+ 'variant_uid': self.variant_uid,`
		`+ 'modulemd': self.modulemd,`
		`+ 'koji_tag': self.koji_tag,`
		`+ 'build_deps': self.build_deps,`
		`+ 'runtime_deps': self.runtime_deps,`
		`+ }`
		`+ return module`
		`+`
		`+`
		`+ class PDCModuleInfo(PDCModuleInfoFactory):`
		`+ def add_build_dep(self, name, stream):`
		`+ self.build_deps.append({'dependency': name, 'stream': stream})`

tests/test_consumer.py

file modified

+26 -7

		`@@ -22,20 +22,19 @@`
		`import mock`
		`import fedmsg.config`

		`- from mock import patch`
		`- from freshmaker.consumer import FreshmakerConsumer`
		`+ import freshmaker`


		`- @patch("freshmaker.consumer.get_global_consumer")`
		`- class TestPoller(unittest.TestCase):`
		`-`
		`+ class ConsumerTest(unittest.TestCase):`
		`def setUp(self):`
		`pass`

		`def tearDown(self):`
		`pass`

		`- def test_consumer_processing_message(self, global_consumer):`
		`+ @mock.patch("freshmaker.handlers.mbs.MBS.handle_module_built")`
		`+ @mock.patch("freshmaker.consumer.get_global_consumer")`
		`+ def test_consumer_processing_message(self, global_consumer, handle_module_built):`
		`"""`
		`Tests that consumer parses the message, forwards the event`
		`to proper handler and is able to get the further work from`
		`@@ -43,7 +42,8 @@`
		`"""`
		`hub = mock.MagicMock()`
		`hub.config = fedmsg.config.load_config()`
		`- consumer = FreshmakerConsumer(hub)`
		`+ hub.config['freshmakerconsumer'] = True`
		`+ consumer = freshmaker.consumer.FreshmakerConsumer(hub)`
		`global_consumer.return_value = consumer`

		`msg = {'body': {`
		`@@ -57,7 +57,26 @@`
		`}`
		`}}`

		`+ handle_module_built.return_value = [freshmaker.events.TestingEvent("ModuleBuilt handled")]`
		`consumer.consume(msg)`

		`event = consumer.incoming.get()`
		`self.assertEqual(event.msg_id, "ModuleBuilt handled")`
		`+`
		`+ @mock.patch("freshmaker.consumer.get_global_consumer")`
		`+ def test_consumer_subscribe_to_specified_topics(self, global_consumer):`
		`+ """`
		`+ Tests consumer will try to subscribe specified topics.`
		`+ """`
		`+ hub = mock.MagicMock()`
		`+ hub.config = fedmsg.config.load_config()`
		`+ consumer = freshmaker.consumer.FreshmakerConsumer(hub)`
		`+ global_consumer.return_value = consumer`
		`+ topics = freshmaker.events.BaseEvent.get_parsed_topics()`
		`+ callback = consumer._consume_json if consumer.jsonify else consumer.consume`
		`+ for topic in topics:`
		`+ self.assertIn(mock.call(topic, callback), hub.subscribe.call_args_list)`
		`+`
		`+`
		`+ if __name__ == '__main__':`
		`+ unittest.main()`

tests/test_mbs_handler.py

file added

+158

		`@@ -0,0 +1,158 @@`
		`+ # Copyright (c) 2017 Red Hat, Inc.`
		`+ #`
		`+ # Permission is hereby granted, free of charge, to any person obtaining a copy`
		`+ # of this software and associated documentation files (the "Software"), to deal`
		`+ # in the Software without restriction, including without limitation the rights`
		`+ # to use, copy, modify, merge, publish, distribute, sublicense, and/or sell`
		`+ # copies of the Software, and to permit persons to whom the Software is`
		`+ # furnished to do so, subject to the following conditions:`
		`+ #`
		`+ # The above copyright notice and this permission notice shall be included in all`
		`+ # copies or substantial portions of the Software.`
		`+ #`
		`+ # THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR`
		`+ # IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,`
		`+ # FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE`
		`+ # AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER`
		`+ # LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,`
		`+ # OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE`
		`+ # SOFTWARE.`
		`+`
		`+ import os`
		`+ import sys`
		`+ import unittest`
		`+ import mock`
		`+`
		`+ sys.path.insert(0, os.path.join(os.path.dirname(__file__), ".."))`
		`+ from tests import helpers`
		`+`
		`+ from freshmaker import events`
		`+ from freshmaker.handlers.mbs import MBS`
		`+ from freshmaker.parsers.mbsmodule import MBSModuleParser`
		`+`
		`+`
		`+ class MBSHandlerTest(unittest.TestCase):`
		`+ def setUp(self):`
		`+ events.BaseEvent.register_parser(MBSModuleParser)`
		`+`
		`+ def _get_event(self, message):`
		`+ event = events.BaseEvent.from_fedmsg(message['body']['topic'], message['body'])`
		`+ return event`
		`+`
		`+ def test_can_handle_module_built_ready_event(self):`
		`+ """`
		`+ Tests MBS handler can handle modult build ready message`
		`+ """`
		`+`
		`+ msg = helpers.ModuleBuiltMessage('testmodule', 'master', state='ready').produce()`
		`+ event = self._get_event(msg)`
		`+`
		`+ handler = MBS()`
		`+ self.assertTrue(handler.can_handle(event))`
		`+`
		`+ def test_can_not_handle_module_built_non_ready_event(self):`
		`+ """`
		`+ Tests MBS handler cannot handle modult build message which is not with`
		`+ 'ready' state.`
		`+ """`
		`+ for s in ['init', 'wait', 'build', 'done', 'failed']:`
		`+ msg = helpers.ModuleBuiltMessage('testmodule', 'master', state=s).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_rebuild_depending_modules_on_module_built_event(self, conf, pdc, utils):`
		`+ """`
		`+ Tests MBS handler can rebuild all modules which depend on the module`
		`+ in module built event.`
		`+ """`
		`+ msg = helpers.ModuleBuiltMessage('testmodule', 'master', state='ready').produce()`
		`+ event = self._get_event(msg)`
		`+`
		`+ handler = MBS()`
		`+`
		`+ mod2_r1_info = helpers.PDCModuleInfo('testmodule2', 'master', '20170412010101')`
		`+ mod2_r1_info.add_build_dep('testmodule', 'master')`
		`+ mod2_r1 = mod2_r1_info.produce()`
		`+`
		`+ mod3_r1_info = helpers.PDCModuleInfo('testmodule3', 'master', '20170412010201')`
		`+ 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):`
		`+`
		`+ if name == 'testmodule2' and version == 'master':`
		`+ return [mod2_r1]`
		`+ elif name == 'testmodule3' and version == 'master':`
		`+ return [mod3_r1]`
		`+ else:`
		`+ return [mod2_r1, mod3_r1]`
		`+`
		`+ pdc.get_modules.side_effect = get_modules`
		`+ conf.git_base_url = "git://pkgs.fedoraproject.org"`
		`+ utils.get_commit_hash.side_effect = [`
		`+ "fae7848fa47a854f25b782aa64441040a6d86544",`
		`+ "43ec03000d249231bc7135b11b810afc96e90efb",`
		`+ ]`
		`+ handler.rebuild_module = mock.Mock()`
		`+ handler.handle_module_built(event)`
		`+`
		`+ self.assertEqual(handler.rebuild_module.call_args_list,`
		`+ [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.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):`
		`+ """`
		`+ 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.`
		`+ """`
		`+ msg = helpers.ModuleBuiltMessage('testmodule', 'master', state='ready').produce()`
		`+ event = self._get_event(msg)`
		`+`
		`+ handler = MBS()`
		`+`
		`+ mod2_r1_info = helpers.PDCModuleInfo('testmodule2', 'master', '20170412010101')`
		`+ mod2_r1_info.add_build_dep('testmodule', 'master')`
		`+ mod2_r1 = mod2_r1_info.produce()`
		`+`
		`+ mod3_r1_info = helpers.PDCModuleInfo('testmodule3', 'master', '20170412010101')`
		`+ mod3_r1_info.add_build_dep('testmodule', 'master')`
		`+ mod3_r1 = mod3_r1_info.produce()`
		`+`
		`+ mod3_r2_info = helpers.PDCModuleInfo('testmodule3', 'master', '20170412010201')`
		`+ 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):`
		`+`
		`+ if name == 'testmodule2' and version == 'master':`
		`+ return [mod2_r1]`
		`+ elif name == 'testmodule3' and version == 'master':`
		`+ return [mod3_r1, mod3_r2]`
		`+ else:`
		`+ return [mod2_r1, mod3_r1]`
		`+`
		`+ # 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`
		`+ conf.git_base_url = "git://pkgs.fedoraproject.org"`
		`+ utils.get_commit_hash.side_effect = [`
		`+ "fae7848fa47a854f25b782aa64441040a6d86544",`
		`+ "43ec03000d249231bc7135b11b810afc96e90efb",`
		`+ ]`
		`+ handler.rebuild_module = mock.Mock()`
		`+ handler.handle_module_built(event)`
		`+`
		`+ self.assertEqual(handler.rebuild_module.call_args_list,`
		`+ [mock.call(u'git://pkgs.fedoraproject.org/modules/testmodule2.git?#fae7848fa47a854f25b782aa64441040a6d86544', u'master')])`
		`+`
		`+`
		`+ if __name__ == '__main__':`
		`+ unittest.main()`

qwan commented 8 years ago

Rebuild depending modules when receives module built message

When there is a module built message received, and state for the module
built is 'ready', query PDC to get depending modules, send module
build request to MBS. For the depending modules, we will filter out any
modules that are not the latest release of (module_name, module_version).

jkaluza commented on line 10 of freshmaker/events.py 8 years ago

It seems you are using only "name", and "stream" from body. Could you store it separately here like:

def __init__(self, ..., name, stream):
   ...
   self.name = name
   self.stream = stream

The reason is that we might support another messaging system later and we would then have to convert that message from another messaging system to fedmsg. We should also not pass real fedmsg messages out of the parser, because the format is not clear from the code - I don't know what is stored in the body.

jkaluza commented on line 51 of freshmaker/handlers/mbs.py 8 years ago

That's just a nitpick, but it would be cool to set the msg to "bumped to rebuild because of %s update" % event.body.get('name').

Edited 8 years ago by jkaluza

jkaluza commented on line 90 of freshmaker/utils.py 8 years ago

I'm not sure if we want to use "fedpkg" here. Just git clone with the generated URL should be enough. In case of "fedpkg", we would have to make it configurable, so it could be executed with "rhpkg" later for example...

jkaluza commented on line 106 of freshmaker/utils.py 8 years ago

Same about "fedpkg" as above - you can just use git push here. You should not need to set --user here I think, because that's already set in the .git directory of cloned git repo.

jkaluza commented on line 104 of tests/test_mbs_handler.py 8 years ago

Can you store this msg and also the one in "test_only_rebuild_latest_depending_modules_on_module_built_event" to "tests/fedmsgs" and use "tests.get_fedmsg" to load them. They seem to be the same and also the modulemd part is quite long. I think it would be better to keep that in separate file out of the test.

jkaluza commented on line 31 of tests/test_mbs_handler.py 8 years ago

Any reason why this is everything is not stored in test_mbs.py together with that single test we have for MBS handler?

qwan commented on line 10 of freshmaker/events.py 8 years ago

Makes sense, I was trying to keep message body as we may want to get some modulemd or something else in handler later, we can get necessary info from message in parser and assign it to event latest.

qwan commented on line 51 of freshmaker/handlers/mbs.py 8 years ago

Good point.

qwan commented on line 106 of freshmaker/utils.py 8 years ago

Will update, and I was thinking about reusing MBS's scm.py, there is something we can share between several projects, it would be good to have a library package to provide some generate utilities. At this moment, we can stay with this.

qwan commented on line 104 of tests/test_mbs_handler.py 8 years ago

I'd like to keep these test messages within the test cases, as the implementation changed, we can find and update the test message easily when there is a test case fails, I'll try to add a test message producer later, so that we can generate such test messages as wish, and won't get a big file that contains a lot of test messages.

qwan commented on line 31 of tests/test_mbs_handler.py 8 years ago

I think we may want to have test_mbs_parser.py later, and actually planning to move the case from test_mbs.py to test_mbs_handler.py.

jkaluza commented on line 31 of tests/test_mbs_handler.py 8 years ago

+1 then :)

jkaluza commented 8 years ago

Can you also please check the tests in jenkins? They seem to fail in that new test code.

2 new commits added

Rebuild depending modules when receives module built message
Fix consumer's topics are not subscribed

8 years ago

qwan commented 8 years ago

Updated per comments, for the test failures, I'm still investigating, the cases pass on my local env with both:

FRESHMAKER_DEVELOPER_ENV=1 python tests/test_mbs_handler.py

and

FRESHMAKER_DEVELOPER_ENV=1 pytest tests/test_mbs_handler.py

But it fails with

tox -r -e py27,py35

Mocking the rebuild_module:

handler.rebuild_module = mock.Mock()

from the test cases don't take effect at all in tox env.

jkaluza commented on line 38 of tests/test_consumer.py 8 years ago

So the problem why tests fail is partly here. You overwrite the MBS.handle_module_built here globally for all the tests, so test_mbs_handler.py test fails then, because handle_module_built is this mocked object instead of the real method. You should not do it like this and instead do this:

diff --git a/tests/test_consumer.py b/tests/test_consumer.py
index 29436bf..beaf67c 100644
--- a/tests/test_consumer.py
+++ b/tests/test_consumer.py
@@ -33,7 +33,8 @@ class ConsumerTest(unittest.TestCase):
         pass

     @mock.patch("freshmaker.consumer.get_global_consumer")
-    def test_consumer_processing_message(self, global_consumer):
+    @mock.patch("freshmaker.handlers.mbs.MBS.handle_module_built")
+    def test_consumer_processing_message(self, handle_module_built, global_consumer):
         """
         Tests that consumer parses the message, forwards the event
         to proper handler and is able to get the further work from
@@ -55,9 +56,7 @@ class ConsumerTest(unittest.TestCase):
             }
         }}

-        from freshmaker.handlers.mbs import MBS
-        MBS.handle_module_built = mock.Mock()
-        MBS.handle_module_built.return_value = [freshmaker.events.TestingEvent("ModuleBuilt handled")]
+        handle_module_built.return_value = [freshmaker.events.TestingEvent("ModuleBuilt handled")]
         consumer.consume(msg)

         event = consumer.incoming.get()

There are more places in test_mbs_handler where you do this :).

2 new commits added

Rebuild depending modules when receives module built message
Fix consumer's topics are not subscribed

8 years ago

qwan commented 8 years ago

So the problem why tests fail is partly here. You overwrite the MBS.handle_module_built here globally

Good catch, thanks. I've fixed the test failures and pushed the updated patch.

2 new commits added

Rebuild depending modules when receives module built message
Fix consumer's topics are not subscribed

8 years ago

cqi commented on line 87 of freshmaker/utils.py 8 years ago

This docstring needs update as well.

cqi commented on line 97 of freshmaker/utils.py 8 years ago

Just a thought FYI, how about make this configurable?

cqi commented on line 39 of tests/test_mbs_handler.py 8 years ago

If no need of tearDown, why not delete it?

cqi commented on line 136 of freshmaker/utils.py 8 years ago

Why not merge _run_command and _get_command_output? These two methods do many same things except return value.

cqi commented on line 39 of freshmaker/handlers/mbs.py 8 years ago

For Python 2.7+, this set can be simplified

set((m.get('variant_name'), m.get('variant_version')) for m in depending_modules)

I'm also curious why need to remove duplicate pair of (name, version)?

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

Should this be :return: pdc_client.PDCClient instance?

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

I guess this if-else is required for detecting different versions of pdc_client. If yes, it would probably good to use pdc_client version rather than inspecting __init__. Because, that could be straightforward and easy to understand the purpose.

If my understand is incorrect, please correct me :)

cqi commented on line 39 of freshmaker/utils.py 8 years ago

What does it mean by these dots? :)

jkaluza commented on line 37 of freshmaker/pdc.py 8 years ago

It is required. pdc_client 1.2.0 is New API here and pdc_client 1.1.0 is old API.

qwan commented on line 39 of freshmaker/handlers/mbs.py 8 years ago

We need to get the unique result of (name, version) and then query PDC to the latest (name, version) to filter out any (name, version) in depending_modules that is not the latest one.