-
Notifications
You must be signed in to change notification settings - Fork 746
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
feat: AMQPEventSource extra parameters. Fixes #1007 #1009
Conversation
Signed-off-by: Davide Berdin <davideberdin@gmail.com>
Signed-off-by: Davide Berdin <davideberdin@gmail.com>
d9ff89a
to
bb012fb
Compare
Signed-off-by: Davide Berdin <davideberdin@gmail.com>
e1ee476
to
5dfa4f7
Compare
I created a |
eventsources/sources/amqp/start.go
Outdated
@@ -155,24 +161,93 @@ func (el *EventListener) StartListening(ctx context.Context, dispatch func([]byt | |||
} | |||
} | |||
|
|||
// setDefaults sets the default values in case the user hasn't defined them | |||
// helps also to keep retro-compatibility with current dpeloyments | |||
func setDefaults(eventSource *v1alpha1.AMQPEventSource) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks good to me except a minor: there's no chance/need to return error in this function. Also, please confirm it is backward compatible with real testing - I don't have an AMQP env setup, can not do a test :), thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested this branch using the example on the website and it works without issues. The messages below are a confirmation
{"level":"info","ts":1610568336.564611,"logger":"argo-events.eventsource","caller":"eventsources/eventing.go:350","msg":"succeeded to publish an event","eventSourceName":"amqp","eventName":"example","eventSourceType":"amqp"}
{"level":"info","ts":1610568406.6777697,"logger":"argo-events.eventsource","caller":"amqp/start.go:114","msg":"received the message","eventSourceName":"amqp","eventSourceType":"amqp","eventName":"example","message-id":""}
{"level":"info","ts":1610568406.6780057,"logger":"argo-events.eventsource","caller":"amqp/start.go:143","msg":"dispatching event ...","eventSourceName":"amqp","eventSourceType":"amqp","eventName":"example"}
{"level":"info","ts":1610568406.6828492,"logger":"argo-events.eventsource","caller":"eventsources/eventing.go:350","msg":"succeeded to publish an event","eventSourceName":"amqp","eventName":"example","eventSourceType":"amqp"}
If somebody encounters an issue, I'll be happy to fix it 😄
Signed-off-by: Davide Berdin <davideberdin@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks!
…#1009) * add more configuration parameters to AMQPEventSource Signed-off-by: Davide Berdin <davideberdin@gmail.com> * add optional values in the amqp event source example Signed-off-by: Davide Berdin <davideberdin@gmail.com> * add nil checks and set default parameters Signed-off-by: Davide Berdin <davideberdin@gmail.com> * remove returned error from setDefaults() func) Signed-off-by: Davide Berdin <davideberdin@gmail.com>
This PR aims to fix #1007.
The following new properties are added to the
AMQPEventSource
object:ExchangeDeclare
to set the values of the exchange in the serverQueueDeclare
to hold the configuration of a queue to hold messages and deliver to consumersQueueBind
to hold the configuration that binds an exchange to a queue so that publishings to the exchange will be routed to the queue when the publishing routing key matches the binding routing keyConsume
to hold the configuration to immediately starts delivering queued messagesAll these new properties are optional. The user can take a look at the
amqp.yaml
example file to see how to set them up.