-
Notifications
You must be signed in to change notification settings - Fork 5.1k
Add support for Microsoft.Extensions.Configuration #53527
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
base: main
Are you sure you want to change the base?
Conversation
sdk/core/System.ClientModel/src/Convenience/ConfigurationManagerExtensions.cs
Outdated
Show resolved
Hide resolved
sdk/core/System.ClientModel/src/Convenience/ConfigurationManagerExtensions.cs
Outdated
Show resolved
Hide resolved
sdk/identity/Azure.Identity/src/Credentials/DefaultAzureCredentialOptions.cs
Outdated
Show resolved
Hide resolved
sdk/identity/Azure.Identity/src/Credentials/DefaultAzureCredentialOptions.cs
Outdated
Show resolved
Hide resolved
sdk/resourcemanager/Azure.ResourceManager/src/ClientConnectionExtensions.cs
Outdated
Show resolved
Hide resolved
sdk/core/System.ClientModel/src/Convenience/ConfigurationManagerExtensions.cs
Outdated
Show resolved
Hide resolved
API Change CheckAPIView identified API level changes in this PR and created the following API reviews Azure.Core |
| } | ||
| public static partial class ConfigurationManagerExtensions | ||
| { | ||
| public static System.ClientModel.Primitives.ClientConnection GetConnection(this Microsoft.Extensions.Configuration.IConfigurationManager configuration, string sectionName) { throw null; } |
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.
since these apis are in SCM, should these be just static Create methods on ClientConnection, or maybe even ClientConnection ctor? This way we don't need to add a new type (the config extensions)
| /// <param name="configuration"></param> | ||
| /// <param name="sectionName"></param> | ||
| /// <returns></returns> | ||
| public static ClientConnection GetConnection(this IConfigurationManager configuration, string sectionName) |
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 wonder if we dont need an optional parameter called "format" (or something like that). This would allow us to add support for all the combinations in aspire. Unless we are fine with probing all the formats.
| /// <summary> | ||
| /// . | ||
| /// </summary> | ||
| public string? CredentialSource { get; set; } |
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.
Should we name this CredentialKind or is CredentialSource already established somewhere else?
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.
Its established in Aspire and we would like to reuse that name / enumeration if possible.
| /// <summary> | ||
| /// . | ||
| /// </summary> | ||
| public IConfigurationSection? Properties { get; set; } |
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.
should we name this ConfigurationSection?
| /// <summary> | ||
| /// . | ||
| /// </summary> | ||
| public CredentialSettings? Credential { get; set; } |
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.
should we name this CredentialSettings?
|
/azp run net - pullrequest |
|
Azure Pipelines failed to run 1 pipeline(s). |
|
@mokarchi This is the draft PR we are working on to work out how configuration / di can work with our non azure sdks that depend on System.ClientModel and branded sdks that depend on Azure.Core and use Azure.Identity for credentials. Given you are working on the openai PR wanted to make you aware in case you have some comments / suggestions as well. |
| public static System.ClientModel.ContinuationToken FromBytes(System.BinaryData bytes) { throw null; } | ||
| public virtual System.BinaryData ToBytes() { throw null; } | ||
| } | ||
| public partial class CredentialSettings |
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.
@christothes, do we need to let people configure the tenentid?
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.
Yes, this should be possible.
| protected virtual void Wait(System.TimeSpan time, System.Threading.CancellationToken cancellationToken) { } | ||
| protected virtual System.Threading.Tasks.Task WaitAsync(System.TimeSpan time, System.Threading.CancellationToken cancellationToken) { throw null; } | ||
| } | ||
| public abstract partial class ClientSettingsBase |
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.
why do we need this base class? Why si ClientSettings not enough?
I would really like to avoid it. If we cannot, we need to come up with a better name than simply suffixing it with "Base"
| protected sealed override void ProcessCore(System.ClientModel.Primitives.PipelineMessage message) { } | ||
| protected sealed override System.Threading.Tasks.ValueTask ProcessCoreAsync(System.ClientModel.Primitives.PipelineMessage message) { throw null; } | ||
| } | ||
| public partial interface IClientBuilder : Microsoft.Extensions.Hosting.IHostApplicationBuilder |
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.
do we really need it?
Thanks for the heads-up! @m-nash |
Initial prototype to support Microsoft.Extensions.Configuration and Microsoft.Extensions.DependencyInjection
We don't intend to support both ClientConnection and ClientSettings we are exploring both and will choose one.
The example below shows using ApiKey credential for public openai client and azure cli credential for azure openai client. If you are using ApiKey credential for both the
WithAzureCredentialorGetAzureConnectionis not necessary the code for public openai works for both.OpenAIClient appsettings.json
AzureOpenAIClient-Cli appsettings.json
Sample usage with configuration only using
ClientConnectionSample usage with configuration only using new
ClientSettingsSample usage with dependency injection