1063791 - Move TranslatorPublisher out of pulsetranslator and into mozillapulse

Status: RESOLVED FIXED

(Webtools :: Pulse, defect)

Description

[Mark Côté [:mcote]]

Any public pulse service should have its publisher class in mozillapulse rather than in the publishing app itself.  This keeps all exchange names in one place for easy reference and in case we ever have to rename them again (as we are doing right now).

In this case, we already have NormalizedBuildConsumer in there, so we should move TranslatorPublisher out of pulsetranslator and into mozillapulse as NormalizedBuildPublisher.  In addition, TranslatorConsumer should be removed from pulsetranslator and any uses of it switched to NormalizedBuildConsumer.

Comment 1

[Mark Côté [:mcote]]

Attachment: Add NormalizedBuildPublisher

Also passes **kwargs (sigh) onto the superclass constructor so that we can set the 'connect' variable.

Comment 2

[Jonathan Griffin (:jgriffin)]

Comment on attachment 8487570 [details] [diff] [review]
Add NormalizedBuildPublisher

Review of attachment 8487570 [details] [diff] [review]:
-----------------------------------------------------------------

Makes sense, thanks.

Comment 3

[Mark Côté [:mcote]]

https://hg.mozilla.org/automation/mozillapulse/rev/0bb8d916b03e

Comment 4

[Mark Côté [:mcote]]

Actually, reopening this for the pulsetranslator side, although also going to be lumping in pulse configuration-file support.

Comment 5

[Mark Côté [:mcote]]

Attachment: Remove publisher and consumer classes from pulsetranslator; add support for pulse config files

This does a few things:

* Strips out the built-in publisher and consumer classes in favour of those now in mozillapulse.
* Adds support for pulse configuration files.
* Removes the broken test-message support.
* Some PEP-8 fixes.

I kind of hate how PulseConfiguration has to be inserted now, after instantiation of the publisher/consumer and thus requiring connect=False, but I'll fix that some other time.

Comment 6

[Jonathan Griffin (:jgriffin)]

Comment on attachment 8487607 [details] [diff] [review]
Remove publisher and consumer classes from pulsetranslator; add support for pulse config files

Review of attachment 8487607 [details] [diff] [review]:
-----------------------------------------------------------------

We should also update the mozillapulse dep in setup.py.

::: README.md
@@ -5,5 @@
> -
> -The normalized messages are published to the exchange "org.mozilla.exchange.build.normalized".
> -
> -
> -Routing Keys

Why are we nuking README.md?

Comment 7

[Mark Côté [:mcote]]

(In reply to Jonathan Griffin (:jgriffin) from comment #6)
> Comment on attachment 8487607 [details] [diff] [review]
> Remove publisher and consumer classes from pulsetranslator; add support for
> pulse config files
> 
> Review of attachment 8487607 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> We should also update the mozillapulse dep in setup.py.
> 
> ::: README.md
> @@ -5,5 @@
> > -
> > -The normalized messages are published to the exchange "org.mozilla.exchange.build.normalized".
> > -
> > -
> > -Routing Keys
> 
> Why are we nuking README.md?

Wow, that's a Splinter fail.  If you look at the diff directly (raw or formatted), you can see that I just split two very long lines into a number of less-than-80-character lines.

Comment 8

[Mark Côté [:mcote]]

Oh, the other thing that I did to README.md was dos2unix it; maybe that also confused Splinter.  Anyway you can see the results in the GitHub commit.

https://github.com/mozilla/pulsetranslator/commit/cd9feca560474ff03b0d05745d52c01551cfc8c5

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Mark Côté [:mcote] from comment #5)
> Created attachment 8487607 [details] [diff] [review]
> Remove publisher and consumer classes from pulsetranslator; add support for
> pulse config files
> 
> * Removes the broken test-message support.

Can you please explain this a bit more? What was broken with that? It was very helpful to actually test pulse messages via pulsetranslator. Now I do not see a way to push in a message, which breaks pulsetranslator or doesn't let process it. Also you removed all test messages which existed under test. Why that? Right now it makes it very hard to debug broken messages with pulsetranslator. :(

This kinda blocks me now for investigating the problem on bug 1105356.

Comment 10

[Mark Côté [:mcote]]

As far as I remember, those test messages were all out of date, and at least several of them were generating bad-message errors.  I admit to not having tried all of them, but jgriffin and I decided they weren't serving any purpose in their current state.  I have no problems adding new ones back in, although I would prefer if there were a test set up at the same time to verify that they are still relevant.

Comment 11

[Mark Côté [:mcote]]

I'd like to keep this FIXED; if you want to add new test messages, please file a new bug, since I don't want to add obsolete messages back in.