diff --git a/code-reviews/DeploymentManager/findings.md b/code-reviews/DeploymentManager/findings.md index 57a88d0..c906410 100644 --- a/code-reviews/DeploymentManager/findings.md +++ b/code-reviews/DeploymentManager/findings.md @@ -402,18 +402,20 @@ Add an `IConfiguration` parameter (or a configure callback) to **Resolution** -Resolved 2026-05-16 (commit pending): added an -`AddDeploymentManager(IServiceCollection, IConfiguration)` overload that binds -`DeploymentManagerOptions` to the `ScadaLink:DeploymentManager` configuration -section (exposed as `ServiceCollectionExtensions.OptionsSection`). The -`Microsoft.Extensions.Options.ConfigurationExtensions` package was added to the -project. The original parameterless overload is retained for callers/tests that -do not bind configuration. Regression tests: -`AddDeploymentManager_WithConfiguration_BindsDeploymentManagerOptions`, -`AddDeploymentManager_WithConfiguration_MissingSection_UsesDefaults`. Note: a -one-line follow-up in `Host/Program.cs` (call the new overload with -`builder.Configuration`) is required to take effect at runtime — that file is -outside this module's edit scope and is surfaced for the Host owner. +Resolved 2026-05-16 (commit pending): `AddDeploymentManager()` now calls +`services.AddOptions()` so `IOptions` +is always resolvable, and `Host/Program.cs` binds the +`ScadaLink:DeploymentManager` section (exposed as +`ServiceCollectionExtensions.OptionsSection`) via +`services.Configure(...)` — the same pattern the Host +uses for `SecurityOptions`/`InboundApiOptions`. An earlier attempt added an +`AddDeploymentManager(IConfiguration)` overload; that was reverted because the +project convention (enforced by `Host.Tests.OptionsTests`) forbids component +`Add*` methods from depending on `IConfiguration` — the Host owns +configuration binding. Regression tests: +`AddDeploymentManager_RegistersResolvableOptions_WithDefaults`, +`AddDeploymentManager_OptionsBindToConfigurationSection_AsTheHostWires`, +`OptionsSection_MatchesTheConventionalComponentSectionPath`. ### DeploymentManager-009 — Misleading timeout comment on `DeleteInstanceAsync` diff --git a/src/ScadaLink.Communication/Grpc/SiteStreamGrpcServer.cs b/src/ScadaLink.Communication/Grpc/SiteStreamGrpcServer.cs index a17481c..9a62e46 100644 --- a/src/ScadaLink.Communication/Grpc/SiteStreamGrpcServer.cs +++ b/src/ScadaLink.Communication/Grpc/SiteStreamGrpcServer.cs @@ -24,7 +24,11 @@ public class SiteStreamGrpcServer : SiteStreamService.SiteStreamServiceBase private volatile bool _ready; private long _actorCounter; - public SiteStreamGrpcServer( + /// + /// Test-only constructor — kept internal so the DI container sees a + /// single public constructor and is not faced with an ambiguous choice. + /// + internal SiteStreamGrpcServer( ISiteStreamSubscriber streamSubscriber, ILogger logger, int maxConcurrentStreams = 100) diff --git a/src/ScadaLink.Communication/ScadaLink.Communication.csproj b/src/ScadaLink.Communication/ScadaLink.Communication.csproj index f9be739..b9e7fed 100644 --- a/src/ScadaLink.Communication/ScadaLink.Communication.csproj +++ b/src/ScadaLink.Communication/ScadaLink.Communication.csproj @@ -9,6 +9,7 @@ + diff --git a/src/ScadaLink.DeploymentManager/ScadaLink.DeploymentManager.csproj b/src/ScadaLink.DeploymentManager/ScadaLink.DeploymentManager.csproj index ff9fe7d..4d0eec3 100644 --- a/src/ScadaLink.DeploymentManager/ScadaLink.DeploymentManager.csproj +++ b/src/ScadaLink.DeploymentManager/ScadaLink.DeploymentManager.csproj @@ -11,7 +11,6 @@ - diff --git a/src/ScadaLink.DeploymentManager/ServiceCollectionExtensions.cs b/src/ScadaLink.DeploymentManager/ServiceCollectionExtensions.cs index 955d620..7efeeb2 100644 --- a/src/ScadaLink.DeploymentManager/ServiceCollectionExtensions.cs +++ b/src/ScadaLink.DeploymentManager/ServiceCollectionExtensions.cs @@ -1,4 +1,3 @@ -using Microsoft.Extensions.Configuration; using Microsoft.Extensions.DependencyInjection; namespace ScadaLink.DeploymentManager; @@ -7,37 +6,26 @@ public static class ServiceCollectionExtensions { /// /// Configuration section that is bound to. + /// The Host binds this section to appsettings.json (see + /// Program.cs); component libraries do not depend on + /// IConfiguration directly, consistent with the Options-pattern + /// convention enforced by OptionsTests. /// public const string OptionsSection = "ScadaLink:DeploymentManager"; /// - /// Registers the Deployment Manager services and binds - /// to the - /// configuration section, consistent with the - /// Options-pattern convention ("Per-component configuration via - /// appsettings.json sections bound to options classes"). - /// - public static IServiceCollection AddDeploymentManager( - this IServiceCollection services, - IConfiguration configuration) - { - ArgumentNullException.ThrowIfNull(configuration); - - // DeploymentManager-008: bind the options class so the operation-lock - // and artifact-deployment timeouts are tunable via appsettings.json. - services.Configure(configuration.GetSection(OptionsSection)); - return services.AddDeploymentManager(); - } - - /// - /// Registers the Deployment Manager services without binding options to - /// configuration. falls back to its - /// declared defaults unless configured elsewhere. Prefer the - /// - /// overload so the options are bound to appsettings.json. + /// Registers the Deployment Manager services. + /// is registered via so + /// IOptions<DeploymentManagerOptions> is always resolvable; the Host + /// binds to configuration so the operation-lock and + /// artifact-deployment timeouts are tunable via appsettings.json. /// public static IServiceCollection AddDeploymentManager(this IServiceCollection services) { + // DeploymentManager-008: ensure the options class is always resolvable. + // The Host binds OptionsSection to appsettings.json; absent that binding + // the declared option-class defaults apply. + services.AddOptions(); services.AddSingleton(); services.AddScoped(); services.AddScoped(); diff --git a/src/ScadaLink.Host/Program.cs b/src/ScadaLink.Host/Program.cs index d4ca049..9eca495 100644 --- a/src/ScadaLink.Host/Program.cs +++ b/src/ScadaLink.Host/Program.cs @@ -106,6 +106,8 @@ try SiteServiceRegistration.BindSharedOptions(builder.Services, builder.Configuration); builder.Services.Configure(builder.Configuration.GetSection("ScadaLink:Security")); builder.Services.Configure(builder.Configuration.GetSection("ScadaLink:InboundApi")); + builder.Services.Configure( + builder.Configuration.GetSection(ScadaLink.DeploymentManager.ServiceCollectionExtensions.OptionsSection)); var app = builder.Build(); diff --git a/tests/ScadaLink.DeploymentManager.Tests/ServiceCollectionExtensionsTests.cs b/tests/ScadaLink.DeploymentManager.Tests/ServiceCollectionExtensionsTests.cs index 7503814..e35ce4b 100644 --- a/tests/ScadaLink.DeploymentManager.Tests/ServiceCollectionExtensionsTests.cs +++ b/tests/ScadaLink.DeploymentManager.Tests/ServiceCollectionExtensionsTests.cs @@ -5,15 +5,32 @@ using Microsoft.Extensions.Options; namespace ScadaLink.DeploymentManager.Tests; /// -/// DeploymentManager-008: DeploymentManagerOptions must be bound to the -/// "ScadaLink:DeploymentManager" configuration section, consistent with the -/// Options-pattern convention used by the other components. +/// DeploymentManager-008: DeploymentManagerOptions must be resolvable via the +/// Options pattern and bindable to the "ScadaLink:DeploymentManager" +/// configuration section. The component itself does not depend on +/// IConfiguration (enforced by Host's OptionsTests) — the Host binds the +/// section; AddDeploymentManager only guarantees IOptions resolvability. /// public class ServiceCollectionExtensionsTests { [Fact] - public void AddDeploymentManager_WithConfiguration_BindsDeploymentManagerOptions() + public void AddDeploymentManager_RegistersResolvableOptions_WithDefaults() { + var services = new ServiceCollection(); + services.AddDeploymentManager(); + + using var provider = services.BuildServiceProvider(); + var options = provider.GetRequiredService>().Value; + + // No section bound -> the option-class defaults are retained. + Assert.Equal(TimeSpan.FromSeconds(5), options.OperationLockTimeout); + } + + [Fact] + public void AddDeploymentManager_OptionsBindToConfigurationSection_AsTheHostWires() + { + // Mirrors the Host wiring: the Host calls Configure + // against OptionsSection, then AddDeploymentManager(). var configuration = new ConfigurationBuilder() .AddInMemoryCollection(new Dictionary { @@ -23,7 +40,9 @@ public class ServiceCollectionExtensionsTests .Build(); var services = new ServiceCollection(); - services.AddDeploymentManager(configuration); + services.Configure( + configuration.GetSection(ServiceCollectionExtensions.OptionsSection)); + services.AddDeploymentManager(); using var provider = services.BuildServiceProvider(); var options = provider.GetRequiredService>().Value; @@ -33,17 +52,8 @@ public class ServiceCollectionExtensionsTests } [Fact] - public void AddDeploymentManager_WithConfiguration_MissingSection_UsesDefaults() + public void OptionsSection_MatchesTheConventionalComponentSectionPath() { - var configuration = new ConfigurationBuilder().Build(); - - var services = new ServiceCollection(); - services.AddDeploymentManager(configuration); - - using var provider = services.BuildServiceProvider(); - var options = provider.GetRequiredService>().Value; - - // No section present -> the option-class defaults are retained. - Assert.Equal(TimeSpan.FromSeconds(5), options.OperationLockTimeout); + Assert.Equal("ScadaLink:DeploymentManager", ServiceCollectionExtensions.OptionsSection); } } diff --git a/tests/ScadaLink.Host.Tests/CompositionRootTests.cs b/tests/ScadaLink.Host.Tests/CompositionRootTests.cs index 8c5348f..5aa5d8e 100644 --- a/tests/ScadaLink.Host.Tests/CompositionRootTests.cs +++ b/tests/ScadaLink.Host.Tests/CompositionRootTests.cs @@ -298,6 +298,9 @@ public class SiteCompositionRootTests : IDisposable ["ScadaLink:Node:RemotingPort"] = "0", ["ScadaLink:Node:GrpcPort"] = "0", ["ScadaLink:Database:SiteDbPath"] = _tempDbPath, + // ClusterOptions requires at least one seed node (ClusterOptionsValidator). + ["ScadaLink:Cluster:SeedNodes:0"] = "akka.tcp://scadalink@localhost:2551", + ["ScadaLink:Cluster:SeedNodes:1"] = "akka.tcp://scadalink@localhost:2552", }); // gRPC server registration (mirrors Program.cs site section)