Since fedmsg does not support using STOMP in a wsgi, I've adopted this approach used by resultsdb
https://pagure.io/taskotron/resultsdb/blob/develop/f/resultsdb/messaging.py#_107
Test Plan: I'll test it on the dev server which is hooked up to publish messages to UMB.
2 new commits added
rebased
I would love to a functional test which sends a message to an actual broker, and asserts that it can be received -- instead of this mock unit test stuff. Hunor was planning to do something very similar for the resultsdb-updater integration tests anyway. I wonder if we can re-use the same approach?
Does this handle reconnecting in case the remote end dropped the connection? For example if we idle for >10 hours the TCP connection might drop etc. Or if the broker is restarted.
Would it be easier/more reliable to just open a new connection to the broker for every message we want to publish?
Could reduce these to a single tri-state option, like:
MESSAGING = 'fedmsg|stomp|none'
but this is fine too.
Oh... it actually does disconnect and then reconnect for every message it sends. So why the thing with the global connection? That's just confusing. Make it a local so it's clear that it only lives for this function call.
Also you could probably re-use the same connection for all messages in the loop, since they will all be fired off straight away.
Hmm, has Hunor finished it? I can't see it in resultsdb-updater.
Yeah, that's why I use a global connection so that it can be used for all messages in the loop. Here, I should set the global stomp_conn = None in case an error happens, then it will reconnect for next message.
I agree that using a local variable here will be less confusing, :-)
It's still in progress so far as I know. He has some proof of concept, but not rigged up in a Jenkinsfile yet.
I will leave this test here and we can refactor it later.
Rebased to address the code reviews. Unfortunatelly, I've removed the unit test as I can't think of a clean way to test it with mock. I'll wait for Hunor and see if we can adpot his approach used by resultsdb-updater.
I've verifed on the wavierdb dev box and it works as expected, :v:
Pagure still claims this PR includes two of my existing unrelated commits... maybe this needs a rebase again?
Why remove from __future__ import unicode_literals? We should probably have that consistently in all our source files, since it should make cross Python-2-3 compatibility easier.
from __future__ import unicode_literals
... or consistently in none of them, depending on which side of the fence we decide to fall on:
http://python-future.org/unicode_literals.html
Hmm, I forgot to fill in the reason why I removed unicode_literal. It seems we don't need to support Pthon 3.2, so I am keen to use u'' prefix instead of it. However, I shouldn't remove it in this patch, I'll file a new issue and then we can discuss this over there.
Yeah consistently removing it is okay -- but yeah let's do that separately.
Rebased to address Dan's comment.
:+1:
Pull-Request has been merged by mjia
Since fedmsg does not support using STOMP in a wsgi, I've adopted this approach used by resultsdb
https://pagure.io/taskotron/resultsdb/blob/develop/f/resultsdb/messaging.py#_107
Test Plan: I'll test it on the dev server which is hooked up to publish messages to UMB.