feat(core-amqp): Add support for specifying the Port in the connection string #32091
Conversation
kf6kjg
requested review from
jeremymeng,
deyaaeldeen,
HarshaNalluru and
a team
as code owners
github-actions
bot
added
Azure.Core
Community Contribution
|
Thank you for your contribution @kf6kjg! We will review the pull request and get back to you soon. |
kf6kjg
force-pushed
the
feature/amqp-connstr-port
branch
from
22ded92 to
9967540
Compare
|
API change check API changes are not detected in this pull request. |
kf6kjg
force-pushed
the
feature/amqp-connstr-port
branch
2 times, most recently
from
c279abe to
a3ed496
Compare
…onnection tests Using the `sb::/h:port` Endpoint syntax is invalid, the underlying connection socket library doesn't support specifying the port in the host field. https://nodejs.org/api/net.html#socketconnectport-host-connectlistener ``` ServiceBusError: ENOTFOUND: getaddrinfo ENOTFOUND localhost:5681 ```
kf6kjg
force-pushed
the
feature/amqp-connstr-port
branch
2 times, most recently
from
8a1e4ce to
c441f96
Compare
…nnection string Before this it was impossible to run multiple AMQP-based service emulators on the same host with different port mappings, e.g. by running each emulator in a Docker container with the internal 5672 port mapped to an arbitrary host port. Now with the ability to specify the port we can create such a set up.
kf6kjg
force-pushed
the
feature/amqp-connstr-port
branch
from
c441f96 to
b5f5959
Compare
|
Thanks for opening the PR @kf6kjg! I would expect the format of |
|
@jeremymeng in order to support that syntax the Endpoint value would need to be parsed and the port placed in the appropriate field of the resulting object. As it stands it's being sent to the host field of the call to net.connect, and thus failing. |
|
@kf6kjg We just released |
|
@jeremymeng I'll give it a test soon. I'm personally not fond that there's two patterns in use with connection strings and ports. Some MS projects do as I did here and place the port in it own field, some do as the recent update and place it in the endpoint... I chose the one I did because it seemed to match the ethos I thought was more common and hoped it would be easier to accept the PR that way. |
Understandable. Yes, we have existing pattern with |
|
@kf6kjg btw were you able to run two emulators at the same time without one interfering the other? When I tested it, the second container instance would clear out the sqlege container database and causing problems. |
|
I don't think any of our tests push that far into the services. |
Packages impacted by this PR
core-amqp, and as a consequence all packages that depend upon that library, including:eventhubservicebusIssues associated with this PR
None that I could find.
Describe the problem that is addressed by this PR
Before this it was impossible to run multiple AMQP-based service emulators on the same host with different port mappings, e.g. by running each emulator in a Docker container with the internal 5672 port mapped to an arbitrary host port. Now with the ability to specify the port we can create such a set up.
What are the possible designs available to address the problem? If there are more than one possible design, why was the one in this PR chosen?
I could have placed the port in the
Endpointentry, e.g.Endpoint=sb://localhost:5681;, but looking across the limited Microsoft documentation I have access to I found that other implementations of the connection string format use a separatePortentry e.g.Endpoint=sb://localhost;Port=5681;so I went with that pattern.The existing tests attempted to demonstrate using the colon syntax, but the placing the port in the host field of the Node Socket
connectfunction isn't valid and was the result of what was here before. Simply placing the port in the standard location for a connection string solves the problem. Could more code be added to support colon syntax? Yes, but not my goal here.Are there test cases added in this PR?
Yes! :D
Provide a list of related PRs (if any)
None that I'm aware of, even after searching.
Checklists