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

Implement new transport seam #221

Merged
merged 42 commits into from
Feb 5, 2021
Merged

Implement new transport seam #221

merged 42 commits into from
Feb 5, 2021

Conversation

timbussmann
Copy link
Contributor

@timbussmann timbussmann commented Jan 8, 2021

Implements the new transport seam for ASB

# Conflicts:
#	src/AcceptanceTests/NServiceBus.Transport.AzureServiceBus.AcceptanceTests.csproj
#	src/Tests/NServiceBus.Transport.AzureServiceBus.Tests.csproj
#	src/Transport/Administration/QueueCreator.cs
#	src/Transport/Administration/SubscriptionManager.cs
#	src/Transport/NServiceBus.Transport.AzureServiceBus.csproj
#	src/Transport/Sending/MessageDispatcher.cs
#	src/TransportTests/ConfigureAzureServiceBusTransportInfrastructure.cs
#	src/TransportTests/NServiceBus.Transport.AzureServiceBus.TransportTests.csproj
@timbussmann timbussmann marked this pull request as ready for review January 25, 2021 15:58
@timbussmann
Copy link
Contributor Author

timbussmann commented Jan 25, 2021

all tests are green now and it's targeting the latest version of core.

@SeanFeldman do you have some chance to have a look at this, especially on the changes to the outgoing message customization that were necessary?

@SeanFeldman
Copy link
Contributor

@timbussmann, since the change to the native message customizations, is done, would it make sense to address #218 here as well?

Copy link
Contributor Author

@timbussmann timbussmann left a comment

Choose a reason for hiding this comment

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

review comments from review with @SeanFeldman

@timbussmann
Copy link
Contributor Author

addressed the review comments

Copy link
Member

@SzymonPobiega SzymonPobiega left a comment

Choose a reason for hiding this comment

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

One minor comment, nitpicking.

Guard.AgainstNull(nameof(SubscriptionNamingConvention), value);

// wrap the custom convention:
subscriptionNamingConvention = subscriptionName =>
Copy link
Member

Choose a reason for hiding this comment

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

Should we do the wrapping when we actually use it? Otherwise we might (although I know it might a synthetic case) with funny behavior when people do

config.SubscriptionNamingConvention = config.SubscriptionNamingConvention

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that's a good point. In that case I'd rather vote to just remove the wrapper completely anyway though. Because once we call the convention, we don't really know anymore whether this is the default convention or a user-provided convention. So I'd say we leave it for now as is and if this is causing issues, we should look at just removing the wrapper.

@timbussmann timbussmann merged commit 8ee8e08 into master Feb 5, 2021
@timbussmann timbussmann deleted the new-transport-seam branch February 5, 2021 10:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants