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.

Sign up for GitHub

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

Jump to bottom

Automatically determine the RabbitMQ protocol when possible #1459

Merged
merged 2 commits into from
Jan 6, 2021
Merged

Automatically determine the RabbitMQ protocol when possible #1459

merged 2 commits into from
Jan 6, 2021

Conversation

coderanger
Copy link
Contributor

@coderanger coderanger commented Dec 29, 2020 •
edited
Loading

And support setting the protocol via TriggerAuthentication since if overriding host, you will probably want that too at some point (if the new auto-detection doesn't handle it for you).

Checklist

@coderanger coderanger mentioned this pull request Dec 29, 2020
Merged
@coderanger
Copy link
Contributor Author

Honestly I'm not 100% sure there's a reason to leave protocol as a configurable thing rather than making it always automatic. The URLs that are accepted by either httpclient.Get() or amql.Dial() are limited to these specific schemes I think? There might be some cases for taking an HTTP URL and forcing it to an AMQP connection, but that's all I can think of and not even 100% sure Dial will accept that.

@coderanger coderanger force-pushed the rabbitmq-auto-protocol branch from ea3597b to 386a2f1 Compare December 29, 2020 14:07
tomkerkhove
tomkerkhove approved these changes Jan 4, 2021
View reviewed changes
Copy link
Member

@tomkerkhove tomkerkhove left a comment

Choose a reason for hiding this comment

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

LGTM but will leave it up to @zroubalik to do a final sweep

zroubalik
zroubalik reviewed Jan 4, 2021
View reviewed changes
Copy link
Member

@zroubalik zroubalik left a comment

Choose a reason for hiding this comment

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

Looking good.

Honestly I'm not 100% sure there's a reason to leave protocol as a configurable thing rather than making it always automatic.

So what if we make the auto protocol a default value and thus protocol an optional parameter? By no means I am an not a RabbitMQ expert, but from what you are saying, it shouldn't do any harm and if anyone wants to specify the protocol, there's still a way.

@coderanger
Copy link
Contributor Author

I did change the default to auto so that is exactly the case now.

@zroubalik
Copy link
Member

Could please rebase this PR?

@coderanger coderanger force-pushed the rabbitmq-auto-protocol branch from 386a2f1 to 66ab9ad Compare January 5, 2021 20:20
@coderanger
Automatically determine the RabbitMQ protocol when possible, and supp…
fd13b8c 
…ort setting the protocl via TriggerAuthentication.

Signed-off-by: Noah Kantrowitz <noah@coderanger.net>
@coderanger coderanger force-pushed the rabbitmq-auto-protocol branch from 66ab9ad to fd13b8c Compare January 5, 2021 20:24
@zroubalik zroubalik merged commit 54f8db3 into kedacore:main Jan 6, 2021
ycabrer pushed a commit to ycabrer/keda that referenced this pull request Mar 1, 2021
@coderanger @zroubalik @ycabrer
Automatically determine the RabbitMQ protocol when possible, and supp…
41b89ed 
…ort setting the protocl via TriggerAuthentication. (kedacore#1459)

Signed-off-by: Noah Kantrowitz <noah@coderanger.net>

Co-authored-by: Zbynek Roubalik <726523+zroubalik@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zroubalik zroubalik zroubalik left review comments

@tomkerkhove tomkerkhove tomkerkhove approved these changes

@ahmelsayed ahmelsayed ahmelsayed approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@coderanger @zroubalik @ahmelsayed @tomkerkhove