add hook possibility for consumers to configure themselves

[glensc]

proposed solution for #427

add hook possibility for consumers to configure themselves,
for example setup topic based on config, etc

[codecov-io]

Codecov Report

Merging #428 into develop will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           develop     #428   +/-   ##
========================================
  Coverage    47.15%   47.15%           
========================================
  Files           33       33           
  Lines         2059     2059           
  Branches       331      331           
========================================
  Hits           971      971           
  Misses         993      993           
  Partials        95       95

Impacted Files	Coverage Δ
fedmsg/consumers/__init__.py	72.09% <ø> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5aad9af...3e32322. Read the comment docs.

[jeremycline]

Thanks for the PR, @glensc!

I agree with you that the consumer API is hard to work with and needs to be changed.

However, I'm hesitant to accept this for a few reasons. The first is that when I saw a configure method on the class, my expectation was that it was an API to configure the consumer "from the outside". This looks like an API that should never be called by the creator of a consumer object. The second is that it introduces a second way to do something (initialize a consumer), which is confusing.

What I'd really like to do is flatten out the inheritance structure here by dropping Moksha as a dependency and defining our own consumer API. The combination of fewer layers of inheritance along with better error handling and reporting would, I think, do wonders for the usability of this. What I haven't done is come up with a concrete plan for getting rid of Moksha.

[glensc]

this actually does not work as expected. for #427 to be able to configure topic dynamically (from config) the configure needs to be called:

1. after method to fetch config is initialized (self.hub = hub assigned likely by moksha.hub.api.consumer.Consumer)
2. before parent class is called

because doing after __init__ the topic value in consumer is always current topic what msg is passed for consume, not the one intended to "listen" to

but they both happen in parent class:
https://github.com/fedora-infra/fedmsg/blob/0.18.3/fedmsg/consumers/__init__.py#L98

so the current way, i see this could work, is to call on hub directly and then call parent constructor in consumer class:

class MyConsumer(fedmsg.consumers.FedmsgConsumer):
    config_key = "my_consumer_enabled"
    
    def __init__(self, hub):
        # I'm only interested in messages by this topic
        self.topic = hub.config.get('my_topic')

        super(MyConsumer, self).__init__(hub)

[jeremycline]

That specific example probably won't work because topic is scoped to __init__ rather than the class. What's worse, you have to step outside the fedmsg codebase to figure out what's happening because moksha, not fedmsg, uses the topic. This is the relevant code - you should be fine doing self.topic = hub.config.get('my_topic') before calling the parent's __init__.

[glensc]

the example given here, does work. i.e the same way you described: init self.topic by reading hub.config.get() and then call parent __init__, and it's ugly, that's why i wanted configure method, it could be called initialize if the naming is the only concern. however this PR does not work we both agree.

ps: never link to branch, use commit or hash to ensure the link stays valid when branch is updated!

[glensc]

actually, i wouldn't want to configure topic at all if it would be relative to topic_prefix + environment, that's another implementation shortcoming.

added locally to my code:

    def __init__(self, hub):
        # listen to file commits
        self.topic = self.abs_topic(hub.config, "my.key")

        super(CVSConsumer, self).__init__(hub)

    # no proper way to configure just topic suffix
    # https://github.com/fedora-infra/fedmsg/pull/428
    def abs_topic(self, config, topic):
        """
        prefix topic with topic_prefix and environment config values
        """
        topic_prefix = config.get('topic_prefix')
        environment = config.get('environment')
        return "%s.%s.%s" % (topic_prefix, environment, topic)

[glensc]

@jeremycline here's my project i'm working on: https://github.com/glensc/fedmsg-cvs

i don't mind if you do small codereview there and perhaps open ticket about problems/solutions how to do things better.

it "works", but likely doesn't use any best practices

[jeremycline]

Hey @glensc, I know we talked about some of your issues with fedmsg-cvs on IRC. Are you good to go? I'm pretty new to fedmsg myself, so I'm not sure about best practices either.

As I outlined above, I'd really like to make this whole process easier. However, in my opinion adding this API will just muddy the waters further. However, I really appreciate you highlighting this issue and submitting this PR!