From c8351da5f057a7459425970499d3e035d48f19da Mon Sep 17 00:00:00 2001 From: Jan Kaluza Date: Nov 14 2017 09:16:11 +0000 Subject: Publish build.state.changed and event.state.changed messages to message bus. --- diff --git a/freshmaker/consumer.py b/freshmaker/consumer.py index 80c8450..e373f3b 100644 --- a/freshmaker/consumer.py +++ b/freshmaker/consumer.py @@ -28,7 +28,7 @@ to use. import fedmsg.consumers import moksha.hub -from freshmaker import log, conf, messaging, events +from freshmaker import log, conf, messaging, events, app from freshmaker.utils import load_classes @@ -100,7 +100,16 @@ class FreshmakerConsumer(fedmsg.consumers.FedmsgConsumer): # Primary work is done here. try: - self.process_event(msg) + # There is no Flask app-context in the backend and we need some, + # because models.Event.json() and models.ArtifactBuild.json() uses + # flask.url_for, which needs app_context to generate the URL. + # We also cannot generate Flask context on the fly each time in the + # mentioned json() calls, because each generation of Flask context + # changes db.session and unfortunatelly does not give it to original + # state which might be Flask bug, so the only safe way on backend is + # to have global app_context. + with app.app_context(): + self.process_event(msg) except Exception: log.exception('Failed while handling {0!r}'.format(msg)) diff --git a/freshmaker/handlers/errata/errata_advisory_rpms_signed.py b/freshmaker/handlers/errata/errata_advisory_rpms_signed.py index f755d23..3e0a847 100644 --- a/freshmaker/handlers/errata/errata_advisory_rpms_signed.py +++ b/freshmaker/handlers/errata/errata_advisory_rpms_signed.py @@ -136,7 +136,9 @@ class ErrataAdvisoryRPMsSignedHandler(BaseHandler): for url in repo_urls: log.info(" - %s", url) - messaging.publish('images.found', db_event.json()) + # TODO: Once https://pagure.io/freshmaker/issue/137 is fixed, this + # should be moved to models.Event.transition(). + messaging.publish('event.state.changed', db_event.json()) return [] diff --git a/freshmaker/models.py b/freshmaker/models.py index cde3b9d..fb62fd3 100644 --- a/freshmaker/models.py +++ b/freshmaker/models.py @@ -33,6 +33,8 @@ from sqlalchemy.orm import (validates, relationship) from flask_login import UserMixin from freshmaker import app, db, log +from freshmaker import messaging +from freshmaker.utils import get_url_for from freshmaker.types import ArtifactType, ArtifactBuildState from freshmaker.events import ( MBSModuleStateChangeEvent, GitModuleMetadataChangeEvent, @@ -210,21 +212,24 @@ class Event(FreshmakerBase): for build in self.builds: build.transition(state, reason) + # TODO: Once https://pagure.io/freshmaker/issue/137 is fixed, this + # should be moved to models.Event.transition(). + messaging.publish('event.state.changed', self.json()) + def __repr__(self): return "" % (self.message_id, self.event_type, self.search_key) def json(self): - with app.app_context(): - event_url = flask.url_for('event', id=self.id) - db.session.add(self) - return { - "id": self.id, - "message_id": self.message_id, - "search_key": self.search_key, - "event_type_id": self.event_type_id, - "url": event_url, - "builds": [b.json() for b in self.builds], - } + event_url = get_url_for('event', id=self.id) + db.session.add(self) + return { + "id": self.id, + "message_id": self.message_id, + "search_key": self.search_key, + "event_type_id": self.event_type_id, + "url": event_url, + "builds": [b.json() for b in self.builds], + } class EventDependency(FreshmakerBase): @@ -342,6 +347,8 @@ class ArtifactBuild(FreshmakerBase): self.state, "Cannot build artifact, because its " "dependency cannot be built.") + messaging.publish('build.state.changed', self.json()) + def __repr__(self): return "" % ( self.name, ArtifactType(self.type).name, @@ -352,27 +359,26 @@ class ArtifactBuild(FreshmakerBase): if self.build_args: build_args = json.loads(self.build_args) - with app.app_context(): - build_url = flask.url_for('build', id=self.id) - db.session.add(self) - return { - "id": self.id, - "name": self.name, - "original_nvr": self.original_nvr, - "rebuilt_nvr": self.rebuilt_nvr, - "type": self.type, - "type_name": ArtifactType(self.type).name, - "state": self.state, - "state_name": ArtifactBuildState(self.state).name, - "state_reason": self.state_reason, - "dep_on": self.dep_on.name if self.dep_on else None, - "time_submitted": _utc_datetime_to_iso(self.time_submitted), - "time_completed": _utc_datetime_to_iso(self.time_completed), - "event_id": self.event_id, - "build_id": self.build_id, - "url": build_url, - "build_args": build_args, - } + build_url = get_url_for('build', id=self.id) + db.session.add(self) + return { + "id": self.id, + "name": self.name, + "original_nvr": self.original_nvr, + "rebuilt_nvr": self.rebuilt_nvr, + "type": self.type, + "type_name": ArtifactType(self.type).name, + "state": self.state, + "state_name": ArtifactBuildState(self.state).name, + "state_reason": self.state_reason, + "dep_on": self.dep_on.name if self.dep_on else None, + "time_submitted": _utc_datetime_to_iso(self.time_submitted), + "time_completed": _utc_datetime_to_iso(self.time_completed), + "event_id": self.event_id, + "build_id": self.build_id, + "url": build_url, + "build_args": build_args, + } def get_root_dep_on(self): dep_on = self.dep_on diff --git a/freshmaker/utils.py b/freshmaker/utils.py index ccc40d3..1ba9982 100644 --- a/freshmaker/utils.py +++ b/freshmaker/utils.py @@ -32,9 +32,27 @@ import tempfile import time import koji -from freshmaker import conf +from freshmaker import conf, app, log from freshmaker.types import ArtifactType from krbcontext import krbContext +from flask import has_app_context, url_for + + +def get_url_for(*args, **kwargs): + """ + flask.url_for wrapper which creates the app_context on-the-fly. + """ + if has_app_context(): + return url_for(*args, **kwargs) + + # Localhost is right URL only when the scheduler runs on the same + # system as the web views. + app.config['SERVER_NAME'] = 'localhost' + with app.app_context(): + log.warn("get_url_for() has been called without the Flask " + "app_context. That can lead to SQLAlchemy errors caused by " + "multiple session being used in the same time.") + return url_for(*args, **kwargs) def get_rebuilt_nvr(artifact_type, nvr): diff --git a/tests/__init__.py b/tests/__init__.py index cf36e15..611728e 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -23,6 +23,7 @@ from os import path import json +from freshmaker import app def get_fedmsg(name): @@ -31,3 +32,16 @@ def get_fedmsg(name): with open(fedmsg_path, 'r') as f: return {'body': json.load(f)} + + +# There is no Flask app-context in the tests and we need some, +# because models.Event.json() and models.ArtifactBuild.json() uses +# flask.url_for, which needs app_context to generate the URL. +# We also cannot generate Flask context on the fly each time in the +# mentioned json() calls, because each generation of Flask context +# changes db.session and unfortunatelly does not give it to original +# state which might be Flask bug, so the only safe way on backend is +app_context = app.app_context() +# We do not care about __exit__ in a tests, because the app_context is +# just use during the whole test-suite run. +app_context.__enter__() diff --git a/tests/test_messaging.py b/tests/test_messaging.py index ee34b57..80ebb45 100644 --- a/tests/test_messaging.py +++ b/tests/test_messaging.py @@ -27,6 +27,7 @@ import unittest from mock import patch from freshmaker import conf +from freshmaker import messaging from freshmaker.messaging import publish try: @@ -35,7 +36,17 @@ except ImportError: rhmsg = None -class TestSelectMessagingBackend(unittest.TestCase): +class BaseMessagingTest(unittest.TestCase): + """ Base class for messaging related tests """ + + def setUp(self): + messaging._in_memory_msg_id = 0 + + def tearDown(self): + messaging._in_memory_msg_id = 0 + + +class TestSelectMessagingBackend(BaseMessagingTest): """Test messaging backend is selected correctly in publish method""" @patch('freshmaker.messaging._fedmsg_publish') @@ -74,7 +85,7 @@ class TestSelectMessagingBackend(unittest.TestCase): messaging_patcher.start) -class TestPublishToFedmsg(unittest.TestCase): +class TestPublishToFedmsg(BaseMessagingTest): """Test publish message to fedmsg using _fedmsg_publish backend""" @patch.object(conf, 'messaging_sender', new='fedmsg') @@ -91,7 +102,7 @@ class TestPublishToFedmsg(unittest.TestCase): @unittest.skipUnless(rhmsg, 'rhmsg is not available in Fedora yet.') @unittest.skipIf(six.PY3, 'rhmsg has no Python 3 package so far.') -class TestPublishToRhmsg(unittest.TestCase): +class TestPublishToRhmsg(BaseMessagingTest): """Test publish message to UMB using _rhmsg_publish backend""" @patch.object(conf, 'messaging_sender', new='rhmsg') @@ -124,7 +135,7 @@ class TestPublishToRhmsg(unittest.TestCase): Message.return_value) -class TestInMemoryPublish(unittest.TestCase): +class TestInMemoryPublish(BaseMessagingTest): """Test publish message in memory using _in_memory_publish backend""" @patch('freshmaker.consumer.work_queue_put')