dohertj2/scadalink-design
1
0
Fork 0
Code Issues Pull Requests 1 Actions Packages Projects Releases Wiki Activity

fix(host,deployment-manager,communication): repair cross-module DI regressions from batch 1-2

Browse Source 
- DeploymentManager-008: revert IConfiguration overload (violated OptionsTests
  component-convention); Host now binds the ScadaLink:DeploymentManager section
- SiteStreamGrpcServer: make test-only int ctor internal so DI sees one public
  ctor (resolves ambiguous-constructor failure in SiteCompositionRootTests)
- Host site composition-root test config: supply Cluster:SeedNodes for the new
  ClusterOptionsValidator
This commit is contained in:
Joseph Doherty
2026-05-16 21:28:50 -04:00
parent 49fb85e92e
commit 632d44f38c
8 changed files with 64 additions and 55 deletions
Download Patch File Download Diff File Expand all files Collapse all files

26
code-reviews/DeploymentManager/findings.md
View File

@@ -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<DeploymentManagerOptions>()` so `IOptions<DeploymentManagerOptions>`
is always resolvable, and `Host/Program.cs` binds the
`ScadaLink:DeploymentManager` section (exposed as
`ServiceCollectionExtensions.OptionsSection`) via
`services.Configure<DeploymentManagerOptions>(...)` — 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`

6
src/ScadaLink.Communication/Grpc/SiteStreamGrpcServer.cs
View File

@@ -24,7 +24,11 @@ public class SiteStreamGrpcServer : SiteStreamService.SiteStreamServiceBase
private volatile bool _ready;
private long _actorCounter;
public SiteStreamGrpcServer(
/// <summary>
/// Test-only constructor — kept <c>internal</c> so the DI container sees a
/// single public constructor and is not faced with an ambiguous choice.
/// </summary>
internal SiteStreamGrpcServer(
ISiteStreamSubscriber streamSubscriber,
ILogger<SiteStreamGrpcServer> logger,
int maxConcurrentStreams = 100)

1
src/ScadaLink.Communication/ScadaLink.Communication.csproj
View File

@@ -9,6 +9,7 @@
<ItemGroup>
<InternalsVisibleTo Include="ScadaLink.Communication.Tests" />
<InternalsVisibleTo Include="ScadaLink.IntegrationTests" />
</ItemGroup>
<ItemGroup>

1
src/ScadaLink.DeploymentManager/ScadaLink.DeploymentManager.csproj
View File

@@ -11,7 +11,6 @@
<PackageReference Include="Microsoft.Extensions.DependencyInjection.Abstractions" />
<PackageReference Include="Microsoft.Extensions.Logging.Abstractions" />
<PackageReference Include="Microsoft.Extensions.Options" />
<PackageReference Include="Microsoft.Extensions.Options.ConfigurationExtensions" />
</ItemGroup>
<ItemGroup>

38
src/ScadaLink.DeploymentManager/ServiceCollectionExtensions.cs
View File

@@ -1,4 +1,3 @@
using Microsoft.Extensions.Configuration;
using Microsoft.Extensions.DependencyInjection;
namespace ScadaLink.DeploymentManager;
@@ -7,37 +6,26 @@ public static class ServiceCollectionExtensions
{
/// <summary>
/// Configuration section that <see cref="DeploymentManagerOptions"/> is bound to.
/// The Host binds this section to <c>appsettings.json</c> (see
/// <c>Program.cs</c>); component libraries do not depend on
/// <c>IConfiguration</c> directly, consistent with the Options-pattern
/// convention enforced by <c>OptionsTests</c>.
/// </summary>
public const string OptionsSection = "ScadaLink:DeploymentManager";
/// <summary>
/// Registers the Deployment Manager services and binds
/// <see cref="DeploymentManagerOptions"/> to the
/// <see cref="OptionsSection"/> configuration section, consistent with the
/// Options-pattern convention ("Per-component configuration via
/// appsettings.json sections bound to options classes").
/// </summary>
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<DeploymentManagerOptions>(configuration.GetSection(OptionsSection));
return services.AddDeploymentManager();
}
/// <summary>
/// Registers the Deployment Manager services without binding options to
/// configuration. <see cref="DeploymentManagerOptions"/> falls back to its
/// declared defaults unless configured elsewhere. Prefer the
/// <see cref="AddDeploymentManager(IServiceCollection, IConfiguration)"/>
/// overload so the options are bound to <c>appsettings.json</c>.
/// Registers the Deployment Manager services. <see cref="DeploymentManagerOptions"/>
/// is registered via <see cref="OptionsServiceCollectionExtensions.AddOptions"/> so
/// <c>IOptions&lt;DeploymentManagerOptions&gt;</c> is always resolvable; the Host
/// binds <see cref="OptionsSection"/> to configuration so the operation-lock and
/// artifact-deployment timeouts are tunable via <c>appsettings.json</c>.
/// </summary>
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<DeploymentManagerOptions>();
services.AddSingleton<OperationLockManager>();
services.AddScoped<IFlatteningPipeline, FlatteningPipeline>();
services.AddScoped<DeploymentService>();

2
src/ScadaLink.Host/Program.cs
View File

@@ -106,6 +106,8 @@ try
SiteServiceRegistration.BindSharedOptions(builder.Services, builder.Configuration);
builder.Services.Configure<SecurityOptions>(builder.Configuration.GetSection("ScadaLink:Security"));
builder.Services.Configure<InboundApiOptions>(builder.Configuration.GetSection("ScadaLink:InboundApi"));
builder.Services.Configure<DeploymentManagerOptions>(
builder.Configuration.GetSection(ScadaLink.DeploymentManager.ServiceCollectionExtensions.OptionsSection));
var app = builder.Build();

42
tests/ScadaLink.DeploymentManager.Tests/ServiceCollectionExtensionsTests.cs
View File

@@ -5,15 +5,32 @@ using Microsoft.Extensions.Options;
namespace ScadaLink.DeploymentManager.Tests;
/// <summary>
/// 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.
/// </summary>
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<IOptions<DeploymentManagerOptions>>().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<DeploymentManagerOptions>
// against OptionsSection, then AddDeploymentManager().
var configuration = new ConfigurationBuilder()
.AddInMemoryCollection(new Dictionary<string, string?>
{
@@ -23,7 +40,9 @@ public class ServiceCollectionExtensionsTests
.Build();
var services = new ServiceCollection();
services.AddDeploymentManager(configuration);
services.Configure<DeploymentManagerOptions>(
configuration.GetSection(ServiceCollectionExtensions.OptionsSection));
services.AddDeploymentManager();
using var provider = services.BuildServiceProvider();
var options = provider.GetRequiredService<IOptions<DeploymentManagerOptions>>().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<IOptions<DeploymentManagerOptions>>().Value;
// No section present -> the option-class defaults are retained.
Assert.Equal(TimeSpan.FromSeconds(5), options.OperationLockTimeout);
Assert.Equal("ScadaLink:DeploymentManager", ServiceCollectionExtensions.OptionsSection);
}
}

3
tests/ScadaLink.Host.Tests/CompositionRootTests.cs
View File

@@ -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)