-
Notifications
You must be signed in to change notification settings - Fork 138
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 additional AMQP URI query parameters #251
Add support for additional AMQP URI query parameters #251
Conversation
The check that fails was not introduced in this PR. We can safely ignore it for now and address it in a different PR. |
8e0d6fc
to
4835984
Compare
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.
Looks good, I'll merge after CI passes 👍
No problem, already fixed. But I have noticed another issue. When using |
I will fix up this PR, and will check out the functions that @vilius-g mentions. |
We can resolve this by removing the default heartbeat from afore mentioned functions, and performing validation in
We will maintain same behaviour in other functions when URI parameters are not provided, and support URI parameters in those functions. We must keep the previous behaviour, otherwise, it can be a potentially breaking change. |
de8f134
to
9044e89
Compare
The only potential issue I see with this, is that now And there will no longer be a way to disable heartbeat from the client? |
That's a good catch! I usually solve this problem of "is unset or is zero-value?" using a custom type, or a pointer. With the custom type, you can add a function |
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.
@Zerpet, what do you think?
9b20c40
to
70d14d8
Compare
421d09a
to
05a8b67
Compare
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.
This is as good as we can do. We should mention in the function comment of DialConfig
that heartbeat
in the URI takes precedence over heartbeat
in the Config
. If you want to disable heartbeats, set in the URI the heartbeat to 0. I personally don't mind that we make somewhat harder to disable heartbeats, as 99% of environments shouldn't disable them.
https://www.rabbitmq.com/docs/uri-query-parameters specifies several parameters that are used in this library, but not yet supported in URIs. This commit adds support for the following parameters: auth_mechanism heartbeat connection_timeout channel_max Fix default value check when setting SASL authentication from URI Add documentation for added query parameters Add support for additional AMQP URI query parameters https://www.rabbitmq.com/docs/uri-query-parameters specifies several parameters that are used in this library, but not yet supported in URIs. This commit adds support for the following parameters: auth_mechanism heartbeat connection_timeout channel_max Fix default value check when setting SASL authentication from URI Fix ChannelMax type mismatch Use URI heartbeat Bump versions on Windows
05a8b67
to
28ebb24
Compare
In #251, we used `URI.Has()` function, which was introduced in Go 1.17. We also have a dependency on uber-go-leak, which requires Go 1.20. In addition, we only test in "oldstable" and "stable" versions in CI. That means, we test in version N and N-1, where N is the latest available version of Go. Signed-off-by: Aitor Perez Cedres <[email protected]>
RabbitMQ URI Query Parameters specifies several parameters that are used in this library, but not supported in URIs.
This commit adds support for the following parameters:
auth_mechanism
heartbeat
connection_timeout
channel_max