PR#27: Add db entries when module/container build is submitted and track build state - freshmaker

freshmaker

#27 Add db entries when module/container build is submitted and track build state

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

qwan/freshmaker add-db-entries into master

Update build state when receives koji task state change message

Qixiang Wan • 8 years ago

28e0f6b

Update build state when receives module build state change message

Qixiang Wan • 8 years ago

135aaeb

Add db entries when module/container build is submitted

Qixiang Wan • 8 years ago

9d3845a

conf/config.py

file modified

		`@@ -45,6 +45,7 @@`
		`"freshmaker.handlers.mbs:MBS", # Module Build Service`
		`"freshmaker.handlers.image_builder:DockerImageRebuildHandler",`
		`"freshmaker.handlers.image_builder:DockerImageRebuildHandlerForBodhi",`
		`+ "freshmaker.handlers.buildsys:BuildsysHandler",`
		`]`

		`# Base URL of git repository with source artifacts.`
		`@@ -76,6 +77,7 @@`

		`SSL_ENABLED = False`

		`+`
		`class DevConfiguration(BaseConfiguration):`
		`DEBUG = True`
		`LOG_BACKEND = 'console'`

freshmaker/consumer.py

file modified

		`@@ -31,6 +31,7 @@`
		`import freshmaker.parsers.mbsmodule`
		`import freshmaker.parsers.gitreceive`
		`import freshmaker.parsers.bodhiupdate`
		`+ import freshmaker.parsers.buildsys`

		`from freshmaker import log, conf, messaging, events`

		`@@ -65,6 +66,7 @@`
		`events.BaseEvent.register_parser(freshmaker.parsers.mbsmodule.MBSModuleParser)`
		`events.BaseEvent.register_parser(freshmaker.parsers.gitreceive.GitReceiveParser)`
		`events.BaseEvent.register_parser(freshmaker.parsers.bodhiupdate.UpdateCompleteStableParser)`
		`+ events.BaseEvent.register_parser(freshmaker.parsers.buildsys.BuildsysParser)`
		`log.debug("Parser classes: %r", events.BaseEvent._parsers)`

		`self.topic = events.BaseEvent.get_parsed_topics()`

freshmaker/events.py

file modified

+12 -1

		`@@ -143,9 +143,10 @@`
		`:param scm_url: SCM URL of a updated module.`
		`:param branch: Branch of updated module.`
		`"""`
		`- def __init__(self, msg_id, scm_url, branch):`
		`+ def __init__(self, msg_id, scm_url, name, branch):`
		`super(ModuleMetadataUpdated, self).__init__(msg_id)`
		`self.scm_url = scm_url`
		`+ self.name = name`
		`self.branch = branch`


		`@@ -210,3 +211,13 @@`
		`self.update_id = update_id`
		`self.builds = builds`
		`self.release = release`
		`+`
		`+`
		`+ class KojiTaskStateChanged(BaseEvent):`
		`+ """`
		`+ Provides an event object for "the state of task changed in koji"`
		`+ """`
		`+ def __init__(self, msg_id, task_id, task_state):`
		`+ super(KojiTaskStateChanged, self).__init__(msg_id)`
		`+ self.task_id = task_id`
		`+ self.task_state = task_state`

freshmaker/handlers/__init__.py

file modified

+15 -1

		`@@ -24,7 +24,7 @@`
		`import abc`
		`import fedmsg.utils`

		`- from freshmaker import conf`
		`+ from freshmaker import conf, db, models`


		`def load_handlers():`
		`@@ -59,3 +59,17 @@`
		`generate internal events for other handlers in Freshmaker.`
		`"""`
		`raise NotImplementedError()`
		`+`
		`+ def record_build(self, event, name, type, build_id, dep_of=None):`
		`+ """`
		`+ Record build in db.`
		`+`
		`+ :param event: instance of an event.`
		`+ :param name: name of the artifact.`
		`+ :param type: type of the artifact, can be 'rpm', 'image' or module.`
		`+ :param build_id: id of the build in build system.`
		`+ :param def_of: the artifact which this one depends on.`
		`+ """`
		`+ ev = models.Event.get_or_create(db.session, event.msg_id)`
		`+ models.ArtifactBuild.create(db.session, ev, name, type, build_id, dep_of)`
		`+ db.session.commit()`

freshmaker/handlers/buildsys.py

file added

+62

		`@@ -0,0 +1,62 @@`
		`+ # -- 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.`
		`+`
		`+ from freshmaker import log, db, models`
		`+ from freshmaker.handlers import BaseHandler`
		`+ from freshmaker.events import KojiTaskStateChanged`
		`+`
		`+`
		`+ class BuildsysHandler(BaseHandler):`
		`+ name = "BuildsysHandler"`
		`+`
		`+ def can_handle(self, event):`
		`+ if isinstance(event, KojiTaskStateChanged):`
		`+ return True`
		`+`
		`+ return False`
		`+`
		`+ def handle_koji_task_state_changed(self, event):`
		`+ task_id = event.task_id`
		`+ task_state = event.task_state`
		`+`
		`+ # check whether the task exists in db as image build`
		`+ builds = db.session.query(models.ArtifactBuild).filter_by(build_id=task_id,`
		`+ type=models.ARTIFACT_TYPES['image']).all()`
		`+ if len(builds) > 1:`
		`+ raise RuntimeError("Found duplicate image build '%s' in db" % task_id)`
		`+ if len(builds) == 1:`
		`+ build = builds[0]`
		`+ if task_state in ['CLOSED', 'FAILED']:`
cqi commented 8 years ago `task_state` is integer rather than str?
qwan commented 8 years ago By the document, koji send the message with states in string rather than integer, I haven't check with the real message by now, datagrepper is unable to show messages under topic `task.state.change` (https://pagure.io/fedora-infrastructure/issue/6064), I'll capture a real message to confirm this. of course we can translate it to integer in Freshmaker, but I don't think it worth to do that since we'll need to import koji.TASK_STATE to do the translation.
cqi commented 8 years ago I saw an example in https://fedora-fedmsg.readthedocs.io/en/latest/topics.html#buildsys-build-state-change, both `new` and `old` are integer. Edited 8 years ago by cqi
qwan commented 8 years ago We are listening on `buildsys.task.state.change` rather than `buildsys.build.state.change`: https://fedora-fedmsg.readthedocs.io/en/latest/topics.html#buildsys-task-state-change
cqi commented 8 years ago Sorry. my original comment is invalid.
		`+ log.info("Image build '%s' state changed in koji, updating it in db.", task_id)`
		`+ if task_state == 'CLOSED':`
		`+ build.state = models.BUILD_STATES['done']`
		`+ db.session.commit()`
		`+ if task_state == 'FAILED':`
		`+ build.state = models.BUILD_STATES['failed']`
		`+ db.session.commit()`
		`+`
		`+ return []`
		`+`
		`+ def handle(self, event):`
		`+ if isinstance(event, KojiTaskStateChanged):`
		`+ return self.handle_koji_task_state_changed(event)`
		`+`
		`+ return []`

freshmaker/handlers/image_builder.py

file modified

+9 -5

		`@@ -49,10 +49,13 @@`
		`log.info('Start to rebuild docker image %s', event.repo)`

		`try:`
		`- self.build_image(repo_url=event.repo_url,`
		`- rev=event.rev,`
		`- branch=event.branch,`
		`- namespace=event.namespace)`
		`+ task_id = self.build_image(repo_url=event.repo_url,`
		`+ rev=event.rev,`
		`+ branch=event.branch,`
		`+ namespace=event.namespace)`
		`+`
		`+ self.record_build(event, event.repo, 'image', task_id)`
		`+`
		`except koji.krbV.Krb5Error as e:`
		`log.exception('Failed to login Koji via Kerberos using GSSAPI. %s', e.args[1])`
		`except:`
		`@@ -98,7 +101,8 @@`

		`for container in containers:`
		`try:`
		`- self.handle_image_build(container)`
		`+ task_id = self.handle_image_build(container)`
		`+ self.record_build(event, container['name'], 'image', task_id)`
		`except:`
		`log.exception('Error when rebuild %s', container)`

freshmaker/handlers/mbs.py

file modified

+54 -22

		`@@ -23,20 +23,28 @@`

		`import requests`

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


		`+ MBS_BUILD_STATES = {`
		`+ "init": 0,`
		`+ "wait": 1,`
		`+ "build": 2,`
		`+ "done": 3,`
		`+ "failed": 4,`
		`+ "ready": 5,`
		`+ }`
		`+`
		`+`
		`class MBS(BaseHandler):`
		`name = "MBS"`

		`def can_handle(self, event):`
		`- # Handle only "ready" state of ModuleBuilt.`
		`# TODO: Handle only when something depends on`
		`# this module.`
		`- if (isinstance(event, ModuleBuilt) and`
		`- event.module_build_state == 5):`
		`+ if isinstance(event, ModuleBuilt):`
		`return True`

		`if isinstance(event, ModuleMetadataUpdated):`
		`@@ -82,11 +90,13 @@`

		`if commitid is not None:`
		`scm_url = conf.git_base_url + '/modules/%s.git?#%s' % (name, commitid)`
		`- self.rebuild_module(scm_url, branch)`
		`+ return 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)`
		`+ build_id = self.rebuild_module(event.scm_url, event.branch)`
		`+ if build_id is not None:`
		`+ self.record_build(event, event.name, 'module', build_id)`

		`return []`

		`@@ -98,21 +108,41 @@`
		`"""`
		`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)`
		`- 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`
		`- self.bump_and_rebuild_module(mod['variant_name'],`
		`- mod['variant_version'],`
		`- commit_msg=commit_msg)`
		`- return []`
		`+ build_id = event.module_build_id`
		`+ build_state = event.module_build_state`
		`+`
		`+ # update build state if the build is submitted by Freshmaker`
		`+ builds = db.session.query(models.ArtifactBuild).filter_by(build_id=build_id,`
		`+ type=models.ARTIFACT_TYPES['module']).all()`
		`+ if len(builds) > 1:`
		`+ raise RuntimeError("Found duplicate module build '%s' in db" % build_id)`
		`+ if len(builds) == 1:`
		`+ build = builds[0]`
		`+ if build_state in [MBS_BUILD_STATES['ready'], MBS_BUILD_STATES['failed']]:`
		`+ log.info("Module build '%s' state changed in MBS, updating it in db.", build_id)`
		`+ if build_state == MBS_BUILD_STATES['ready']:`
		`+ build.state = models.BUILD_STATES['done']`
		`+ if build_state == MBS_BUILD_STATES['failed']:`
		`+ build.state = models.BUILD_STATES['failed']`
		`+ db.session.commit()`
		`+`
		`+ # Rebuild depending modules when state of ModuleBuilt is 'ready'`
		`+ if build_state == MBS_BUILD_STATES['ready']:`
		`+ log.info("Triggering rebuild of modules depending on %s:%s "`
		`+ "in MBS", module_name, module_stream)`
		`+`
		`+ pdc_session = pdc.get_client_session(conf)`
		`+ modules = pdc.get_latest_modules(pdc_session,`
		`+ build_dep_name=module_name,`
		`+ build_dep_stream=module_stream,`
		`+ active='true')`
		`+ for mod in modules:`
		`+ name = mod['variant_name']`
		`+ version = mod['variant_version']`
		`+ commit_msg = "Bump to rebuild because of %s update" % module_name`
		`+ build_id = self.bump_and_rebuild_module(name, version, commit_msg=commit_msg)`
		`+ if build_id is not None:`
		`+ self.record_build(event, name, 'module', build_id)`

		`def handle_rpm_spec_updated(self, event):`
		`"""`
		`@@ -135,7 +165,9 @@`
		`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)`
		`+ build_id = self.bump_and_rebuild_module(module_name, module_branch, commit_msg=commit_msg)`
		`+ if build_id is not None:`
		`+ self.record_build(event, module_name, 'module', build_id)`

		`return []`

freshmaker/models.py

file modified

+15 -8

		`@@ -24,15 +24,10 @@`
		`""" SQLAlchemy Database models for the Flask app`
		`"""`

		`- import contextlib`
		`-`
		`from datetime import datetime`
		`- from sqlalchemy import engine_from_config`
		`- from sqlalchemy.orm import (validates, scoped_session, sessionmaker,`
		`- relationship)`
		`- from sqlalchemy.ext.declarative import declarative_base`
		`+ from sqlalchemy.orm import (validates, relationship)`

		`- from freshmaker import db, log`
		`+ from freshmaker import db`

		`# BUILD_STATES for the builds submitted by Freshmaker`
		`BUILD_STATES = {`
		`@@ -56,9 +51,11 @@`

		`INVERSE_ARTIFACT_TYPES = {v: k for k, v in ARTIFACT_TYPES.items()}`

		`+`
		`class FreshmakerBase(db.Model):`
		`__abstract__ = True`

		`+`
		`class Event(FreshmakerBase):`
		`__tablename__ = "events"`
		`id = db.Column(db.Integer, primary_key=True)`
		`@@ -75,6 +72,17 @@`
		`session.add(event)`
		`return event`

		`+ @classmethod`
		`+ def get_or_create(cls, session, message_id):`
		`+ instance = session.query(cls).filter_by(message_id=message_id).first()`
		`+ if instance:`
		`+ return instance`
		`+ return cls.create(session, message_id)`
		`+`
		`+ def __repr__(self):`
		`+ return "<Event %s>" % (self.message_id)`
		`+`
		`+`
		`class ArtifactBuild(FreshmakerBase):`
		`__tablename__ = "artifact_builds"`
		`id = db.Column(db.Integer, primary_key=True)`
		`@@ -131,4 +139,3 @@`
		`return "<ArtifactBuild %s, type %s, state %s, event %s>" % (`
		`self.name, INVERSE_ARTIFACT_TYPES[self.type],`
		`INVERSE_BUILD_STATES[self.state], self.event.message_id)`
		`-`

freshmaker/parsers/buildsys.py

file added

+53

		`@@ -0,0 +1,53 @@`
		`+ # -- 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.`
		`+`
		`+ from freshmaker import log`
		`+ from freshmaker.parsers import BaseParser`
		`+ from freshmaker.events import KojiTaskStateChanged`
		`+`
		`+`
		`+ class BuildsysParser(BaseParser):`
		`+ """`
		`+ Parser parsing task state change message from buildsys (koji), generating`
		`+ KojiTaskStateChanged event.`
		`+ """`
		`+ name = "BuildsysParser"`
		`+ topic_suffixes = ["buildsys.task.state.change"]`
		`+`
		`+ def can_parse(self, topic, msg):`
		`+ log.debug(topic)`
		`+ if not any([topic.endswith(s) for s in self.topic_suffixes]):`
		`+ return False`
		`+ return True`
		`+`
		`+ def parse(self, topic, msg):`
		`+ msg_id = msg.get('msg_id')`
		`+ msg_inner_msg = msg.get('msg')`
		`+`
		`+ # If there isn't a msg dict in msg then this message can be skipped`
		`+ if not msg_inner_msg:`
		`+ log.debug(('Skipping message without any content with the '`
		`+ 'topic "{0}"').format(topic))`
		`+ return None`
		`+`
		`+ return KojiTaskStateChanged(msg_id,`
		`+ msg_inner_msg.get('id'),`
		`+ msg_inner_msg.get('new'))`

freshmaker/parsers/gitreceive.py

file modified

+1 -1

		`@@ -67,7 +67,7 @@`
		`scm_url = "%s/%s/%s.git?#%s" % (conf.git_base_url, namespace, repo, rev)`
		`log.debug("Parsed ModuleMetadataUpdated fedmsg, scm_url=%s, "`
		`"branch=%s", scm_url, branch)`
		`- return ModuleMetadataUpdated(msg_id, scm_url, branch)`
		`+ return ModuleMetadataUpdated(msg_id, scm_url, repo, branch)`

		`elif namespace == 'container':`
		`changed_files = msg['msg']['commit']['stats']['files']`

tests/handlers/test_image_builder.py

file modified

+43 -7

		`@@ -20,7 +20,6 @@`
		`#`
		`# Written by Chenxiong Qi <cqi@redhat.com>`

		`- import shutil`
		`import tempfile`
		`import unittest`

		`@@ -32,13 +31,23 @@`
		`from mock import MagicMock`
		`from mock import call`

		`- from freshmaker import conf`
		`+ from freshmaker import conf, db, models`
		`from freshmaker.consumer import FreshmakerConsumer`
		`from freshmaker.handlers.image_builder import DockerImageRebuildHandlerForBodhi`
		`from tests import get_fedmsg`


		`class BaseTestCase(unittest.TestCase):`
		`+ def setUp(self):`
		`+ db.session.remove()`
		`+ db.drop_all()`
		`+ db.create_all()`
		`+ db.session.commit()`
		`+`
		`+ def tearDown(self):`
		`+ db.session.remove()`
		`+ db.drop_all()`
		`+ db.session.commit()`

		`def create_consumer(self):`
		`hub = MagicMock()`
		`@@ -65,9 +74,11 @@`
		`'weburl': 'https://localhost/koji',`
		`}`

		`- self.consume_fedmsg(get_fedmsg('git_receive_dockerfile_changed'))`
		`-`
		`mock_session = ClientSession.return_value`
		`+ mock_session.buildContainer.return_value = 123`
		`+ msg = get_fedmsg('git_receive_dockerfile_changed')`
		`+ self.consume_fedmsg(msg)`
		`+`
		`mock_session.krb_login.assert_called_once_with(proxyuser=None)`
		`mock_session.buildContainer.assert_called_once_with(`
		`'git://pkgs.fedoraproject.org/container/testimage.git?#e1f39d43471fc37ec82616f76a119da4eddec787',`
		`@@ -75,6 +86,15 @@`
		`{'scratch': True, 'git_branch': 'master'})`
		`mock_session.logout.assert_called_once()`

		`+ events = models.Event.query.all()`
		`+ self.assertEquals(len(events), 1)`
		`+ self.assertEquals(events[0].message_id, msg['body']['msg_id'])`
		`+ builds = models.ArtifactBuild.query.all()`
		`+ self.assertEquals(len(builds), 1)`
		`+ self.assertEquals(builds[0].name, 'testimage')`
		`+ self.assertEquals(builds[0].type, models.ARTIFACT_TYPES['image'])`
		`+ self.assertEquals(builds[0].build_id, 123)`
		`+`
		`@patch('freshmaker.handlers.image_builder.DockerImageRebuildHandler.build_image')`
		`def test_not_rebuild_if_Dockerfile_not_changed(self, build_image):`
		`self.consume_fedmsg(get_fedmsg('git_receive_dockerfile_not_changed'))`
		`@@ -157,6 +177,7 @@`
		`}`
		`}`

		`+`
		`def mock_get_release_component(pdc_session, id):`
		`return mock_release_components[id]`

		`@@ -165,6 +186,7 @@`
		`class TestRebuildWhenBodhiUpdateStable(BaseTestCase):`

		`def setUp(self):`
		`+ super(TestRebuildWhenBodhiUpdateStable, self).setUp()`
		`# Use to return a temporary directory from temp_dir method. So, no need`
		`# to delete this directory, since temp_dir ensures to do that.`
		`self.working_dir = tempfile.mkdtemp(prefix='test-image-rebuild-')`
		`@@ -199,7 +221,11 @@`

		`get_containers_including_rpms.return_value = mock_found_containers`

		`- self.consume_fedmsg(get_fedmsg('bodhi_update_stable'))`
		`+ session = ClientSession.return_value`
		`+ session.buildContainer.side_effect = [123, 456]`
		`+`
		`+ msg = get_fedmsg('bodhi_update_stable')`
		`+ self.consume_fedmsg(msg)`

		`self.assertEqual(2, _run_command.call_count)`

		`@@ -212,8 +238,6 @@`
		`rundir=self.working_dir)`
		`])`

		`- session = ClientSession.return_value`
		`-`
		`self.assertEqual(2, session.krb_login.call_count)`

		`buildContainer = session.buildContainer`
		`@@ -232,6 +256,18 @@`
		`],`
		`any_order=True)`

		`+ events = models.Event.query.all()`
		`+ self.assertEquals(len(events), 1)`
		`+ self.assertEquals(events[0].message_id, msg['body']['msg_id'])`
		`+ builds = models.ArtifactBuild.query.all()`
		`+ self.assertEquals(len(builds), 2)`
		`+ self.assertEquals(builds[0].name, 'testimage1')`
		`+ self.assertEquals(builds[0].type, models.ARTIFACT_TYPES['image'])`
		`+ self.assertEquals(builds[0].build_id, 123)`
		`+ self.assertEquals(builds[1].name, 'testimage2')`
		`+ self.assertEquals(builds[1].type, models.ARTIFACT_TYPES['image'])`
		`+ self.assertEquals(builds[1].build_id, 456)`
		`+`

		`class TestContainersIncludingRPMs(unittest.TestCase):`

tests/helpers.py

file modified

+34 -2

		`@@ -23,6 +23,9 @@`
		`import string`
		`import time`
		`import uuid`
		`+ import unittest`
		`+`
		`+ from freshmaker import events`


		`BUILD_STATES = {`
		`@@ -35,13 +38,19 @@`
		`}`


		`+ class FreshmakerTestCase(unittest.TestCase):`
		`+ def get_event_from_msg(self, message):`
		`+ event = events.BaseEvent.from_fedmsg(message['body']['topic'], message['body'])`
		`+ return event`
		`+`
		`+`
		`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.source_name = 'unittest'`
		`+ self.source_version = '0.1.1'`
		`self.timestamp = time.time()`
		`self.topic = ''`
		`self.username = 'freshmaker'`
		`@@ -152,6 +161,29 @@`
		`self.stats['total']['lines'] += self.stats['files'][filename]['lines']`


		`+ class BuildsysTaskStateChangeMessage(FedMsgFactory):`
		`+ def __init__(self, task_id, old_state, new_state, args, *kwargs):`
		`+ super(BuildsysTaskStateChangeMessage, self).__init__(args, *kwargs)`
		`+ self.topic = 'org.fedoraproject.prod.buildsys.task.state.change'`
		`+ self.attribute = 'state'`
		`+ self.task_id = task_id`
		`+ self.old_state = old_state`
		`+ self.new_state = new_state`
		`+ self.owner = 'freshmaker'`
		`+ self.method = 'build'`
		`+`
		`+ @property`
		`+ def inner_msg(self):`
		`+ return {`
		`+ 'attribute': self.attribute,`
		`+ 'id': self.task_id,`
		`+ 'method': self.method,`
		`+ 'new': self.new_state,`
		`+ 'old': self.old_state,`
		`+ 'owner': self.owner,`
		`+ }`
		`+`
		`+`
		`class PDCModuleInfoFactory(object):`
		`def __init__(self, name, version, release, active=True):`
		`self.variant_name = name`

tests/test_buildsys_handler.py

file added

+91

		`@@ -0,0 +1,91 @@`
		`+ # 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`
		`+`
		`+ sys.path.insert(0, os.path.join(os.path.dirname(__file__), ".."))`
		`+ from tests import helpers`
		`+`
		`+ from freshmaker import events, db, models`
		`+ from freshmaker.handlers.buildsys import BuildsysHandler`
		`+ from freshmaker.parsers.buildsys import BuildsysParser`
		`+`
		`+`
		`+ class BuildsysHandlerTest(helpers.FreshmakerTestCase):`
		`+ def setUp(self):`
		`+ db.session.remove()`
		`+ db.drop_all()`
		`+ db.create_all()`
		`+ db.session.commit()`
		`+`
		`+ events.BaseEvent.register_parser(BuildsysParser)`
		`+`
		`+ def tearDown(self):`
		`+ db.session.remove()`
		`+ db.drop_all()`
		`+ db.session.commit()`
		`+`
		`+ def test_can_handle_koji_task_state_changed_event(self):`
		`+ """`
		`+ Tests buildsys handler can handle koji task state changed message`
		`+ """`
		`+ m = helpers.BuildsysTaskStateChangeMessage(123, 'OPEN', 'FAILED')`
		`+ msg = m.produce()`
		`+ event = self.get_event_from_msg(msg)`
		`+ handler = BuildsysHandler()`
		`+ self.assertTrue(handler.can_handle(event))`
		`+`
		`+ def test_update_build_state_on_koji_task_state_changed_event(self):`
		`+ """`
		`+ Tests build state will be updated when receives koji task state changed message`
		`+ """`
		`+ task_id = 123`
		`+ ev = models.Event.create(db.session, 'test_msg_id')`
		`+ build = models.ArtifactBuild.create(db.session,`
		`+ ev,`
		`+ 'testimage',`
		`+ models.ARTIFACT_TYPES['image'],`
		`+ task_id)`
		`+ db.session.add(ev)`
		`+ db.session.add(build)`
		`+ db.session.commit()`
		`+`
		`+ m = helpers.BuildsysTaskStateChangeMessage(task_id, 'OPEN', 'FAILED')`
		`+ msg = m.produce()`
		`+ event = self.get_event_from_msg(msg)`
		`+`
		`+ handler = BuildsysHandler()`
		`+ handler.handle(event)`
		`+ build = models.ArtifactBuild.query.all()[0]`
		`+ self.assertEqual(build.state, models.BUILD_STATES['failed'])`
		`+`
		`+ m = helpers.BuildsysTaskStateChangeMessage(task_id, 'OPEN', 'CLOSED')`
		`+ msg = m.produce()`
		`+ event = self.get_event_from_msg(msg)`
		`+`
		`+ handler = BuildsysHandler()`
		`+ handler.handle(event)`
		`+ build = models.ArtifactBuild.query.all()[0]`
		`+ self.assertEqual(build.state, models.BUILD_STATES['done'])`
		`+`
		`+ if __name__ == '__main__':`
		`+ unittest.main()`

tests/test_mbs_handler.py

file modified

+117 -28

		`@@ -26,43 +26,37 @@`
		`sys.path.insert(0, os.path.join(os.path.dirname(__file__), ".."))`
		`from tests import helpers`

		`- from freshmaker import events`
		`+ from freshmaker import events, db, models`
		`from freshmaker.handlers.mbs import MBS`
		`from freshmaker.parsers.mbsmodule import MBSModuleParser`
		`from freshmaker.parsers.gitreceive import GitReceiveParser`


		`- class MBSHandlerTest(unittest.TestCase):`
		`+ class MBSHandlerTest(helpers.FreshmakerTestCase):`
		`def setUp(self):`
		`+ db.session.remove()`
		`+ db.drop_all()`
		`+ db.create_all()`
		`+ db.session.commit()`
		`+`
		`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'])`
		`- return event`
		`+ def tearDown(self):`
		`+ db.session.remove()`
		`+ db.drop_all()`
		`+ db.session.commit()`

		`- def test_can_handle_module_built_ready_event(self):`
		`+ def test_can_handle_module_built_event(self):`
		`"""`
		`- Tests MBS handler can handle modult build ready message`
		`+ Tests MBS handler can handle module built 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)`
		`+ for state in ['init', 'wait', 'build', 'done', 'failed', 'ready']:`
		`+ msg = helpers.ModuleBuiltMessage('testmodule', 'master', state=state).produce()`
		`+ event = self.get_event_from_msg(msg)`

		`handler = MBS()`
		`- self.assertFalse(handler.can_handle(event))`
		`+ self.assertTrue(handler.can_handle(event))`

		`@mock.patch('freshmaker.pdc.get_modules')`
		`@mock.patch('freshmaker.handlers.mbs.utils')`
		`@@ -73,7 +67,7 @@`
		`in module built event.`
		`"""`
		`msg = helpers.ModuleBuiltMessage('testmodule', 'master', state='ready').produce()`
		`- event = self._get_event(msg)`
		`+ event = self.get_event_from_msg(msg)`

		`handler = MBS()`

		`@@ -103,12 +97,25 @@`
		`"43ec03000d249231bc7135b11b810afc96e90efb",`
		`]`
		`handler.rebuild_module = mock.Mock()`
		`+ handler.rebuild_module.side_effect = [123, 456]`
		`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')])`

		`+ event_list = models.Event.query.all()`
		`+ self.assertEquals(len(event_list), 1)`
		`+ self.assertEquals(event_list[0].message_id, event.msg_id)`
		`+ builds = models.ArtifactBuild.query.all()`
		`+ self.assertEquals(len(builds), 2)`
		`+ self.assertEquals(builds[0].name, mod2_r1['variant_name'])`
		`+ self.assertEquals(builds[0].type, models.ARTIFACT_TYPES['module'])`
		`+ self.assertEquals(builds[0].build_id, 123)`
		`+ self.assertEquals(builds[1].name, mod3_r1['variant_name'])`
		`+ self.assertEquals(builds[1].build_id, 456)`
		`+ self.assertEquals(builds[1].type, models.ARTIFACT_TYPES['module'])`
		`+`
		`@mock.patch('freshmaker.pdc.get_modules')`
		`@mock.patch('freshmaker.handlers.mbs.utils')`
		`@mock.patch('freshmaker.handlers.mbs.conf')`
		`@@ -118,7 +125,7 @@`
		`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)`
		`+ event = self.get_event_from_msg(msg)`

		`handler = MBS()`

		`@@ -154,11 +161,21 @@`
		`"43ec03000d249231bc7135b11b810afc96e90efb",`
		`]`
		`handler.rebuild_module = mock.Mock()`
		`+ handler.rebuild_module.return_value = 123`
		`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')])`

		`+ event_list = models.Event.query.all()`
		`+ self.assertEquals(len(event_list), 1)`
		`+ self.assertEquals(event_list[0].message_id, event.msg_id)`
		`+ builds = models.ArtifactBuild.query.all()`
		`+ self.assertEquals(len(builds), 1)`
		`+ self.assertEquals(builds[0].name, mod2_r1['variant_name'])`
		`+ self.assertEquals(builds[0].type, models.ARTIFACT_TYPES['module'])`
		`+ self.assertEquals(builds[0].build_id, 123)`
		`+`
		`def test_can_handle_rpm_spec_updated_event(self):`
		`"""`
		`Tests MBS handler can handle rpm spec updated event`
		`@@ -167,7 +184,7 @@`
		`m.add_changed_file('bash.spec', 1, 1)`
		`msg = m.produce()`

		`- event = self._get_event(msg)`
		`+ event = self.get_event_from_msg(msg)`

		`handler = MBS()`
		`self.assertTrue(handler.can_handle(event))`
		`@@ -182,7 +199,7 @@`
		`m.add_changed_file('test.c', 1, 1)`
		`msg = m.produce()`

		`- event = self._get_event(msg)`
		`+ event = self.get_event_from_msg(msg)`

		`handler = MBS()`
		`self.assertFalse(handler.can_handle(event))`
		`@@ -201,7 +218,7 @@`
		`m.add_changed_file('bash.spec', 1, 1)`
		`msg = m.produce()`

		`- event = self._get_event(msg)`
		`+ event = self.get_event_from_msg(msg)`

		`mod_info = helpers.PDCModuleInfo('testmodule', 'master', '20170412010101')`
		`mod_info.add_rpm("bash-1.2.3-4.f26.rpm")`
		`@@ -213,10 +230,82 @@`

		`handler = MBS()`
		`handler.rebuild_module = mock.Mock()`
		`+ handler.rebuild_module.return_value = 123`
		`handler.handle(event)`
		`self.assertEqual(handler.rebuild_module.call_args_list,`
		`[mock.call('git://pkgs.fedoraproject.org/modules/testmodule.git?#%s' % commitid, 'master')])`

		`+ event_list = models.Event.query.all()`
		`+ self.assertEquals(len(event_list), 1)`
		`+ self.assertEquals(event_list[0].message_id, event.msg_id)`
		`+ builds = models.ArtifactBuild.query.all()`
		`+ self.assertEquals(len(builds), 1)`
		`+ self.assertEquals(builds[0].name, 'testmodule')`
		`+ self.assertEquals(builds[0].type, models.ARTIFACT_TYPES['module'])`
		`+ self.assertEquals(builds[0].build_id, 123)`
		`+`
		`+ @mock.patch('freshmaker.handlers.mbs.utils')`
		`+ @mock.patch('freshmaker.handlers.mbs.pdc')`
		`+ @mock.patch('freshmaker.handlers.mbs.conf')`
		`+ def test_update_build_state_in_db(self, conf, pdc, utils):`
		`+ """`
		`+ Test build state in db will be updated when receives module build`
		`+ state change message.`
		`+ """`
		`+`
		`+ # trigger a build on rpm spec updated event first`
		`+ 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_from_msg(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.rebuild_module.return_value = 123`
		`+ handler.handle(event)`
		`+ self.assertEqual(handler.rebuild_module.call_args_list,`
		`+ [mock.call('git://pkgs.fedoraproject.org/modules/testmodule.git?#%s' % commitid, 'master')])`
		`+`
		`+ event_list = models.Event.query.all()`
		`+ self.assertEquals(len(event_list), 1)`
		`+ self.assertEquals(event_list[0].message_id, event.msg_id)`
		`+ builds = models.ArtifactBuild.query.all()`
		`+ self.assertEquals(len(builds), 1)`
		`+ self.assertEquals(builds[0].name, 'testmodule')`
		`+ self.assertEquals(builds[0].type, models.ARTIFACT_TYPES['module'])`
		`+ self.assertEquals(builds[0].build_id, 123)`
		`+ self.assertEquals(builds[0].state, models.BUILD_STATES['build'])`
		`+`
		`+ # update build state when receive module built messages`
		`+ # build is failed`
		`+ msg = helpers.ModuleBuiltMessage('testmodule', 'master', state='failed', build_id=123).produce()`
		`+ event = self.get_event_from_msg(msg)`
		`+ handler.handle(event)`
		`+ builds = models.ArtifactBuild.query.all()`
		`+ self.assertEquals(len(builds), 1)`
		`+ # build state updated to 'failed'`
		`+ self.assertEquals(builds[0].state, models.BUILD_STATES['failed'])`
		`+`
		`+ # build is ready`
		`+ pdc.get_latest_modules.return_value = []`
		`+ msg = helpers.ModuleBuiltMessage('testmodule', 'master', state='ready', build_id=123).produce()`
		`+ event = self.get_event_from_msg(msg)`
		`+ handler.handle(event)`
		`+ builds = models.ArtifactBuild.query.all()`
		`+ self.assertEquals(len(builds), 1)`
		`+ # build state updated to 'done'`
		`+ self.assertEquals(builds[0].state, models.BUILD_STATES['done'])`

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

qwan commented 8 years ago

Add db entries for event and build when module/container build is submitted
Update build state in db when receives module build state change message
Update build state in db when receives koji task state change message

cqi commented on line 19 of freshmaker/models.py 8 years ago

Both this and above session.commit would prevent from creating multiple Events or ArtifactBuilds in one transaction.

cqi commented on line 77 of tests/handlers/test_image_builder.py 8 years ago

Why not just make call self.consume_git_receive_msg(get_fedmsg("git_receive_dockerfile_changed"))?

cqi commented on line 79 of tests/handlers/test_image_builder.py 8 years ago

My preference is to use this format to write an assertion

self.assert*(expected_value, test_value)

Whatever the way to use, I think we need to keep it being used in all test code in freshmaker, or probably other projects we are working on. What's your opinions guys?

qwan commented on line 77 of tests/handlers/test_image_builder.py 8 years ago

we'll use msg['body']['msg_id'] later to avoid call get_fedmsg twice.

qwan commented on line 19 of freshmaker/models.py 8 years ago

create is our shortcut of adding the record and commit it, if you do that multiple times in one transation, I think that's a mis-use of this shortcut, you should just do that with the original approach of adding and commit.

cqi commented on line 118 of tests/handlers/test_image_builder.py 8 years ago

I don't think we need a separate test for record_koji_build. Instead, check whether ArtifactBuild is recorded or not in above existing tests, because record_koji_build is one of the steps of each rebuild that need to verify.

For example, in test test_ensure_do_nothing_if_fail_to_login_koji, when fail to login Koji, we need to check

nothing is build via buildContainer
no ArtifactBuild is stored into database

It does not make sense to separate second check into another test.

cqi commented on line 77 of tests/handlers/test_image_builder.py 8 years ago

How about split it into

msg = get_fedmsg("git_receive_dockerfile_changed")
self.consume_git_receive_msg(msg)

cqi commented on line 14 of freshmaker/handlers/image_builder.py 8 years ago

The only different between this method and record_module_build is the 4th parameter, 'image' and 'module'. Can we merge them into one method?

cqi commented on line 19 of freshmaker/models.py 8 years ago

the original approach of adding and commit.

Do you mean we have to write another create or repeat what create does to add more Events or ArtifactBuilds at once?

On the other hand, session is passed into create via a parameter, and it gets changed, by calling commit in this case, inside the method. I don't think it's a good idea.

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

Same idea above.

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

Same idea above.

qwan commented on line 19 of freshmaker/models.py 8 years ago

not exactly, what you need to do is:

db.session.add(...)
db.session.add(...)
db.session.commit()

create is a shortcut of add and commit, so it makes sense you should go back with add and commit in such cases instead of the shortcut. This is similar to Django's create which not requires you to save the record.

qwan commented on line 77 of tests/handlers/test_image_builder.py 8 years ago

What's the benefit of this? and where consume_git_receive_msg comes from?

cqi commented on line 77 of tests/handlers/test_image_builder.py 8 years ago

What's the benefit of this? and where consume_git_receive_msg comes from?

Scratch my comment about this.

rebased

8 years ago

qwan commented 8 years ago

Updated patches in previous version and merged them into one commit, added 2 commits to update build state in db when receives module built message and koji task state change message.

cqi commented on line 48 of freshmaker/handlers/buildsys.py 8 years ago

task_state is integer rather than str?

qwan commented on line 48 of freshmaker/handlers/buildsys.py 8 years ago

By the document, koji send the message with states in string rather than integer, I haven't check with the real message by now, datagrepper is unable to show messages under topic task.state.change (https://pagure.io/fedora-infrastructure/issue/6064), I'll capture a real message to confirm this. of course we can translate it to integer in Freshmaker, but I don't think it worth to do that since we'll need to import koji.TASK_STATE to do the translation.