Switch from Environment.GetEnvironmentVariable to IConfiguration#1704
Switch from Environment.GetEnvironmentVariable to IConfiguration#1704srnagar wants to merge 3 commits intomicrosoft:mainfrom
Conversation
srnagar
changed the title
| var env = Environment.GetEnvironmentVariables() | ||
| .Cast<System.Collections.DictionaryEntry>() | ||
| .ToDictionary(e => (string)e.Key, e => (string?)e.Value); | ||
| // Merge current configuration values (includes environment variables) with serverInfo.Env (serverInfo.Env overrides) |
There was a problem hiding this comment.
IConfiguration comprises of a lot more than just environment variables. Also includes things like appsettings.json, other configuration providers. Is this what you want?
There was a problem hiding this comment.
I think the intent here is for serverInfo.Env to take precedence over everything else. So, yes, this should still work as expected.
There was a problem hiding this comment.
I would also argue that it may not be appropriate for azmcp's json config to appear as transitive config at the environment variable level for another server we start.
If environment variables have higher priority than json config. We'd be elevating azmcp's json config over the other server's json config
| .Returns(Substitute.For<HttpClient>()); | ||
| return new RegistryServerProvider(id, serverInfo, httpClientFactory); | ||
| var configuration = new ConfigurationBuilder().AddEnvironmentVariables().Build(); | ||
| return new RegistryServerProvider(id, serverInfo, httpClientFactory, configuration); |
There was a problem hiding this comment.
I'm 100% behind replacing our environment variable pulls with DI, but passing IConfiguration into the business layer feels like an antipattern. It's hiding the real dependency and letting some deep logic decide that it now cares about a new json or environment variable config without "informing" the startup.
I'd much prefer we use the options pattern over raw IConfiguration.
There was a problem hiding this comment.
Program.cs and ServiceCollection/Provider would be responsible for converting the state of the world at program start into strongly typed options classes. Everything below program.cs would deal with strongly typed POCOs.
We would just need to decide how coupled our option classes would be. We could have:
RegistryServerProvider depend on IOptions<RegistryServerProviderOptions>
Or we could have a more aggregate options class that serves any of our internal classes that needed config:
RegistryServerProvider depends on IOptions<AzureMcpServerOptions>
There was a problem hiding this comment.
Or we could punt on IOptions and just replace the non-mockable Environment.GetEnvironmentVariable with IEnvironmentVariables.Get
| // Merge current configuration values (includes environment variables) with serverInfo.Env (serverInfo.Env overrides) | ||
| var env = _configuration.AsEnumerable() | ||
| .Where(kvp => kvp.Value is not null) | ||
| .ToDictionary(kvp => kvp.Key, kvp => (string?)kvp.Value); |
There was a problem hiding this comment.
I'm not sure if this would round trip correctly. While IConfig and environment variables are both string/string maps, their key serialization scheme is different.
When IConfiguration is built from environment variables, it expects a hierarchy delimiter of __ or _, but that's converted to IConfig's normal hierarchy delimiter of : internally.
To round trip, you'd have to convert all : in each key to __
The : delimiter doesn't work with environment variable hierarchical keys on all platforms. For example, the : delimiter is not supported by Bash. The double underscore (__), which is supported on all platforms, automatically replaces any : delimiters in environment variables.
3 participants
…Configuration
What does this PR do?
[Provide a clear, concise description of the changes][Any additional context, screenshots, or information that helps reviewers]GitHub issue number?
[Link to the GitHub issue this PR addresses]Pre-merge Checklist
servers/Azure.Mcp.Server/CHANGELOG.mdand/orservers/Fabric.Mcp.Server/CHANGELOG.mdfor product changes (features, bug fixes, UI/UX, updated dependencies)servers/Azure.Mcp.Server/README.mdand/orservers/Fabric.Mcp.Server/README.mddocumentationeng/scripts/Process-PackageReadMe.ps1. See Package README/servers/Azure.Mcp.Server/docs/azmcp-commands.mdand/or/docs/fabric-commands.md.\eng\scripts\Update-AzCommandsMetadata.ps1to update tool metadata in azmcp-commands.md (required for CI)ToolDescriptionEvaluatorand obtained a score of0.4or more and a top 3 ranking for all related test promptsconsolidated-tools.json/servers/Azure.Mcp.Server/docs/e2eTestPrompts.mdcrypto mining, spam, data exfiltration, etc.)/azp run mcp - pullrequest - liveto run Live Test Pipeline