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

Core AMQP cleanup for v2 #12210

Merged
merged 5 commits into from
Nov 2, 2020
Merged

Conversation

ramya-rao-a
Copy link
Contributor

@ramya-rao-a ramya-rao-a commented Oct 31, 2020

This PR addresses the points in #12116 (comment) to clean the public API surface of @azure/core-amqp package.

Context: @azure/amqp-common is the package with common code for Event Hubs v2 and Service Bus v1 packages. When we started working on Event Hubs v5 based on the new guidelines, we copied this package, named it @azure/core-amqp, added TokenCredential support and made it easier to have retries. What we missed was doing a cleanup of things that we don't really need exported from here.

This PR does the said cleanup as follows:

  • Stop exporting things that are not currently used by either Service Bus or Event Hubs packages
    • AsyncLockOptions
    • executePromisesSequentially
    • Func
    • getNewAsyncLock
    • isNode
    • randomNumberFromInterval
    • Timeout
  • Remove all IotHub related artifacts. These existed to support the IotHub support we had in Event Hubs v2 which has since been removed in Event Hubs v5 for a better separation of concerns
    • IotHubConnectionConfig
    • IotHubConnectionStringModel
    • IotSharedKeyCredential
    • isIotHubConnectionString
  • With Implement a Service Bus Shared Access Key Credential #11891 and Implement an Event Hubs Shared Access Key Credential #11892, both Service Bus and Event Hubs packages will have dedicated SharedKeyCredential classes with the class name reflecting the service so that one doesnt see a generic exported class and attempt to use it with other services. As part of this I am
    • removing the SharedKeyCredential class & its tests from @azure/core-amqp and copying them to Service Bus and Event Hubs packages
    • remove credential from the ConnectionContextBase in @azure/core-amqp and have the ConnectionContext in SB & EH packages house it
    • update negotiateClaim() to take token string instead of the AccessToken. With this, @azure/core-amqp need not re-export things from @azure/core-auth
  • Avoid having anything that is specific to Event Hubs or Service Bus as a service in @azure/core-amqp
  • Avoid re-exporting things from rhea-promise. EH & SB already have a dependency on this and can use things from it directly

EH live tests and SB live tests have been run


We are cleaning the public API surface by

- removing exports that are either not used by either `@azure/event-hubs` and `@azure/service-bus` packages (which are the two main consumers of this package)
Copy link
Member

Choose a reason for hiding this comment

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

NIT

Suggested change
- removing exports that are either not used by either `@azure/event-hubs` and `@azure/service-bus` packages (which are the two main consumers of this package)
- removing exports that are neither used by `@azure/event-hubs` nor `@azure/service-bus` package (which are the two main consumers of this package)

Copy link
Member

@richardpark-msft richardpark-msft left a comment

Choose a reason for hiding this comment

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

Looks great, core-amqp feels much healthier.

@ramya-rao-a ramya-rao-a merged commit 54247fd into Azure:master Nov 2, 2020
@ramya-rao-a ramya-rao-a deleted the cleaner-core-amqp branch November 2, 2020 19:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants