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

Allow custom connection string names when creating a DurableClient in an ASP.NET Core app #1895

Merged
merged 5 commits into from
Jul 27, 2021

Conversation

bachuv
Copy link
Collaborator

@bachuv bachuv commented Jul 22, 2021

This PR allows customers to use custom app setting names to create a DurableClient in an ASP.NET Core app.

_client = clientFactory.CreateClient(new DurableClientOptions
          {
              ConnectionName = "Storage",
              TaskHub = configuration["TaskHub"]
          });

Currently, customers have to set ConnectionName to "Storage" in DurableClientOptions because of this line in AzureStorageDurabilityProviderFactory.cs. connectionName is always set to null which means that it's always set to the this.defaultConnectionName. this.defaultConnectionName defaults to using ConnectionStringNames.Storage which is "Storage" because this.azureStorageOptions.ConnectionStringName` is null in this flow.

This PR passes the ConnectionName and TaskHub name from DurableClientOptions to GetAzureStorageOrchestrationServiceSettings() to create an AzureStorageDurabilityProvider using the configured values instead of default values.

Resolves #1656

Pull request checklist

  • My changes do not require documentation changes
    • Otherwise: Documentation PR is ready to merge and referenced in pending_docs.md
  • My changes should not be added to the release notes for the next release
    • Otherwise: I've added my notes to release_notes.md
  • My changes do not need to be backported to a previous version
    • Otherwise: Backport tracked by issue/PR #issue_or_pr
  • I have added all required tests (Unit tests, E2E tests)

@bachuv bachuv added this to the v2.5.1 milestone Jul 22, 2021
@bachuv bachuv requested a review from ConnorMcMahon July 22, 2021 16:34
@bachuv bachuv self-assigned this Jul 22, 2021
Copy link
Contributor

@ConnorMcMahon ConnorMcMahon left a comment

Choose a reason for hiding this comment

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

I think this is very close.

I have a couple of minor production code suggestions, and a few questions/suggestions about the tests.

test/Common/ClientFunctions.cs Outdated Show resolved Hide resolved
@@ -41,7 +41,7 @@ public static IServiceCollection AddDurableClientFactory(this IServiceCollection
}

serviceCollection.TryAddSingleton<INameResolver, DefaultNameResolver>();
serviceCollection.TryAddSingleton<IConnectionStringResolver, StandardConnectionStringProvider>();
serviceCollection.TryAddSingleton<IConnectionStringResolver, WebJobsConnectionStringProvider>();
Copy link
Contributor

Choose a reason for hiding this comment

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

I am a little concerned about changing this, just because some customers will be running in non-webjobs scenario.

Copy link
Collaborator Author

@bachuv bachuv Jul 24, 2021

Choose a reason for hiding this comment

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

I initially changed it because of this comment in the linked issue:

In addition when consuming this service via non web job (standard aspnet application) the extention method defaults to using the implementation of IConnectionStringProvider as StandardConnectionStringProvider.

After looking the source code it appears that the standard implementation would be better suited using WebJobsConnectionStringProvider this will locate connection string via both local.settings.json:Values:ConnString in webjob and appsettings.json:ConnectionStrings:ConnString in standard ASP.NET application and therefore feels like the naming convention of these implementations are reversed.

I can change it back to StandardConnectionStringProvider. I looked through StandardConnectionStringProvider and WebJobsConnectionStringProvider to understand this comment and the only difference I see is that WebJobsConnectionStringProvider evaluates configuration.GetConnectionString(connectionName) first before configuration[connectionName]. I'm not exactly clear on what configuration.GetConnectionString(connectionName) does/where it looks for connection strings. Let me know if you think WebJobsConnectionStringProvider is fine otherwise I'll change it to StandardConnectionStringProvider

return this.hostConfiguration.GetWebJobsConnectionString(connectionStringName);

https://github.com/Azure/azure-webjobs-sdk/blob/4a16c273b40ee47ec318bb6d7aa12e0075f30402/src/Microsoft.Azure.WebJobs.Host/Extensions/IConfigurationExtensions.cs#L79

@bachuv bachuv marked this pull request as ready for review July 26, 2021 21:06
@bachuv bachuv requested review from amdeel and davidmrdavid July 26, 2021 21:06
Copy link
Contributor

@ConnorMcMahon ConnorMcMahon left a comment

Choose a reason for hiding this comment

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

LGTM!

@bachuv bachuv merged commit aa88b8c into dev Jul 27, 2021
@bachuv bachuv deleted the vabachu/durableclientconnectionstring branch July 27, 2021 00:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants