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
[ServiceBus] Helper function to parse connection string #11949
[ServiceBus] Helper function to parse connection string #11949
Conversation
sdk/servicebus/service-bus/test/internal/connectionString.spec.ts
Outdated
Show resolved
Hide resolved
/azp run js - servicebus - tests |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run js - servicebus - tests |
Azure Pipelines successfully started running 1 pipeline(s). |
export function parseServiceBusConnectionString( | ||
connectionString: string | ||
): ServiceBusConnectionStringProperties { | ||
const parsedResult = parseConnectionString<{ |
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.
Nice! It's good to see that you preserve camelCasing ;)
} | ||
|
||
const output: ServiceBusConnectionStringProperties = { | ||
fullyQualifiedNamespace: (parsedResult.Endpoint.match(".*://([^/]*)") || [])[1], |
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.
I feel like a line could not get more Javascript-y than this :)
@@ -316,7 +318,7 @@ describe("Atom management - Authentication", function(): void { | |||
); | |||
should.equal( | |||
(await serviceBusAdministrationClient.getNamespaceProperties()).name, | |||
host.match("(.*).servicebus.windows.net")[1], | |||
(host.match("(.*).servicebus.windows.net") || [])[1], |
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.
Is this going to work when we get to the "private" clouds?
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.
No, it wont. But since our tests do not run in private clouds, we should be good. We should make sure we dont do this in any of our library code though
it("Extracts Service Bus properties with Endpoint & SharedKey", () => { | ||
const connectionString = `Endpoint=${expectedEndpoint};SharedAccessKey=${expectedSharedKey};SharedAccessKeyName=${expectedSharedKeyName}`; | ||
const connectionStringProperties = parseServiceBusConnectionString(connectionString); | ||
assert.equal(connectionStringProperties.endpoint, expectedEndpoint); |
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.
Not a change you have to make.
I've been doing assert.deepEqual({ object }, expected) - it has nice output when the assertion fails and you get to see all the fields, not just the one that failed to match. Can be helpful sometimes if you're trying to figure out how badly you "missed" the right answer.
// const tokenProvider = new TestTokenCredential(); | ||
// receiver = new ServiceBusReceiverClient( | ||
// { | ||
// host: connectionObject.Endpoint.substr(5), | ||
// host: connectionObject.endpoint.substr(5), |
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.
Your thoroughness knows no bounds. :)
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.
Got some comments, they're really NITs.
This is great, I'm glad you were able to make it very precise (I think that was only real issue with the other connection string PR).
Co-authored-by: Richard Park <51494936+richardpark-msft@users.noreply.github.com>
Update DeploymentScripts swagger to address swagger correctness issues (Azure#11949)
Update DeploymentScripts swagger to address swagger correctness issues (Azure#11949)
Update DeploymentScripts swagger to address swagger correctness issues (Azure#11949)
Update DeploymentScripts swagger to address swagger correctness issues (Azure#11949)
Update DeploymentScripts swagger to address swagger correctness issues (Azure#11949)
Update DeploymentScripts swagger to address swagger correctness issues (Azure#11949)
## What Add the following to the Event Hubs package: - A helper methods `parseEventHubConnectionString` that validates and parses a given connection string - An interface `EventHubConnectionStringProperties` that defines the output of the above method. ## Why In a [recent PR](#11949) the same change has been made for ServiceBus to provide a parsing utility that can help transform a raw connection string for use with credential-based client creation. Implements #11894
This PR adds the below to the Service Bus package to satisfy the requirements listed in #11893
parseServiceBusConnectionString
that validates and parses the given connection stringServiceBusConnectionStringProperties
that defines the output of the above methodThis has not been added to
@azure/core-amqp
intentionally. The core package already has a generic parseConnectionString() method which has limitations when compared to the requirements we have (see #11909 for details).