PR#135: Make fedora-messaging optional - taskotron/resultsdb

taskotron / resultsdb

#135 Make fedora-messaging optional

Merged 5 years ago by jskladan. Opened 5 years ago by mprahl.

taskotron/ mprahl/resultsdb fedora-messaging-optional into develop

Make fedora-messaging optional

mprahl • 5 years ago

2ecb78d

resultsdb/messaging.py

file modified

+10 -4

		`@@ -22,16 +22,22 @@`

		`import pkg_resources`

		`- from fedora_messaging.api import Message, publish`
		`- from fedora_messaging.exceptions import PublishReturned, ConnectionException`
		`-`
		`- from resultsdb import db`
		`+ from resultsdb import db, app`
		`from resultsdb.models.results import Result, ResultData`
		`from resultsdb.serializers.api_v2 import Serializer`

		`import logging`
		`log = logging.getLogger(__name__)`

		`+ try:`
		`+ from fedora_messaging.api import Message, publish`
		`+ from fedora_messaging.exceptions import PublishReturned, ConnectionException`
		`+ except ImportError:`
		`+ if app.config.get('MESSAGE_BUS_PUBLISH_TASKOTRON') or app.config.get('MESSAGE_BUS_PLUGIN'):`
		`+ log.error('fedora-messaging must be installed if "MESSAGE_BUS_PUBLISH_TASKOTRON" is '`
		`+ 'enabled or "MESSAGE_BUS_PLUGIN" is set to "fedmsg"')`
		`+ raise`
		`+`

		`SERIALIZE = Serializer().serialize`

mprahl commented 5 years ago

Since fedora-messaging is not packaged in EPEL7, RHEL 7 deployments outside of Fedora Infra can't upgrade to v2.2.0. This will allow
for a deployment that uses alternate messaging plugins such as stomp.

jskladan commented 5 years ago

How about:

  - from fedora_messaging.api import Message, publish
  - from fedora_messaging.exceptions import PublishReturned, ConnectionException

  + try:
  +     from fedora_messaging.api import Message, publish
  +     from fedora_messaging.exceptions import PublishReturned, ConnectionException
  + except ImportError:
  +     # EPEL does not have fedora_messaging packaged, see: https://pagure.io/taskotron/resultsdb/pull-request/135
  +     logging.info("EPEL does not have fedora_messaging packaged (yet?), see: https://pagure.io/taskotron/resultsdb/pull-request/135")

instead?

Also, are you sure you don't want to have some substitute instead? The patch as you propose (IIUIC) just plain removes any messaging ability from ResultsDB in EPEL altogether (which does not sound right).

This would also require specfile changes, and maybe also testsuite changes, in order to build the package for EPEL (again IIUIC @frantisekz probably knows more).

Edited 5 years ago by jskladan

mprahl commented 5 years ago

How about:
instead?
Also, are you sure you don't want to have some substitute instead? The patch as you propose (IIUIC) just plain removes any messaging ability from ResultsDB in EPEL altogether (which does not sound right).
This would also require specfile changes, and maybe also testsuite changes, in order to build the package for EPEL (again IIUIC @frantisekz probably knows more).

A ResultsDB deployment on RHEL 7 can still use stomp for messaging, which is provided in EPEL7 through the stomppy RPM.

Does this change your opinion on this PR at all?

Edited 5 years ago by mprahl

jskladan commented 5 years ago

It clears things up as for the intent/merit. I'd still rather see the change be done as I shown in https://pagure.io/taskotron/resultsdb/pull-request/135#comment-81422 rather than having the imports "randomly" scattered around the file.

I also do not think the requirements.txt change is really necessary or right - the fedora-messaging is no needed "just" for testing, it's just that ResultsDB (in the proposed state) could technically work without it.

If packaging for EPEL7/RHEL7 is done separately (or from a different specfile), as you discussed on IRC with @frantisekz , then that solves the other part of my issue/question.

rebased onto 2ecb78d

5 years ago

mprahl commented 5 years ago

Thank you for the review. I responded inline below.

It clears things up as for the intent/merit. I'd still rather see the change be done as I shown in https://pagure.io/taskotron/resultsdb/pull-request/135#comment-81422 rather than having the imports "randomly" scattered around the file.

I modified the PR to make this centralized as requested, but I have it raise an exception if fedora-messaging is not installed but is required. The reason I had it the other way originally is because that's how it was done for the STOMP messaging plugin.

I also do not think the requirements.txt change is really necessary or right - the fedora-messaging is no needed "just" for testing, it's just that ResultsDB (in the proposed state) could technically work without it.

I disagree with this. A ResultsDB deployment works just fine without it, but I put it back to the way it was as requested.

If packaging for EPEL7/RHEL7 is done separately (or from a different specfile), as you discussed on IRC with @frantisekz , then that solves the other part of my issue/question.

jskladan commented 5 years ago

Awesome, thanks!