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

rabbitmq exchange type as config value #2104

Merged
merged 7 commits into from
Apr 28, 2020

Conversation

voicenter
Copy link
Contributor

By the feature request mention in #2023

@lminiero
Copy link
Member

Thanks! I'll notify the people in that issue about this effort so that they can test it.

@pitkonst
Copy link
Contributor

Looks fine for me. There is FIXME comment in both files (line 95 and 112), witch could be removed.

remove fixme  about that
remove fixme   about exchange type
@voicenter
Copy link
Contributor Author

voicenter commented Apr 27, 2020

@pitkonst , let me know if any more change needed,
I didn't add the parameter to the config file as it is optional,
let me know what is the project policy about it,
should I add it in the doc and sample config files...
thanks again ...
Shlomi

@pitkonst
Copy link
Contributor

About project policies we better ask @lminiero

@lminiero
Copy link
Member

A commented value in both config files (just to let people know it exists) would definitely help. You can use the existing lines as a template for the new property. I also noticed your patch has a lot of empty lines in between code snippets, so please just keep one for those (even though it's an easy nit I can fix myself after merging).

@lminiero
Copy link
Member

Thanks for the quick fixes! This seems good to merge for me: unless I see any objection on the original issue, I'll merge tomorrow morning 👍

@lminiero
Copy link
Member

I don't see any objection, so I'll merge, thanks again! 👍

@lminiero lminiero merged commit c1927c2 into meetecho:master Apr 28, 2020
This pull request was closed.
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