The commits can be reviewed separately, I can merge the commit porting to fedora-messaging with the one adding the tests if desired (I wanted to see if CI would kicked in on this PR and inform me whether just this change had broke something).

The second commit fixing a typo in the test could also likely be simplified, we're generating a list of messages sent but only checking the first one and we're already extracted the information above when checking the topic. So if desired I can drop the list from there. I can also move this change to a separate PR if desired.

I've tested the consumer using the following script:

from greenwave.consumers.resultsdb import ResultsDBHandler
from greenwave.consumers.waiverdb import WaiverDBHandler
import fedora_messaging.api
from fedora_messaging.config import conf
class Dummy(object):
    """ Dummy object only storing a dictionary named "config" that can be passed
    onto the fedmsg consumer.
    """
    def __init__(self, config):
        self.config = config
config = {
    "topic_prefix": "org.fedoraproject",
    "environment":  "dev",
    "resultsdb_topic_suffix": "taskotron.result.new"
}
hub = Dummy(config)
handler = ResultsDBHandler(hub)
msg = {"body": {'msg': {
    "task": {
      "item": "git-secret-0.2.5-2.fc31", 
      "type": "koji_build", 
      "name": "dist.rpmgrill"
    }, 
    "result": {
      "prev_outcome": None, 
      "outcome": "PASSED", 
      "id": 27816938, 
      "submit_time": "2019-03-05 15:30:43 UTC", 
      "log_url": "https://taskotron.fedoraproject.org/artifacts/all/56eeecd4-3f5b-11e9-a73c-525400fc9f92/tests.yml/rpmgrill.json"
    }
  }}}
handler.consume(msg)

The code doesn't explode, it also doesn't complete because contexts_product_versions is [] so there is that :)

I guess you can move this one before the if, since you do the same in both branches (if and elif).

s/fedora-messaing/fedora/messaging/g

Is that used in this file?

There are 2 white-spaces here.

Also here 2 white-spaces.

Mmm I think the bare except will fail in Flake8.

I'm not the best at English :D but shouldn't here be something like "Emitted a message on the message bus"? Or maybe "Emitted a message on the bus"?

Same here about the bare expect.

I guess s/bodhi/body/g :)

I'm afraid our pylint would fail here: W0703: Catching too general exception Exception (broad-except)

Doesn't "fedora_messaging" need to be added to the requirements?

Could you write a test also for waiverdb consumer?

Doesn't "fedora_messaging" need to be added to the requirements?

I've tried to make it optional, I'm fine with making it mandatory if you prefer :)

I thought I'd need it but apparently not, dropping it :)

fixing

fixing

fixing

fixing

I'll catch ImportError then

I'll adjust :)

Shall I make pylint ignore it or just remove the except?

Catching ImportError now :)

You haven't heard, fedora-messaging support bodhi now :-p

/me goes to fix

Should these print calls be replaced with logging method calls?

You're right, either log or plain remove, which do you prefer?

I would like to have the important things to be logged.
Maybe we can:
- log.debug() --> not so important stuff
- log.info() --> interesting logging (example: message published, or message not published because...)

@gnaponie adjusted :)

Since this is a repeated pattern, I wonder if we can use a mixin class to provide this dynamic publish property.

class DynamicPublisherMixin(object):
  def dynamic_pulish(self, topic, msg):
    ...
class ResultsDBHandler(fedmsg.consumers.FedmsgConsumer, DynamicPublisherMixin):
  def _publish_decision_changes(...):
    ...
    self.dynamic_pulish(topic, msg)
# And the same for WaiverDBHandler.

:+1: LGTM

My previous comment is simply a suggestion, not a blocker for this MR.

sounds good to me too

message => messages

Nitpick: Could you use single quotes like the rest of the method?

Nitpick: Could you use single quotes like the rest of the method?

In my opinion, this validation should happen when the greenwave module is initialized, and upon failure, an exception should be raised. What do you think @gnaponie?

Why was this log message updated in this consumer but not the other?

Yeah, agreed, it would be preferable.

Why is this log message in this consumer but not the other?

Why is this log message in this consumer but not the other?

Same comment as in the other consumer

This seems very similar to the test above it. Could you use pytest parametrize instead? It'd make the maintenance of the tests easier.

fixing

fixing

fixing

fixing

fixing

Fixing the quotes here and above and in the two files

Do we have a mechanism in place for this? Do you want me to build one?
I guess we would basically like a mechanism checking the entire configuration file, so should we make this part of this PR or record this in a ticket and fix it in another PR?

I'll see what I can do :)

@pingou How about you just remove the else and we can handle the configuration validation in a separate ticket/PR?

@pingou How about you just remove the else and we can handle the configuration validation in a separate ticket/PR?

I can most certainly do this :)

At this point I've all the changes lined up except for the pytest one, I'll look drop the else clause and fix the pytest one tomorrow :)

Thanks!

All done :)

It seems fine to me. @mprahl could you review the latest changes?

Looks goods @pingou. Thanks for the PR!