Skip to content
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

Add support for arguments when declaring an exchange #54

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

balooka
Copy link

@balooka balooka commented Aug 5, 2016

Thanks for contributing to Logstash! If you haven't already signed our CLA, here's a handy link: https://www.elastic.co/contributor-agreement/

@karmi
Copy link

karmi commented Aug 5, 2016

Hi @balooka, we have found your signature in our records, but it seems like you have signed with a different e-mail than the one used in yout Git commit. Can you please add both of these e-mails into your Github profile (they can be hidden), so we can match your e-mails to your Github profile?

@balooka
Copy link
Author

balooka commented Aug 5, 2016

CI failure is due to dependency to another pull request on logstash-mixin-rabbitmq_connection project

@balooka balooka closed this Aug 23, 2016
@balooka balooka reopened this Aug 23, 2016
@balooka
Copy link
Author

balooka commented Mar 16, 2017

Added missing mail to CLA. Apologies for the delay (hadn't realized the exact mail was showing in your mail, wrongly assumed the linked mail was another one)

Copy link
Contributor

@jordansissel jordansissel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for helping improve logstash! I left some comments about the new setting.

Also, this change does not add any tests, so it cannot be merged until there are tests covering it.

@@ -36,9 +36,12 @@ class RabbitMQ < LogStash::Outputs::Base
# Should RabbitMQ persist messages to disk?
config :persistent, :validate => :boolean, :default => true

# Additional arguments for the exchange, for example could be the presence of an alternate exchange, ...
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Conflict here -- the :validate => :array doesn't match up with your default value (a hash).

Also, the documentation for this setting isn't quite enough. Can you detail why a user would want to set this?

Is "arguments" the right word? It feels like it is not specific enough of a word.

Original pull request dates, making sure we take comments into account
based on latest version
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants