Skip to content

Switch from Environment.GetEnvironmentVariable to IConfiguration #1704

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.

Sign up for GitHub

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

Draft
srnagar wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

Switch from Environment.GetEnvironmentVariable to IConfiguration #1704

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6da0680
initial commit to switch from Environment.GetEnvironmentVariable to I…
srnagar Feb 11, 2026
9634ca6
fix test
srnagar Feb 11, 2026
c72f0e4
fix formatting
srnagar Feb 12, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions core/Azure.Mcp.Core/tests/Azure.Mcp.Core.UnitTests/Areas/Server/CommandFactoryHelpers.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
using Azure.Mcp.Tools.Storage;
using Azure.Mcp.Tools.VirtualDesktop;
using Azure.Mcp.Tools.Workbooks;
using Microsoft.Extensions.Configuration;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Logging;
using Microsoft.Mcp.Core.Areas;
Expand Down Expand Up @@ -160,6 +161,7 @@ public static IServiceCollection SetupCommonServices()

var builder = new ServiceCollection()
.AddLogging()
.AddSingleton<IConfiguration>(new ConfigurationBuilder().AddEnvironmentVariables().Build())
.AddSingleton<ITelemetryService, NoOpTelemetryService>();

foreach (var area in areaSetups)
Expand Down
4 changes: 3 additions & 1 deletion ...s/Azure.Mcp.Core.UnitTests/Areas/Server/Commands/Discovery/RegistryServerProviderTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
using System.Net.Sockets;
using Azure.Mcp.Core.Areas.Server.Commands.Discovery;
using Azure.Mcp.Core.Areas.Server.Models;
using Microsoft.Extensions.Configuration;
using ModelContextProtocol.Client;
using NSubstitute;
using Xunit;
Expand All @@ -18,7 +19,8 @@ private static RegistryServerProvider CreateServerProvider(string id, RegistrySe
var httpClientFactory = Substitute.For<IHttpClientFactory>();
httpClientFactory.CreateClient(Arg.Any<string>())
.Returns(Substitute.For<HttpClient>());
return new RegistryServerProvider(id, serverInfo, httpClientFactory);
var configuration = new ConfigurationBuilder().AddEnvironmentVariables().Build();
return new RegistryServerProvider(id, serverInfo, httpClientFactory, configuration);
Copy link
Member

@hallipr hallipr Feb 12, 2026

Choose a reason for hiding this comment

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

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.

Copy link
Member

@hallipr hallipr Feb 13, 2026

Choose a reason for hiding this comment

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

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>

Copy link
Member

@hallipr hallipr Feb 13, 2026

Choose a reason for hiding this comment

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

Or we could punt on IOptions and just replace the non-mockable Environment.GetEnvironmentVariable with IEnvironmentVariables.Get

}
[Fact]
public void Constructor_InitializesCorrectly()
Expand Down
4 changes: 3 additions & 1 deletion .../Azure.Mcp.Core.UnitTests/Areas/Server/Commands/ToolLoading/SingleProxyToolLoaderTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
using Azure.Mcp.Core.Areas.Server.Commands.ToolLoading;
using Azure.Mcp.Core.Areas.Server.Options;
using Azure.Mcp.Core.Helpers;
using Microsoft.Extensions.Configuration;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Logging;
using ModelContextProtocol.Protocol;
Expand All @@ -21,7 +22,8 @@ private static RegistryDiscoveryStrategy CreateStrategy(ServiceStartOptions opti
var serviceOptions = Microsoft.Extensions.Options.Options.Create(options ?? new ServiceStartOptions());
var httpClientFactory = Substitute.For<IHttpClientFactory>();
var registryRoot = RegistryServerHelper.GetRegistryRoot();
return new RegistryDiscoveryStrategy(serviceOptions, logger, httpClientFactory, registryRoot!);
var configuration = new ConfigurationBuilder().AddEnvironmentVariables().Build();
return new RegistryDiscoveryStrategy(serviceOptions, logger, httpClientFactory, registryRoot!, configuration);
}

private static (SingleProxyToolLoader toolLoader, IMcpDiscoveryStrategy discoveryStrategy) CreateToolLoader(bool useRealDiscovery = true)
Expand Down
4 changes: 3 additions & 1 deletion core/Azure.Mcp.Core/tests/Azure.Mcp.Core.UnitTests/RegistryDiscoveryStrategyHelper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
using Azure.Mcp.Core.Areas.Server.Commands.Discovery;
using Azure.Mcp.Core.Areas.Server.Options;
using Azure.Mcp.Core.Helpers;
using Microsoft.Extensions.Configuration;
using Microsoft.Extensions.Logging;

namespace Azure.Mcp.Core.UnitTests;
Expand All @@ -16,6 +17,7 @@ public static RegistryDiscoveryStrategy CreateStrategy(ServiceStartOptions? opti
logger = logger ?? NSubstitute.Substitute.For<Microsoft.Extensions.Logging.ILogger<RegistryDiscoveryStrategy>>();
var httpClientFactory = NSubstitute.Substitute.For<IHttpClientFactory>();
var registryRoot = RegistryServerHelper.GetRegistryRoot();
return new RegistryDiscoveryStrategy(serviceOptions, logger, httpClientFactory, registryRoot!);
var configuration = new ConfigurationBuilder().AddEnvironmentVariables().Build();
return new RegistryDiscoveryStrategy(serviceOptions, logger, httpClientFactory, registryRoot!, configuration);
}
}
6 changes: 4 additions & 2 deletions core/Microsoft.Mcp.Core/src/Areas/Server/Commands/Discovery/RegistryDiscoveryStrategy.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

using Azure.Mcp.Core.Areas.Server.Models;
using Azure.Mcp.Core.Areas.Server.Options;
using Microsoft.Extensions.Configuration;
using Microsoft.Extensions.Logging;
using Microsoft.Extensions.Options;

Expand All @@ -16,10 +17,11 @@ namespace Azure.Mcp.Core.Areas.Server.Commands.Discovery;
/// <param name="logger">Logger instance for this discovery strategy.</param>
/// <param name="httpClientFactory">Factory that can create HttpClient objects.</param>
/// <param name="registryRoot">Manifest of all the MCP server registries.</param>
public sealed class RegistryDiscoveryStrategy(IOptions<ServiceStartOptions> options, ILogger<RegistryDiscoveryStrategy> logger, IHttpClientFactory httpClientFactory, IRegistryRoot registryRoot) : BaseDiscoveryStrategy(logger)
public sealed class RegistryDiscoveryStrategy(IOptions<ServiceStartOptions> options, ILogger<RegistryDiscoveryStrategy> logger, IHttpClientFactory httpClientFactory, IRegistryRoot registryRoot, IConfiguration configuration) : BaseDiscoveryStrategy(logger)
{
private readonly IOptions<ServiceStartOptions> _options = options;
private readonly IHttpClientFactory _httpClientFactory = httpClientFactory;
private readonly IConfiguration _configuration = configuration;

/// <inheritdoc/>
public override async Task<IEnumerable<IMcpServerProvider>> DiscoverServersAsync(CancellationToken cancellationToken)
Expand All @@ -34,7 +36,7 @@ public override async Task<IEnumerable<IMcpServerProvider>> DiscoverServersAsync
.Where(s => _options.Value.Namespace == null ||
_options.Value.Namespace.Length == 0 ||
_options.Value.Namespace.Contains(s.Key, StringComparer.OrdinalIgnoreCase))
.Select(s => new RegistryServerProvider(s.Key, s.Value, _httpClientFactory))
.Select(s => new RegistryServerProvider(s.Key, s.Value, _httpClientFactory, _configuration))
.Cast<IMcpServerProvider>();
}
}
12 changes: 7 additions & 5 deletions core/Microsoft.Mcp.Core/src/Areas/Server/Commands/Discovery/RegistryServerProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

using Azure.Mcp.Core.Areas.Server.Models;
using Azure.Mcp.Core.Helpers;
using Microsoft.Extensions.Configuration;
using ModelContextProtocol.Client;

namespace Azure.Mcp.Core.Areas.Server.Commands.Discovery;
Expand All @@ -15,11 +16,12 @@ namespace Azure.Mcp.Core.Areas.Server.Commands.Discovery;
/// <param name="serverInfo">Configuration information for the server.</param>
/// <param name="httpClientFactory">Factory for creating HTTP clients.</param>
/// <param name="tokenCredentialProvider">The token credential provider for OAuth authentication.</param>
public sealed class RegistryServerProvider(string id, RegistryServerInfo serverInfo, IHttpClientFactory httpClientFactory) : IMcpServerProvider
public sealed class RegistryServerProvider(string id, RegistryServerInfo serverInfo, IHttpClientFactory httpClientFactory, IConfiguration configuration) : IMcpServerProvider
{
private readonly string _id = id;
private readonly RegistryServerInfo _serverInfo = serverInfo;
private readonly IHttpClientFactory _httpClientFactory = httpClientFactory;
private readonly IConfiguration _configuration = configuration;

/// <summary>
/// Creates metadata that describes this registry-based server.
Expand Down Expand Up @@ -126,10 +128,10 @@ private async Task<McpClient> CreateStdioClientAsync(McpClientOptions clientOpti
throw new InvalidOperationException($"Registry server '{_id}' does not have a valid command for stdio transport.");
}

// Merge current system environment variables with serverInfo.Env (serverInfo.Env overrides system)
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)
Copy link
Member

@conniey conniey Feb 12, 2026

Choose a reason for hiding this comment

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

IConfiguration comprises of a lot more than just environment variables. Also includes things like appsettings.json, other configuration providers. Is this what you want?

Copy link
Member Author

@srnagar srnagar Feb 12, 2026

Choose a reason for hiding this comment

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

I think the intent here is for serverInfo.Env to take precedence over everything else. So, yes, this should still work as expected.

Copy link
Member

@hallipr hallipr Feb 13, 2026

Choose a reason for hiding this comment

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

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

var env = _configuration.AsEnumerable()
.Where(kvp => kvp.Value is not null)
.ToDictionary(kvp => kvp.Key, kvp => (string?)kvp.Value);
Copy link
Member

@hallipr hallipr Feb 13, 2026

Choose a reason for hiding this comment

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

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 __

https://learn.microsoft.com/en-us/dotnet/core/extensions/configuration-providers#environment-variable-configuration-provider

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.


if (_serverInfo.Env != null)
{
Expand Down
8 changes: 5 additions & 3 deletions tools/Azure.Mcp.Tools.Extension/src/Commands/AzCommand.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,16 +8,18 @@
using Azure.Mcp.Core.Services.Azure.Authentication;
using Azure.Mcp.Core.Services.ProcessExecution;
using Azure.Mcp.Tools.Extension.Options;
using Microsoft.Extensions.Configuration;
using Microsoft.Extensions.Logging;
using Microsoft.Mcp.Core.Commands;
using Microsoft.Mcp.Core.Models.Command;

namespace Azure.Mcp.Tools.Extension.Commands;

public sealed class AzCommand(ILogger<AzCommand> logger, int processTimeoutSeconds = 300) : GlobalCommand<AzOptions>()
public sealed class AzCommand(ILogger<AzCommand> logger, IConfiguration configuration, int processTimeoutSeconds = 300) : GlobalCommand<AzOptions>()
{
private const string CommandTitle = "Azure CLI Command";
private readonly ILogger<AzCommand> _logger = logger;
private readonly IConfiguration _configuration = configuration;
private readonly int _processTimeoutSeconds = processTimeoutSeconds;
private static string? _cachedAzPath;
private volatile bool _isAuthenticated = false;
Expand Down Expand Up @@ -65,7 +67,7 @@ protected override AzOptions BindOptions(ParseResult parseResult)
return options;
}

internal static string? FindAzCliPath()
internal string? FindAzCliPath()
{
string executableName = "az";

Expand All @@ -75,7 +77,7 @@ protected override AzOptions BindOptions(ParseResult parseResult)
return _cachedAzPath;
}

var pathEnv = Environment.GetEnvironmentVariable("PATH");
var pathEnv = _configuration["PATH"];
if (string.IsNullOrEmpty(pathEnv))
return null;

Expand Down