Skip to content

Add support for arguments when declaring an exchange #89

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

Open
wants to merge 3 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 balooka closed this Aug 23, 2016
@balooka balooka reopened this Aug 23, 2016
@jordansissel
Copy link
Contributor

CLA failed, which is probably why nobody's reviewed this yet. Please see this comment for what to do --- specifically, the email you use in the git commits must be the same email as you signed for the CLA, and I do not see the email used in your commit in our CLA database.

@jordansissel
Copy link
Contributor

@balooka Let me know if you have any questions about how to resolve the CLA issue. :)

@balooka
Copy link
Author

balooka commented Mar 16, 2017

Hi @jordansissel ,
I followed the comment regarding CLA (adding the additional mail used to sign the CLA to my account.
And actually signed the CLA twice, once with each mail account.
I can if needed provide you with the transaction id for both.

@acchen97
Copy link

@balooka it appears you created these commits with your @staff.voo.be email address. Can you please sign the CLA with this one as well? After that, we should be good to go to move into the review stage.

@balooka
Copy link
Author

balooka commented Mar 16, 2017

@acchen97 signed with additional mail. Thanks for spotting the issue and sorry for the confusion.

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.

@@ -155,6 +155,10 @@ class RabbitMQ < LogStash::Inputs::Threadable
# to declare the exchange if it does not exist.
config :exchange_type, :validate => :string

# Additional arguments for exchange creation,
# for example, alternate-exchange
config :exchange_arguments, :validate => :array, :default => {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Consistency problem: You called this arguments in the output, but exchange_arguments here.

Also, same comments as in the output -- this is validate array but your default is a hash. Further, the docs aren't helpful enough, in my opinion.

Original pull request dates, making sure we take the comments into
account based on latest version
@asinha94
Copy link

asinha94 commented Jan 29, 2019

@jordansissel @balooka Any updates to this PR? I am willing to help out with this PR if you don't have the time

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.

5 participants