New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Get fedmsg-hub working on STOMP. #380

Merged
merged 9 commits into from Oct 13, 2016

Conversation

Projects
None yet
3 participants
@ralphbean
Contributor

ralphbean commented Oct 9, 2016

The goal here is to be able to take the litany of apps we've written (FMN, fedmsg-irc, datanommer, fedimg, etc..) and be able to run them in other messaging environments (in particular, the RH unified message bus).

I intend to support:

  • The hub/consumer approach. If you have a fedmsg consumer, it should be able
    to listen to messages over other protocols now.
  • Publishing with fedmsg.publish. This will only work from a fedmsg
    consumer. It will figure out the moksha context and ask it to publish on
    fedmsg's behalf, thereby publishing over STOMP or AMQP, depending on
    configuration.

I explicitly am not supporting:

  • The naive listening approach, i.e. fedmsg.tail_messages(). It would be a
    ton of work and its not worth it atm. If we find a really good reason to
    implement support for it, we can always revisit this later.
@ralphbean

This comment has been minimized.

Contributor

ralphbean commented Oct 9, 2016

FYI, the code here is probably ready to go, but I'm waiting for a dev broker to become available to do some integration testing. Let's wait to merge this until I can confirm things work at that level.

@pypingou

This comment has been minimized.

Member

pypingou commented Oct 10, 2016

Code wise it looks good to me, maybe we could just document somewhere how the configuration file should look like then (since it needs the moksha hub to be correctly configured).

Let's wait for your tests and then 👍 to merge for me.

@ralphbean

This comment has been minimized.

Contributor

ralphbean commented Oct 11, 2016

OK, this works now! It depends on mokshaproject/moksha#36 too..

@ralphbean

This comment has been minimized.

Contributor

ralphbean commented Oct 11, 2016

And, docs available now too. 👍 to merge from me.

Any concerns?

@@ -154,7 +154,9 @@ def handle_msg(self, msg, **config):
return match.groups()[-1] or ""
def title(self, msg, **config):
return '.'.join(msg['topic'].split('.')[3:])
if msg['topic'][0].isalpha():

This comment has been minimized.

@pypingou

pypingou Oct 12, 2016

Member

This checks that the first letter of the topic is alphabetic, right? What about checking that there are at least 3 . in there?

Also, thoughts on using msg['topic'].split('.', 3)[-1] below?

This comment has been minimized.

@ralphbean

ralphbean Oct 13, 2016

Contributor

FYI, the topic structure for stomp looks like /topic/Consumer.....

Could I check instead if if msg['topic'].startswith('/topic/') ?

This comment has been minimized.

@ralphbean

ralphbean Oct 13, 2016

Contributor

Added in 9159488

This comment has been minimized.

@pypingou

pypingou Oct 13, 2016

Member

Cool, what about the msg['topic'].split('.', 3)[-1]? Not that it matters more just an idea

This comment has been minimized.

@ralphbean

ralphbean Oct 13, 2016

Contributor

It makes my head spin trying to imagine all the different combinations of dots.. 😵

This comment has been minimized.

@pypingou

pypingou Oct 13, 2016

Member

fair :)

following configuration to `/etc/fedmsg.d/base.py`::
# We almost always want the fedmsg-hub to be sending messages with zmq as
# opposed to amqp or stomp.

This comment has been minimized.

@pypingou

pypingou Oct 12, 2016

Member

Should we precise: you cannot have it all or you cannot have both?

This comment has been minimized.

@sayanchowdhury

sayanchowdhury Oct 12, 2016

Member

I think the same.

This comment has been minimized.

@ralphbean

ralphbean Oct 13, 2016

Contributor

Yup.

This comment has been minimized.

@ralphbean

ralphbean Oct 13, 2016

Contributor

Fixed in 5df1e48.

@pypingou

This comment has been minimized.

Member

pypingou commented Oct 12, 2016

Couple of questions but still looking good :)

@pypingou

This comment has been minimized.

Member

pypingou commented Oct 13, 2016

👍 for me

@ralphbean

This comment has been minimized.

Contributor

ralphbean commented Oct 13, 2016

Thanks @pypingou!

@ralphbean

This comment has been minimized.

Contributor

ralphbean commented Oct 13, 2016

Can you merge it when you have a moment?

@pypingou

This comment has been minimized.

Member

pypingou commented Oct 13, 2016

Sure thing

@pypingou pypingou merged commit e8abf14 into fedora-infra:develop Oct 13, 2016

1 check failed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment