#4235 Add support for fedora-messaging in pagure
Merged by pingou. Opened by pingou.
fedora-messaging  into  master

Download 4235.patch

This effectively port pagure to the new messaging app used in Fedora
relying on AMQP.
As for the other messaging notifications this relies on a dedicated
configuration key.

However, projects will reuse the "fedmsg" option key.

Signed-off-by: Pierre-Yves Chibon pingou@pingoured.fr

1 new commit added

  • Fix default configuration key value to fit the actual default value

AMPQ -> AMQP

@pingou Why aren't we ripping out legacy fedmsg for this?

@jcline Does fedora-messaging support arbitrary AMQP brokers? If so, how do we use that? Because it may make sense to expose that and just make that part of the "general AMQP support" for Pagure.

@pingou Why aren't we ripping out legacy fedmsg for this?

Backward compatibility :)

Would be a good idea for 6.0

@pingou We should mark the fedmsg integration as deprecated then, to warn of the planned removal and suggested migration strategy.

The P was pushed to the beginning of the line :-)

@ngompa : we have only tested the library with RabbitMQ, but I don't think it uses any rabbitmq-specific extensions on the consumer or producer level, so it may work with other brokers.

That said, fedora-messaging isn't aimed at being a general puropose AMQP library, there are other libraries for that in Python (pika, librabbitmq, etc). So I wouldn't say that this PR adds "general AMQP support".

ouspy :s

Why is this flipped? This looks like it will now exit and not publish a notification if it's True?

The default value is False, so this adjusts the code to the actual default value.

not True is False, so the function will not return and thus it will send the mqtt notification :)

3 new commits added

  • Black fixes
  • Fix default configuration key value to fit the actual default value
  • Add support for fedora-messaging in pagure

rebased onto f5d124618b25c5f8a5efd2aee23e4460481fd966

rebased onto efc01fff97e5e1abcfc446632d77e6ef75c285a2

This seems correct, do you feel like there should be unit tests for this?

This seems correct, do you feel like there should be unit tests for this?

Basically, we do not have tests for any of these messaging libraries, we should tackle them all in one go (let's open a ticket to track this).

Then it LGTM :-)

thanks :)

rebased onto e99662c9eaf7101e84e8add5e28da90b168de279

Ticket created at: https://pagure.io/pagure/issue/4242

Pull-Request has been merged by pingou

Metadata