From 34823213b16c263d4fa14878443e251f12be9f68 Mon Sep 17 00:00:00 2001 From: Andreas Bednarz <110360248+abparticular@users.noreply.github.com> Date: Wed, 14 Oct 2026 12:46:26 +1100 Subject: [PATCH 01/14] Add storage type picker to SCMU Add Instance wizard Lets users pick RavenDB, SQL Server, or PostgreSQL as the storage backend when creating a new Error or Audit instance, mirroring the existing transport picker. The persistence manifest/factory seam already supported this at the engine level; the gap was entirely in the SCMU UI (hardcoded to RavenDB) and in the primary instance's config writer, which couldn't write arbitrary persister settings the way the audit config writer already could. The picker is gated behind the ServiceControl_IncludePersistencePicker environment variable (same pattern as ServiceControl_IncludeLearningTransport), since the SQL Server/PostgreSQL persisters are still marked not-production-ready. With the flag unset, RavenDB is auto-selected and the picker is hidden, so behavior is unchanged by default. Error and Audit instances get independent selections rather than a single shared picker. Scoped to net-new instance creation only - no upgrade/migration support, since no RavenDB-to-EF data migration tool exists yet. Co-Authored-By: Claude Sonnet 5 --- .../ServiceControlAddScreenLoadedTests.cs | 19 +++++ .../AddAuditInstanceValidationTests.cs | 57 +++++++++++++ .../AddErrorInstanceValidationTests.cs | 57 +++++++++++++ .../ServiceControlAddAttachment.cs | 6 +- .../UI/InstanceAdd/ServiceControlAddView.xaml | 50 +++++++++++- .../InstanceAdd/ServiceControlAddViewModel.cs | 40 ++++++++++ .../ServiceControlAddViewModelValidator.cs | 18 +++++ .../ServiceControlAuditInformation.cs | 24 ++++++ .../InstanceAdd/ServiceControlInformation.cs | 24 ++++++ .../Validation/Validations.cs | 2 + .../persistence.manifest | 1 + .../persistence.manifest | 1 + ...pected_config_file.PostgreSQL.approved.txt | 20 +++++ ..._expected_config_file.RavenDB.approved.txt | 18 +++++ ...xpected_config_file.SQLServer.approved.txt | 20 +++++ .../Configuration/PersistenceManifestTests.cs | 13 +++ .../InstallationTests.cs | 79 +++++++++++++++++++ .../Validation/QueueValidationTests.cs | 4 + .../AuditInstanceSettingsList.cs | 1 + .../ServiceControl/ServiceControlAppConfig.cs | 52 +++++++++++- .../ServiceControlAuditAppConfig.cs | 5 ++ .../ServiceControl/SettingsList.cs | 2 + .../Instances/ServiceControlAuditInstance.cs | 2 + .../ServiceControlInstallableBase.cs | 2 + .../Instances/ServiceControlInstance.cs | 2 + .../Instances/ServiceControlPersisters.cs | 42 +++++++++- .../Interfaces.cs | 4 + 27 files changed, 558 insertions(+), 7 deletions(-) create mode 100644 src/ServiceControlInstaller.Engine.UnitTests/ApprovalFiles/InstallationTests.ServiceControl_install_should_write_expected_config_file.PostgreSQL.approved.txt create mode 100644 src/ServiceControlInstaller.Engine.UnitTests/ApprovalFiles/InstallationTests.ServiceControl_install_should_write_expected_config_file.RavenDB.approved.txt create mode 100644 src/ServiceControlInstaller.Engine.UnitTests/ApprovalFiles/InstallationTests.ServiceControl_install_should_write_expected_config_file.SQLServer.approved.txt diff --git a/src/ServiceControl.Config.Tests/ServiceControlAddScreenLoadedTests.cs b/src/ServiceControl.Config.Tests/ServiceControlAddScreenLoadedTests.cs index cf18a3c496..d6484c86e0 100644 --- a/src/ServiceControl.Config.Tests/ServiceControlAddScreenLoadedTests.cs +++ b/src/ServiceControl.Config.Tests/ServiceControlAddScreenLoadedTests.cs @@ -37,6 +37,25 @@ public void Transports_are_populated() } } + [Test] + public void Storage_types_default_to_RavenDB_only_when_picker_is_not_enabled() + { + var viewModel = new ServiceControlAddViewModel(); + + using (Assert.EnterMultipleScope()) + { + Assert.That(viewModel.ErrorStorageTypes.Select(m => m.Name), Is.EqualTo(new[] { "RavenDB" })); + Assert.That(viewModel.ErrorSelectedStorageType?.Name, Is.EqualTo("RavenDB")); + Assert.That(viewModel.ShowErrorStorageTypePicker, Is.False); + Assert.That(viewModel.ShowErrorStorageConnectionString, Is.False); + + Assert.That(viewModel.AuditStorageTypes.Select(m => m.Name), Is.EqualTo(new[] { "RavenDB" })); + Assert.That(viewModel.AuditSelectedStorageType?.Name, Is.EqualTo("RavenDB")); + Assert.That(viewModel.ShowAuditStorageTypePicker, Is.False); + Assert.That(viewModel.ShowAuditStorageConnectionString, Is.False); + } + } + [Test] public void Transport_connection_string_is_null() { diff --git a/src/ServiceControl.Config.Tests/Validation/AddAuditInstanceValidationTests.cs b/src/ServiceControl.Config.Tests/Validation/AddAuditInstanceValidationTests.cs index 449208f04a..8e003a58c9 100644 --- a/src/ServiceControl.Config.Tests/Validation/AddAuditInstanceValidationTests.cs +++ b/src/ServiceControl.Config.Tests/Validation/AddAuditInstanceValidationTests.cs @@ -151,6 +151,63 @@ public void #endregion + #region storagetype + + [Test] + public void Storage_type_cannot_be_empty_when_adding_audit_instance() + { + var viewModel = new ServiceControlAddViewModel + { + InstallAuditInstance = true, + SubmitAttempted = true, + AuditSelectedStorageType = null + }; + + viewModel.NotifyOfPropertyChange(nameof(viewModel.AuditSelectedStorageType)); + + var notifyErrorInfo = GetNotifyErrorInfo(viewModel); + var errors = notifyErrorInfo.GetErrors(nameof(viewModel.AuditSelectedStorageType)); + + Assert.That(errors, Is.Not.Empty); + } + + [TestCase("SQLServer")] + [TestCase("PostgreSQL")] + public void Storage_connection_string_cannot_be_empty_for_storage_types_that_require_one_when_adding_audit_instance(string storageTypeName) + { + var viewModel = new ServiceControlAddViewModel + { + InstallAuditInstance = true, + AuditSelectedStorageType = ServiceControlPersisters.GetAuditPersistence(storageTypeName), + SubmitAttempted = true, + AuditStorageConnectionString = string.Empty + }; + + var notifyErrorInfo = GetNotifyErrorInfo(viewModel); + var errors = notifyErrorInfo.GetErrors(nameof(viewModel.AuditStorageConnectionString)); + + Assert.That(errors, Is.Not.Empty); + } + + [Test] + public void Storage_connection_string_can_be_empty_for_RavenDB_when_adding_audit_instance() + { + var viewModel = new ServiceControlAddViewModel + { + InstallAuditInstance = true, + AuditSelectedStorageType = ServiceControlPersisters.GetAuditPersistence("RavenDB"), + SubmitAttempted = true, + AuditStorageConnectionString = string.Empty + }; + + var notifyErrorInfo = GetNotifyErrorInfo(viewModel); + var errors = notifyErrorInfo.GetErrors(nameof(viewModel.AuditStorageConnectionString)); + + Assert.That(errors, Is.Empty); + } + + #endregion + #region auditinstancename [Test] diff --git a/src/ServiceControl.Config.Tests/Validation/AddErrorInstanceValidationTests.cs b/src/ServiceControl.Config.Tests/Validation/AddErrorInstanceValidationTests.cs index d81a07c4bd..75b4058e2d 100644 --- a/src/ServiceControl.Config.Tests/Validation/AddErrorInstanceValidationTests.cs +++ b/src/ServiceControl.Config.Tests/Validation/AddErrorInstanceValidationTests.cs @@ -150,6 +150,63 @@ public void Transport_connection_string_cannot_be_null_if_sample_connection_stri } #endregion + #region storagetype + + [Test] + public void Storage_type_cannot_be_empty_when_adding_error_instance() + { + var viewModel = new ServiceControlAddViewModel + { + InstallErrorInstance = true, + SubmitAttempted = true, + ErrorSelectedStorageType = null + }; + + viewModel.NotifyOfPropertyChange(nameof(viewModel.ErrorSelectedStorageType)); + + var notifyErrorInfo = GetNotifyErrorInfo(viewModel); + var errors = notifyErrorInfo.GetErrors(nameof(viewModel.ErrorSelectedStorageType)); + + Assert.That(errors, Is.Not.Empty); + } + + [TestCase("SQLServer")] + [TestCase("PostgreSQL")] + public void Storage_connection_string_cannot_be_empty_for_storage_types_that_require_one_when_adding_error_instance(string storageTypeName) + { + var viewModel = new ServiceControlAddViewModel + { + InstallErrorInstance = true, + ErrorSelectedStorageType = ServiceControlPersisters.GetPrimaryPersistence(storageTypeName), + SubmitAttempted = true, + ErrorStorageConnectionString = string.Empty + }; + + var notifyErrorInfo = GetNotifyErrorInfo(viewModel); + var errors = notifyErrorInfo.GetErrors(nameof(viewModel.ErrorStorageConnectionString)); + + Assert.That(errors, Is.Not.Empty); + } + + [Test] + public void Storage_connection_string_can_be_empty_for_RavenDB_when_adding_error_instance() + { + var viewModel = new ServiceControlAddViewModel + { + InstallErrorInstance = true, + ErrorSelectedStorageType = ServiceControlPersisters.GetPrimaryPersistence("RavenDB"), + SubmitAttempted = true, + ErrorStorageConnectionString = string.Empty + }; + + var notifyErrorInfo = GetNotifyErrorInfo(viewModel); + var errors = notifyErrorInfo.GetErrors(nameof(viewModel.ErrorStorageConnectionString)); + + Assert.That(errors, Is.Empty); + } + + #endregion + #region errorinstancename [Test] diff --git a/src/ServiceControl.Config/UI/InstanceAdd/ServiceControlAddAttachment.cs b/src/ServiceControl.Config/UI/InstanceAdd/ServiceControlAddAttachment.cs index 8cf5c88352..0a851a3984 100644 --- a/src/ServiceControl.Config/UI/InstanceAdd/ServiceControlAddAttachment.cs +++ b/src/ServiceControl.Config/UI/InstanceAdd/ServiceControlAddAttachment.cs @@ -53,7 +53,7 @@ async Task Add(CancellationToken cancellationToken) viewModel.InProgress = true; - var serviceControlNewInstance = viewModel.InstallErrorInstance ? ServiceControlNewInstance.CreateWithDefaultPersistence() : null; + var serviceControlNewInstance = viewModel.InstallErrorInstance ? ServiceControlNewInstance.CreateWithPersistence(viewModel.ServiceControl.SelectedStorageType.Name) : null; if (viewModel.InstallErrorInstance) { @@ -73,6 +73,7 @@ async Task Add(CancellationToken cancellationToken) serviceControlNewInstance.ForwardErrorMessages = viewModel.ServiceControl.ErrorForwarding.Value; serviceControlNewInstance.TransportPackage = viewModel.SelectedTransport; serviceControlNewInstance.ConnectionString = viewModel.ConnectionString; + serviceControlNewInstance.PersistenceConnectionString = viewModel.ServiceControl.StorageConnectionString; serviceControlNewInstance.ErrorRetentionPeriod = viewModel.ServiceControl.ErrorRetentionPeriod; serviceControlNewInstance.ServiceAccount = viewModel.ServiceControl.ServiceAccount; serviceControlNewInstance.ServiceAccountPwd = viewModel.ServiceControl.Password; @@ -80,7 +81,7 @@ async Task Add(CancellationToken cancellationToken) serviceControlNewInstance.EnableIntegratedServicePulse = viewModel.ServiceControl.EnableIntegratedServicePulse.Value; } - var auditNewInstance = viewModel.InstallAuditInstance ? ServiceControlAuditNewInstance.CreateWithDefaultPersistence() : null; + var auditNewInstance = viewModel.InstallAuditInstance ? ServiceControlAuditNewInstance.CreateWithPersistence(viewModel.ServiceControlAudit.SelectedStorageType.Name) : null; if (viewModel.InstallAuditInstance) { auditNewInstance.DisplayName = viewModel.ServiceControlAudit.InstanceName; @@ -98,6 +99,7 @@ async Task Add(CancellationToken cancellationToken) auditNewInstance.ForwardAuditMessages = viewModel.ServiceControlAudit.AuditForwarding.Value; auditNewInstance.TransportPackage = viewModel.SelectedTransport; auditNewInstance.ConnectionString = viewModel.ConnectionString; + auditNewInstance.PersistenceConnectionString = viewModel.ServiceControlAudit.StorageConnectionString; auditNewInstance.AuditRetentionPeriod = viewModel.ServiceControlAudit.AuditRetentionPeriod; auditNewInstance.ServiceAccount = viewModel.ServiceControlAudit.ServiceAccount; auditNewInstance.ServiceAccountPwd = viewModel.ServiceControlAudit.Password; diff --git a/src/ServiceControl.Config/UI/InstanceAdd/ServiceControlAddView.xaml b/src/ServiceControl.Config/UI/InstanceAdd/ServiceControlAddView.xaml index 8b34111dc8..218c7a34c8 100644 --- a/src/ServiceControl.Config/UI/InstanceAdd/ServiceControlAddView.xaml +++ b/src/ServiceControl.Config/UI/InstanceAdd/ServiceControlAddView.xaml @@ -139,6 +139,28 @@ + + + + + + + + + Text="{Binding ErrorDatabasePath}" + Visibility="{Binding ShowErrorDatabasePathFields, Converter={StaticResource boolToVis}}" /> + + + + + + + + @@ -340,7 +385,8 @@ + Text="{Binding AuditDatabasePath}" + Visibility="{Binding ShowAuditDatabasePathFields, Converter={StaticResource boolToVis}}" /> ServiceControl.DatabaseMaintenancePortNumber = value; } + public IEnumerable ErrorStorageTypes => ServiceControl.StorageTypes; + + public PersistenceManifest ErrorSelectedStorageType + { + get => ServiceControl.SelectedStorageType; + set => ServiceControl.SelectedStorageType = value; + } + + public string ErrorStorageConnectionString + { + get => ServiceControl.StorageConnectionString; + set => ServiceControl.StorageConnectionString = value; + } + + public bool ShowErrorStorageTypePicker => ServiceControl.ShowStorageTypePicker; + + public bool ShowErrorStorageConnectionString => ServiceControl.ShowStorageConnectionString; + + public bool ShowErrorDatabasePathFields => ServiceControl.ShowDatabasePathFields; + public ICommand ErrorSelectDestinationPath => ServiceControl.SelectDestinationPath; public string ErrorDestinationPath @@ -367,6 +387,26 @@ public string AuditDatabaseMaintenancePortNumber set => ServiceControlAudit.DatabaseMaintenancePortNumber = value; } + public IEnumerable AuditStorageTypes => ServiceControlAudit.StorageTypes; + + public PersistenceManifest AuditSelectedStorageType + { + get => ServiceControlAudit.SelectedStorageType; + set => ServiceControlAudit.SelectedStorageType = value; + } + + public string AuditStorageConnectionString + { + get => ServiceControlAudit.StorageConnectionString; + set => ServiceControlAudit.StorageConnectionString = value; + } + + public bool ShowAuditStorageTypePicker => ServiceControlAudit.ShowStorageTypePicker; + + public bool ShowAuditStorageConnectionString => ServiceControlAudit.ShowStorageConnectionString; + + public bool ShowAuditDatabasePathFields => ServiceControlAudit.ShowDatabasePathFields; + public ICommand AuditSelectDestinationPath => ServiceControlAudit.SelectDestinationPath; public string AuditDestinationPath diff --git a/src/ServiceControl.Config/UI/InstanceAdd/ServiceControlAddViewModelValidator.cs b/src/ServiceControl.Config/UI/InstanceAdd/ServiceControlAddViewModelValidator.cs index 50b27017a8..d4d49ec01e 100644 --- a/src/ServiceControl.Config/UI/InstanceAdd/ServiceControlAddViewModelValidator.cs +++ b/src/ServiceControl.Config/UI/InstanceAdd/ServiceControlAddViewModelValidator.cs @@ -46,6 +46,15 @@ public ServiceControlAddViewModelValidator() .WithMessage("Error instance name is already in use") .When(viewModel => viewModel.InstallErrorInstance); + RuleFor(x => x.ErrorSelectedStorageType) + .NotEmpty() + .When(x => x.InstallErrorInstance && x.SubmitAttempted); + + RuleFor(x => x.ErrorStorageConnectionString) + .NotEmpty() + .WithMessage(Validation.Validations.MSG_THIS_STORAGE_TYPE_REQUIRES_A_CONNECTION_STRING) + .When(x => x.InstallErrorInstance && x.ShowErrorStorageConnectionString && x.SubmitAttempted); + RuleFor(x => x.ErrorServiceAccount) .NotEmpty() .When(x => x.InstallErrorInstance && x.SubmitAttempted); @@ -157,6 +166,15 @@ public ServiceControlAddViewModelValidator() .WithMessage(string.Format(Validation.Validations.MSG_CANTCONTAINWHITESPACE, "Error instance queue address")) .When(viewModel => viewModel.InstallAuditInstance && !viewModel.InstallErrorInstance); + RuleFor(x => x.AuditSelectedStorageType) + .NotEmpty() + .When(x => x.InstallAuditInstance && x.SubmitAttempted); + + RuleFor(x => x.AuditStorageConnectionString) + .NotEmpty() + .WithMessage(Validation.Validations.MSG_THIS_STORAGE_TYPE_REQUIRES_A_CONNECTION_STRING) + .When(x => x.InstallAuditInstance && x.ShowAuditStorageConnectionString && x.SubmitAttempted); + RuleFor(x => x.AuditServiceAccount) .NotEmpty() .When(x => x.InstallAuditInstance && x.SubmitAttempted); diff --git a/src/ServiceControl.Config/UI/InstanceAdd/ServiceControlAuditInformation.cs b/src/ServiceControl.Config/UI/InstanceAdd/ServiceControlAuditInformation.cs index 9a8fc02769..8fa61c2341 100644 --- a/src/ServiceControl.Config/UI/InstanceAdd/ServiceControlAuditInformation.cs +++ b/src/ServiceControl.Config/UI/InstanceAdd/ServiceControlAuditInformation.cs @@ -49,10 +49,33 @@ public ServiceControlAuditInformation(ServiceControlEditorViewModel viewModelPar DatabaseMaintenancePortNumber = "44445"; EnableFullTextSearchOnBodies = EnableFullTextSearchOnBodiesOptions.First(p => p.Value); //Default to On. ViewModelParent = viewModelParent; + SelectedStorageType = StorageTypes.First(); } public ServiceControlEditorViewModel ViewModelParent { get; } + public IEnumerable StorageTypes { get; } = ServiceControlPersisters.GetSupportedAuditManifests(); + + // Intentionally not editable after creation - changing storage backend on an existing + // instance has no supported data migration path yet, so this is only ever set at Add time. + public PersistenceManifest SelectedStorageType + { + get => selectedStorageType; + set + { + StorageConnectionString = null; + selectedStorageType = value; + } + } + + public string StorageConnectionString { get; set; } + + public bool ShowStorageTypePicker => StorageTypes.Count() > 1; + + public bool ShowStorageConnectionString => SelectedStorageType != null && SelectedStorageType.Settings.Any(s => s.Mandatory && s.Name.EndsWith("/ConnectionString")); + + public bool ShowDatabasePathFields => SelectedStorageType == null || SelectedStorageType.Settings.Any(s => s.Name.EndsWith("/DBPath")); + public int MinimumAuditRetentionPeriod => SettingConstants.AuditRetentionPeriodMinInDays; public int MaximumAuditRetentionPeriod => SettingConstants.AuditRetentionPeriodMaxInDays; @@ -137,4 +160,5 @@ public void UpdateFromInstance(ServiceControlAuditInstance instance) EnableFullTextSearchOnBodies = EnableFullTextSearchOnBodiesOptions.FirstOrDefault(p => p.Value == instance.EnableFullTextSearchOnBodies); } ForwardingOption auditForwarding; + PersistenceManifest selectedStorageType; } \ No newline at end of file diff --git a/src/ServiceControl.Config/UI/InstanceAdd/ServiceControlInformation.cs b/src/ServiceControl.Config/UI/InstanceAdd/ServiceControlInformation.cs index 13e6d1289c..7727a7834f 100644 --- a/src/ServiceControl.Config/UI/InstanceAdd/ServiceControlInformation.cs +++ b/src/ServiceControl.Config/UI/InstanceAdd/ServiceControlInformation.cs @@ -63,6 +63,7 @@ public ServiceControlInformation(ServiceControlEditorViewModel viewModelParent) EnableFullTextSearchOnBodies = EnableFullTextSearchOnBodiesOptions.First(p => p.Value); //Default to On. EnableIntegratedServicePulse = EnableIntegratedServicePulseOptions.First(p => p.Value); //Default to On. ViewModelParent = viewModelParent; + SelectedStorageType = StorageTypes.First(); } public int MaximumErrorRetentionPeriod => SettingConstants.ErrorRetentionPeriodMaxInDays; @@ -73,6 +74,28 @@ public ServiceControlInformation(ServiceControlEditorViewModel viewModelParent) public ServiceControlEditorViewModel ViewModelParent { get; } + public IEnumerable StorageTypes { get; } = ServiceControlPersisters.GetSupportedPrimaryManifests(); + + // Intentionally not editable after creation - changing storage backend on an existing + // instance has no supported data migration path yet, so this is only ever set at Add time. + public PersistenceManifest SelectedStorageType + { + get => selectedStorageType; + set + { + StorageConnectionString = null; + selectedStorageType = value; + } + } + + public string StorageConnectionString { get; set; } + + public bool ShowStorageTypePicker => StorageTypes.Count() > 1; + + public bool ShowStorageConnectionString => SelectedStorageType != null && SelectedStorageType.Settings.Any(s => s.Mandatory && s.Name.EndsWith("/ConnectionString")); + + public bool ShowDatabasePathFields => SelectedStorageType == null || SelectedStorageType.Settings.Any(s => s.Name.EndsWith("/DBPath")); + public double ErrorRetention { get; set; } public TimeSpan ErrorRetentionPeriod => ErrorRetentionUnits == TimeSpanUnits.Days ? TimeSpan.FromDays(ErrorRetention) : TimeSpan.FromHours(ErrorRetention); @@ -144,4 +167,5 @@ public void UpdateFromInstance(ServiceControlInstance instance) } ForwardingOption errorForwarding; + PersistenceManifest selectedStorageType; } \ No newline at end of file diff --git a/src/ServiceControl.Config/Validation/Validations.cs b/src/ServiceControl.Config/Validation/Validations.cs index 7fb7ba8373..23e68f6f52 100644 --- a/src/ServiceControl.Config/Validation/Validations.cs +++ b/src/ServiceControl.Config/Validation/Validations.cs @@ -221,6 +221,8 @@ public static IRuleBuilderOptions ValidHostname(this IRuleBuilder< public const string MSG_THIS_TRANSPORT_REQUIRES_A_CONNECTION_STRING = "This transport requires a connection string."; + public const string MSG_THIS_STORAGE_TYPE_REQUIRES_A_CONNECTION_STRING = "This storage type requires a database connection string."; + public const string MSG_CANTCONTAINWHITESPACE = "{0} cannot contain white space."; public const string MSG_SELECTAUDITFORWARDING = "Must select audit forwarding."; diff --git a/src/ServiceControl.Persistence.EFCore.PostgreSql/persistence.manifest b/src/ServiceControl.Persistence.EFCore.PostgreSql/persistence.manifest index 433c31a5ae..ae1e3a9851 100644 --- a/src/ServiceControl.Persistence.EFCore.PostgreSql/persistence.manifest +++ b/src/ServiceControl.Persistence.EFCore.PostgreSql/persistence.manifest @@ -15,6 +15,7 @@ }, { "Name": "ServiceControl/MessageBody/StorageType", + "DefaultValue": "FileSystem", "Mandatory": true }, { diff --git a/src/ServiceControl.Persistence.EFCore.SqlServer/persistence.manifest b/src/ServiceControl.Persistence.EFCore.SqlServer/persistence.manifest index 9c1f04c024..2b56bc6a35 100644 --- a/src/ServiceControl.Persistence.EFCore.SqlServer/persistence.manifest +++ b/src/ServiceControl.Persistence.EFCore.SqlServer/persistence.manifest @@ -15,6 +15,7 @@ }, { "Name": "ServiceControl/MessageBody/StorageType", + "DefaultValue": "FileSystem", "Mandatory": true }, { diff --git a/src/ServiceControlInstaller.Engine.UnitTests/ApprovalFiles/InstallationTests.ServiceControl_install_should_write_expected_config_file.PostgreSQL.approved.txt b/src/ServiceControlInstaller.Engine.UnitTests/ApprovalFiles/InstallationTests.ServiceControl_install_should_write_expected_config_file.PostgreSQL.approved.txt new file mode 100644 index 0000000000..e7aa23a337 --- /dev/null +++ b/src/ServiceControlInstaller.Engine.UnitTests/ApprovalFiles/InstallationTests.ServiceControl_install_should_write_expected_config_file.PostgreSQL.approved.txt @@ -0,0 +1,20 @@ + + + + + + + + + + + + + + + + + + + \ No newline at end of file diff --git a/src/ServiceControlInstaller.Engine.UnitTests/ApprovalFiles/InstallationTests.ServiceControl_install_should_write_expected_config_file.RavenDB.approved.txt b/src/ServiceControlInstaller.Engine.UnitTests/ApprovalFiles/InstallationTests.ServiceControl_install_should_write_expected_config_file.RavenDB.approved.txt new file mode 100644 index 0000000000..068e932d9d --- /dev/null +++ b/src/ServiceControlInstaller.Engine.UnitTests/ApprovalFiles/InstallationTests.ServiceControl_install_should_write_expected_config_file.RavenDB.approved.txt @@ -0,0 +1,18 @@ + + + + + + + + + + + + + + + + + + \ No newline at end of file diff --git a/src/ServiceControlInstaller.Engine.UnitTests/ApprovalFiles/InstallationTests.ServiceControl_install_should_write_expected_config_file.SQLServer.approved.txt b/src/ServiceControlInstaller.Engine.UnitTests/ApprovalFiles/InstallationTests.ServiceControl_install_should_write_expected_config_file.SQLServer.approved.txt new file mode 100644 index 0000000000..f5c6c1b0ba --- /dev/null +++ b/src/ServiceControlInstaller.Engine.UnitTests/ApprovalFiles/InstallationTests.ServiceControl_install_should_write_expected_config_file.SQLServer.approved.txt @@ -0,0 +1,20 @@ + + + + + + + + + + + + + + + + + + + \ No newline at end of file diff --git a/src/ServiceControlInstaller.Engine.UnitTests/Configuration/PersistenceManifestTests.cs b/src/ServiceControlInstaller.Engine.UnitTests/Configuration/PersistenceManifestTests.cs index fe91cd2449..8da5af27d7 100644 --- a/src/ServiceControlInstaller.Engine.UnitTests/Configuration/PersistenceManifestTests.cs +++ b/src/ServiceControlInstaller.Engine.UnitTests/Configuration/PersistenceManifestTests.cs @@ -56,6 +56,19 @@ public void Raven35IsDefault() Assert.That(manifests.All(m => m.Name == "RavenDB35")); } + [Test] + public void SupportedManifestsDefaultToRavenDBOnly() + { + // ServiceControl_IncludePersistencePicker is read once in the static constructor, so this only + // exercises the flag-off (default) path. See ServiceControlCoreTransports' equivalent + // ServiceControl_IncludeLearningTransport flag, which has the same untested-when-on limitation. + Assert.Multiple(() => + { + Assert.That(ServiceControlPersisters.GetSupportedPrimaryManifests().Select(m => m.Name), Is.EqualTo(new[] { "RavenDB" })); + Assert.That(ServiceControlPersisters.GetSupportedAuditManifests().Select(m => m.Name), Is.EqualTo(new[] { "RavenDB" })); + }); + } + [Test] public void UnknownPersister() { diff --git a/src/ServiceControlInstaller.Engine.UnitTests/InstallationTests.cs b/src/ServiceControlInstaller.Engine.UnitTests/InstallationTests.cs index b6516ac3a2..261370e987 100644 --- a/src/ServiceControlInstaller.Engine.UnitTests/InstallationTests.cs +++ b/src/ServiceControlInstaller.Engine.UnitTests/InstallationTests.cs @@ -85,4 +85,83 @@ public static IEnumerable GetAuditPersistenceManifestPaths() return Directory.GetFiles(persistersPath, "persistence.manifest", SearchOption.AllDirectories); } } + + class ServiceControlInstallationTests + { + const string ConnectionStringPlaceholder = "placeholder-connection-string"; + + [Test] + [TestCaseSource(nameof(GetPrimaryPersistenceManifestPaths))] + public void ServiceControl_install_should_write_expected_config_file(string manifestPath) + { + var installPath = Path.Combine(Path.GetTempPath(), TestContext.CurrentContext.Test.ID, "install"); + var dbPath = Path.Combine(Path.GetTempPath(), TestContext.CurrentContext.Test.ID, "db"); + var logPath = Path.Combine(Path.GetTempPath(), TestContext.CurrentContext.Test.ID, "log"); + + try + { + PersistenceManifest persistenceManifest; + + using (var reader = new StreamReader(manifestPath)) + { + var manifestContent = reader.ReadToEnd(); + persistenceManifest = JsonSerializer.Deserialize(manifestContent); + } + + if (!persistenceManifest.IsSupported) + { + Assert.Ignore("Don't care about config for unsupported persistence types."); + } + + if (persistenceManifest.Settings.Any(setting => setting.Mandatory && setting.DefaultValue == null && !InstallerSuppliedSettings.Contains(setting.Name))) + { + Assert.Ignore("The installer cannot create an instance on this persister yet: it has no value for one of its mandatory settings."); + } + + var newInstance = new ServiceControlNewInstance(persistenceManifest); + + if (Directory.Exists(installPath)) + { + Directory.Delete(installPath, true); + } + + newInstance.InstallPath = installPath; + newInstance.TransportPackage = ServiceControlCoreTransports.Find("LearningTransport"); + + newInstance.DBPath = dbPath; + newInstance.LogPath = logPath; + newInstance.HostName = "localhost"; + newInstance.DatabaseMaintenancePort = 33333; + newInstance.PersistenceConnectionString = ConnectionStringPlaceholder; + + newInstance.WriteConfigurationFile(); + + var configFile = File.ReadAllText(Path.Combine(installPath, "ServiceControl.exe.config")); + var scenario = Path.GetFileName(Path.GetDirectoryName(manifestPath)); + + Approver.Verify(configFile, input => input.Replace(dbPath, "value-not-asserted").Replace(logPath, "value-not-asserted"), scenario); + } + finally + { + FileUtils.DeleteDirectory(installPath, true, false); + FileUtils.DeleteDirectory(dbPath, true, false); + FileUtils.DeleteDirectory(logPath, true, false); + } + } + + static readonly string[] InstallerSuppliedSettings = + [ + ServiceControlSettings.DBPath.Name, + ServiceControlSettings.DatabaseMaintenancePort.Name, + ServiceControlSettings.HostName.Name, + ServiceControlSettings.DatabaseConnectionString.Name + ]; + + public static IEnumerable GetPrimaryPersistenceManifestPaths() + { + var deployPath = Path.GetFullPath(Path.Combine(TestContext.CurrentContext.TestDirectory, "..", "..", "..", "..", "..", "deploy")); + var persistersPath = Path.Combine(deployPath, "Particular.ServiceControl", "Persisters"); + return Directory.GetFiles(persistersPath, "persistence.manifest", SearchOption.AllDirectories); + } + } } diff --git a/src/ServiceControlInstaller.Engine.UnitTests/Validation/QueueValidationTests.cs b/src/ServiceControlInstaller.Engine.UnitTests/Validation/QueueValidationTests.cs index aaf6fdf138..392ccda2f0 100644 --- a/src/ServiceControlInstaller.Engine.UnitTests/Validation/QueueValidationTests.cs +++ b/src/ServiceControlInstaller.Engine.UnitTests/Validation/QueueValidationTests.cs @@ -61,6 +61,8 @@ class FakeServiceControlInstance : IServiceControlInstance public PersistenceManifest PersistenceManifest { get; set; } + public string PersistenceConnectionString { get; set; } + public string ConnectionString { get; set; } public string Url { get; set; } @@ -84,6 +86,8 @@ class FakeServiceControlAuditInstance : IServiceControlAuditInstance public PersistenceManifest PersistenceManifest { get; set; } + public string PersistenceConnectionString { get; set; } + public bool EnableFullTextSearchOnBodies { get; set; } public int Port { get; set; } diff --git a/src/ServiceControlInstaller.Engine/Configuration/ServiceControl/AuditInstanceSettingsList.cs b/src/ServiceControlInstaller.Engine/Configuration/ServiceControl/AuditInstanceSettingsList.cs index d557bf4bcf..bf7ceb427a 100644 --- a/src/ServiceControlInstaller.Engine/Configuration/ServiceControl/AuditInstanceSettingsList.cs +++ b/src/ServiceControlInstaller.Engine/Configuration/ServiceControl/AuditInstanceSettingsList.cs @@ -21,6 +21,7 @@ public static class AuditInstanceSettingsList public static readonly SettingInfo HostName = new() { Name = "ServiceControl.Audit/HostName" }; public static readonly SettingInfo LogPath = new() { Name = "ServiceControl.Audit/LogPath" }; public static readonly SettingInfo DBPath = new() { Name = "ServiceControl.Audit/DBPath" }; + public static readonly SettingInfo DatabaseConnectionString = new() { Name = "ServiceControl.Audit/Database/ConnectionString" }; public static readonly SettingInfo ForwardAuditMessages = new() { Name = "ServiceControl.Audit/ForwardAuditMessages" }; public static readonly SettingInfo TransportType = new() { Name = "ServiceControl.Audit/TransportType" }; public static readonly SettingInfo PersistenceType = new() { Name = "ServiceControl.Audit/PersistenceType" }; diff --git a/src/ServiceControlInstaller.Engine/Configuration/ServiceControl/ServiceControlAppConfig.cs b/src/ServiceControlInstaller.Engine/Configuration/ServiceControl/ServiceControlAppConfig.cs index 3f2ef78627..824996c768 100644 --- a/src/ServiceControlInstaller.Engine/Configuration/ServiceControl/ServiceControlAppConfig.cs +++ b/src/ServiceControlInstaller.Engine/Configuration/ServiceControl/ServiceControlAppConfig.cs @@ -4,6 +4,7 @@ using System.Configuration; using System.Data.Common; using System.IO; + using System.Linq; using Instances; using NuGet.Versioning; @@ -25,10 +26,8 @@ protected override void UpdateSettings() settings.Set(ServiceControlSettings.InstanceName, details.InstanceName, version); settings.Set(ServiceControlSettings.VirtualDirectory, details.VirtualDirectory); settings.Set(ServiceControlSettings.Port, details.Port.ToString()); - settings.Set(ServiceControlSettings.DatabaseMaintenancePort, details.DatabaseMaintenancePort.ToString(), version); settings.Set(ServiceControlSettings.HostName, details.HostName); settings.Set(ServiceControlSettings.LogPath, details.LogPath); - settings.Set(ServiceControlSettings.DBPath, details.DBPath); settings.Set(ServiceControlSettings.ForwardErrorMessages, details.ForwardErrorMessages.ToString(), version); settings.Set(ServiceControlSettings.TransportType, details.TransportPackage.Name, version); settings.Set(ServiceControlSettings.PersistenceType, details.PersistenceManifest.Name); // TODO: Why is it set here AND at ServiceControlInstance.ApplySettingsChanges 🤬 @@ -40,6 +39,55 @@ protected override void UpdateSettings() settings.Set(ServiceControlSettings.RemoteInstances, RemoteInstanceConverter.ToJson(details.RemoteInstances), version); settings.Set(ServiceControlSettings.EnableIntegratedServicePulse, details.EnableIntegratedServicePulse.ToString(), version); + foreach (var manifestSetting in details.PersistenceManifest.Settings) + { + if (!settings.AllKeys.Contains(manifestSetting.Name)) + { + var value = manifestSetting.DefaultValue; + + if (manifestSetting.Name == ServiceControlSettings.DBPath.Name) + { + value = details.DBPath; + } + + if (manifestSetting.Name == ServiceControlSettings.DatabaseMaintenancePort.Name) + { + value = details.DatabaseMaintenancePort.ToString(); + } + + if (manifestSetting.Name == ServiceControlSettings.DatabaseConnectionString.Name) + { + value = details.PersistenceConnectionString; + } + + // The manifest can't declare a per-instance default, and this setting is + // effectively mandatory at runtime once FileSystem storage is selected, + // even though it's marked optional here (other backends don't need it). + if (manifestSetting.Name == ServiceControlSettings.MessageBodyFileSystemStoragePath.Name && string.IsNullOrEmpty(value)) + { + value = Path.Combine(details.DBPath, "MessageBodies"); + } + + if (!string.IsNullOrEmpty(value)) + { + settings.Add(manifestSetting.Name, value); + } + } + + if (manifestSetting.Mandatory) + { + if (!settings.AllKeys.Contains(manifestSetting.Name)) + { + throw new Exception($"Mandatory setting {manifestSetting.Name} was not configured"); + } + + if (string.IsNullOrEmpty(settings[manifestSetting.Name].Value)) + { + throw new Exception($"Mandatory setting {manifestSetting.Name} can not be null or empty"); + } + } + } + // Windows services allow a maximum of 125 seconds when stopping a service. // When shutting down or restarting the OS we have no control over the // shutdown timeout. This is by the installer engine that is run _only_ on diff --git a/src/ServiceControlInstaller.Engine/Configuration/ServiceControl/ServiceControlAuditAppConfig.cs b/src/ServiceControlInstaller.Engine/Configuration/ServiceControl/ServiceControlAuditAppConfig.cs index 81386283eb..eb85e86542 100644 --- a/src/ServiceControlInstaller.Engine/Configuration/ServiceControl/ServiceControlAuditAppConfig.cs +++ b/src/ServiceControlInstaller.Engine/Configuration/ServiceControl/ServiceControlAuditAppConfig.cs @@ -52,6 +52,11 @@ protected override void UpdateSettings() value = instance.DatabaseMaintenancePort.ToString(); } + if (manifestSetting.Name == AuditInstanceSettingsList.DatabaseConnectionString.Name) + { + value = instance.PersistenceConnectionString; + } + if (!string.IsNullOrEmpty(value)) { settings.Add(manifestSetting.Name, value); diff --git a/src/ServiceControlInstaller.Engine/Configuration/ServiceControl/SettingsList.cs b/src/ServiceControlInstaller.Engine/Configuration/ServiceControl/SettingsList.cs index e209253c27..408eafc7d7 100644 --- a/src/ServiceControlInstaller.Engine/Configuration/ServiceControl/SettingsList.cs +++ b/src/ServiceControlInstaller.Engine/Configuration/ServiceControl/SettingsList.cs @@ -27,6 +27,8 @@ public static class ServiceControlSettings public static readonly SettingInfo HostName = new() { Name = "ServiceControl/HostName" }; public static readonly SettingInfo LogPath = new() { Name = "ServiceControl/LogPath" }; public static readonly SettingInfo DBPath = new() { Name = "ServiceControl/DBPath" }; + public static readonly SettingInfo DatabaseConnectionString = new() { Name = "ServiceControl/Database/ConnectionString" }; + public static readonly SettingInfo MessageBodyFileSystemStoragePath = new() { Name = "ServiceControl/MessageBody/FileSystem/StoragePath" }; public static readonly SettingInfo ForwardAuditMessages = new() { Name = "ServiceControl/ForwardAuditMessages", diff --git a/src/ServiceControlInstaller.Engine/Instances/ServiceControlAuditInstance.cs b/src/ServiceControlInstaller.Engine/Instances/ServiceControlAuditInstance.cs index e280cfaba9..a1db5d7938 100644 --- a/src/ServiceControlInstaller.Engine/Instances/ServiceControlAuditInstance.cs +++ b/src/ServiceControlInstaller.Engine/Instances/ServiceControlAuditInstance.cs @@ -48,6 +48,8 @@ public ServiceControlAuditInstance(IWindowsServiceController service) : base(ser public PersistenceManifest PersistenceManifest { get; set; } + public string PersistenceConnectionString { get; set; } + protected override void ApplySettingsChanges(KeyValueConfigurationCollection settings) { if (!ForwardErrorMessages) diff --git a/src/ServiceControlInstaller.Engine/Instances/ServiceControlInstallableBase.cs b/src/ServiceControlInstaller.Engine/Instances/ServiceControlInstallableBase.cs index d66df319ee..45757fd93c 100644 --- a/src/ServiceControlInstaller.Engine/Instances/ServiceControlInstallableBase.cs +++ b/src/ServiceControlInstaller.Engine/Instances/ServiceControlInstallableBase.cs @@ -98,6 +98,8 @@ string[] FlagFiles public string ConnectionString { get; set; } + public string PersistenceConnectionString { get; set; } + public string Name { get; set; } public string InstanceName { get; set; } diff --git a/src/ServiceControlInstaller.Engine/Instances/ServiceControlInstance.cs b/src/ServiceControlInstaller.Engine/Instances/ServiceControlInstance.cs index 34328ebe5d..592dd38383 100644 --- a/src/ServiceControlInstaller.Engine/Instances/ServiceControlInstance.cs +++ b/src/ServiceControlInstaller.Engine/Instances/ServiceControlInstance.cs @@ -54,6 +54,8 @@ public ServiceControlInstance(IWindowsServiceController service) : base(service) public PersistenceManifest PersistenceManifest { get; set; } + public string PersistenceConnectionString { get; set; } + public void AddRemoteInstance(string apiUri) { if (RemoteInstances.All(x => string.Compare(x.ApiUri, apiUri, StringComparison.InvariantCultureIgnoreCase) != 0)) diff --git a/src/ServiceControlInstaller.Engine/Instances/ServiceControlPersisters.cs b/src/ServiceControlInstaller.Engine/Instances/ServiceControlPersisters.cs index 85edb819f0..77756aaaba 100644 --- a/src/ServiceControlInstaller.Engine/Instances/ServiceControlPersisters.cs +++ b/src/ServiceControlInstaller.Engine/Instances/ServiceControlPersisters.cs @@ -1,5 +1,6 @@ namespace ServiceControlInstaller.Engine.Instances { + using System; using System.Collections.Generic; using System.Linq; using System.Reflection; @@ -7,8 +8,12 @@ public static class ServiceControlPersisters { + const string RavenDB35 = "RavenDB35"; + static readonly PersistenceManifest[] primaryPersistenceManifests; static readonly PersistenceManifest[] auditPersistenceManifests; + static readonly PersistenceManifest[] supportedPrimaryPersistenceManifests; + static readonly PersistenceManifest[] supportedAuditPersistenceManifests; static ServiceControlPersisters() { @@ -30,17 +35,30 @@ static ServiceControlPersisters() .Select(name => Load(assembly, name)) .OrderBy(m => m.Name) .ToArray(); + + // Filtered by environment variable in SCMU, needs to be available all the time for PowerShell. + // RavenDB35 is never a user choice - it's a detection-only entry for unconfigured legacy instances. + var includePersistencePicker = IncludePersistencePicker(); + supportedPrimaryPersistenceManifests = primaryPersistenceManifests + .Where(m => m.Name != RavenDB35 && (includePersistencePicker || m.Name == StorageEngineNames.RavenDB)) + .ToArray(); + supportedAuditPersistenceManifests = auditPersistenceManifests + .Where(m => m.Name != RavenDB35 && (includePersistencePicker || m.Name == StorageEngineNames.RavenDB)) + .ToArray(); } public static PersistenceManifest GetPrimaryPersistence(string name) => GetPersistenceManifest(primaryPersistenceManifests, name); public static PersistenceManifest GetAuditPersistence(string name) => GetPersistenceManifest(auditPersistenceManifests, name); + public static PersistenceManifest[] GetSupportedPrimaryManifests() => supportedPrimaryPersistenceManifests; + public static PersistenceManifest[] GetSupportedAuditManifests() => supportedAuditPersistenceManifests; + static PersistenceManifest GetPersistenceManifest(PersistenceManifest[] manifestCollection, string name) { if (string.IsNullOrEmpty(name)) { // Must always remain RavenDB35 so that SCMU understands that an instance with no configured value is an old Raven 3.5 instance - name = "RavenDB35"; + name = RavenDB35; } return manifestCollection.FirstOrDefault(m => m.IsMatch(name)) ?? new PersistenceManifest @@ -61,5 +79,27 @@ static PersistenceManifest Load(Assembly assembly, string resourceName) var manifest = JsonSerializer.Deserialize(stream); return manifest; } + + static bool IncludePersistencePicker() + { + try + { + var environmentValue = Environment.GetEnvironmentVariable("ServiceControl_IncludePersistencePicker"); + + if (environmentValue != null) + { + environmentValue = Environment.ExpandEnvironmentVariables(environmentValue); + if (bool.TryParse(environmentValue, out var enabled)) + { + return enabled; + } + } + return false; + } + catch (Exception) + { + return false; + } + } } } diff --git a/src/ServiceControlInstaller.Engine/Interfaces.cs b/src/ServiceControlInstaller.Engine/Interfaces.cs index 5d5968b7a5..0a19a30a33 100644 --- a/src/ServiceControlInstaller.Engine/Interfaces.cs +++ b/src/ServiceControlInstaller.Engine/Interfaces.cs @@ -22,6 +22,10 @@ public interface ITransportConfig public interface IPersistenceConfig { PersistenceManifest PersistenceManifest { get; } + + // Only populated when creating a new instance; existing instances read their + // connection string back from the already-written config file instead. + string PersistenceConnectionString { get; set; } } public interface IHttpInstance From f006600ad1e22f3444c47b3f0e3006b3222ac7e8 Mon Sep 17 00:00:00 2001 From: Andreas Bednarz <110360248+abparticular@users.noreply.github.com> Date: Fri, 16 Oct 2026 12:36:44 +1100 Subject: [PATCH 02/14] Default storage type picker to no selection instead of first option The combo box was pre-selecting the alphabetically-first storage type (PostgreSQL) whenever the picker is shown, instead of leaving it unselected like the transport picker does. Only auto-select when there's no real choice to make (picker hidden, RavenDB-only); otherwise leave it null so the control shows its built-in "Please Select..." placeholder. Also adds a launch profile that sets ServiceControl_IncludePersistencePicker=true for local F5 testing. Co-Authored-By: Claude Sonnet 5 --- .../Properties/launchSettings.json | 10 ++++++++++ .../UI/InstanceAdd/ServiceControlAuditInformation.cs | 8 +++++++- .../UI/InstanceAdd/ServiceControlInformation.cs | 8 +++++++- 3 files changed, 24 insertions(+), 2 deletions(-) create mode 100644 src/ServiceControl.Config/Properties/launchSettings.json diff --git a/src/ServiceControl.Config/Properties/launchSettings.json b/src/ServiceControl.Config/Properties/launchSettings.json new file mode 100644 index 0000000000..aec5f20960 --- /dev/null +++ b/src/ServiceControl.Config/Properties/launchSettings.json @@ -0,0 +1,10 @@ +{ + "profiles": { + "ServiceControl.Config": { + "commandName": "Project", + "environmentVariables": { + "ServiceControl_IncludePersistencePicker": "true" + } + } + } +} diff --git a/src/ServiceControl.Config/UI/InstanceAdd/ServiceControlAuditInformation.cs b/src/ServiceControl.Config/UI/InstanceAdd/ServiceControlAuditInformation.cs index 8fa61c2341..947aa820b5 100644 --- a/src/ServiceControl.Config/UI/InstanceAdd/ServiceControlAuditInformation.cs +++ b/src/ServiceControl.Config/UI/InstanceAdd/ServiceControlAuditInformation.cs @@ -49,7 +49,13 @@ public ServiceControlAuditInformation(ServiceControlEditorViewModel viewModelPar DatabaseMaintenancePortNumber = "44445"; EnableFullTextSearchOnBodies = EnableFullTextSearchOnBodiesOptions.First(p => p.Value); //Default to On. ViewModelParent = viewModelParent; - SelectedStorageType = StorageTypes.First(); + + // Only auto-select when there's no real choice to make (picker hidden, RavenDB-only); + // otherwise leave it unselected so the combo box shows "Please Select...", like transport. + if (StorageTypes.Count() == 1) + { + SelectedStorageType = StorageTypes.First(); + } } public ServiceControlEditorViewModel ViewModelParent { get; } diff --git a/src/ServiceControl.Config/UI/InstanceAdd/ServiceControlInformation.cs b/src/ServiceControl.Config/UI/InstanceAdd/ServiceControlInformation.cs index 7727a7834f..26ea4c12c5 100644 --- a/src/ServiceControl.Config/UI/InstanceAdd/ServiceControlInformation.cs +++ b/src/ServiceControl.Config/UI/InstanceAdd/ServiceControlInformation.cs @@ -63,7 +63,13 @@ public ServiceControlInformation(ServiceControlEditorViewModel viewModelParent) EnableFullTextSearchOnBodies = EnableFullTextSearchOnBodiesOptions.First(p => p.Value); //Default to On. EnableIntegratedServicePulse = EnableIntegratedServicePulseOptions.First(p => p.Value); //Default to On. ViewModelParent = viewModelParent; - SelectedStorageType = StorageTypes.First(); + + // Only auto-select when there's no real choice to make (picker hidden, RavenDB-only); + // otherwise leave it unselected so the combo box shows "Please Select...", like transport. + if (StorageTypes.Count() == 1) + { + SelectedStorageType = StorageTypes.First(); + } } public int MaximumErrorRetentionPeriod => SettingConstants.ErrorRetentionPeriodMaxInDays; From 6102358f0191681263bad25c35fcd60794de5dd0 Mon Sep 17 00:00:00 2001 From: Andreas Bednarz <110360248+abparticular@users.noreply.github.com> Date: Fri, 16 Oct 2026 12:36:57 +1100 Subject: [PATCH 03/14] Fix remove-instance cleanup being RavenDB-specific for Error instances GetPersistencePathsToCleanUp() on the primary (Error) instance was a hardcoded list of RavenDB app.config keys, unlike the Audit instance's already-correct implementation which reads PersistenceManifest.Settings WithPathsToCleanup. This meant checking "remove database" on a SQL Server/PostgreSQL-backed Error instance silently deleted nothing, since none of the hardcoded RavenDB keys exist in its config. Made the Error instance manifest-driven to match Audit, and hoisted PersistenceManifest onto the shared ServiceControlBaseService base class so both the engine and the UI can ask a single HasCleanableStoragePaths question regardless of instance type. Also reworded the delete-instance confirmation dialog (and the PowerShell -RemoveDB help text) away from RavenDB-specific "DB subdirectory" language: when the storage type has nothing local to clean up, the checkbox is replaced with a note that the external database is not managed by ServiceControl and must be removed manually. Dropping an external SQL Server/PostgreSQL database automatically is out of scope here - it could be shared by other instances, and there is no existing mechanism in the persistence layer for it. Co-Authored-By: Claude Sonnet 5 --- .../DeleteServiceControlInstanceCommand.cs | 2 + .../DeleteServiceControlConfirmationView.xaml | 13 ++- ...leteServiceControlConfirmationViewModel.cs | 5 + .../RemoveServiceControlAuditInstance.cs | 2 +- .../RemoveServiceControlInstance.cs | 2 +- .../Configuration/InstanceTests.cs | 102 ++++++++++++++++++ .../Instances/ServiceControlAuditInstance.cs | 2 - .../Instances/ServiceControlBaseService.cs | 5 + .../Instances/ServiceControlInstance.cs | 20 +--- 9 files changed, 132 insertions(+), 21 deletions(-) create mode 100644 src/ServiceControlInstaller.Engine.UnitTests/Configuration/InstanceTests.cs diff --git a/src/ServiceControl.Config/Commands/DeleteServiceControlInstanceCommand.cs b/src/ServiceControl.Config/Commands/DeleteServiceControlInstanceCommand.cs index 9ed943fd4e..04e80ee0c2 100644 --- a/src/ServiceControl.Config/Commands/DeleteServiceControlInstanceCommand.cs +++ b/src/ServiceControl.Config/Commands/DeleteServiceControlInstanceCommand.cs @@ -31,6 +31,8 @@ public override async Task ExecuteAsync(ServiceControlAdvancedViewModel model) var confirmation = deleteInstanceConfirmation(); confirmation.InstanceName = model.Name; + confirmation.StorageDisplayName = model.ServiceControlInstance.PersistenceManifest.DisplayName; + confirmation.HasCleanableStoragePaths = model.ServiceControlInstance.HasCleanableStoragePaths; var isConfirmed = await windowManager.ShowDialogAsync(confirmation); if (isConfirmed == true) { diff --git a/src/ServiceControl.Config/UI/DeleteInstanceConfirmation/DeleteServiceControlConfirmationView.xaml b/src/ServiceControl.Config/UI/DeleteInstanceConfirmation/DeleteServiceControlConfirmationView.xaml index cd1641f452..1ec4afd236 100644 --- a/src/ServiceControl.Config/UI/DeleteInstanceConfirmation/DeleteServiceControlConfirmationView.xaml +++ b/src/ServiceControl.Config/UI/DeleteInstanceConfirmation/DeleteServiceControlConfirmationView.xaml @@ -77,9 +77,18 @@ Grid.RowSpan="2" Margin="0,34.6,-0.4,0.6" VerticalContentAlignment="Center" - IsChecked="{Binding RemoveDatabase}"> - + IsChecked="{Binding RemoveDatabase}" + Visibility="{Binding HasCleanableStoragePaths, Converter={StaticResource boolToVis}}"> + + + + "ServiceControl"; + + public bool Exists() => true; + + public string ExePath { get; } + + public string Description { get => throw new NotImplementedException(); set => throw new NotImplementedException(); } + + public ServiceControllerStatus Status => throw new NotImplementedException(); + + public string Account => "system"; + + public string DisplayName => throw new NotImplementedException(); + + public void ChangeAccountDetails(string accountName, string serviceAccountPwd) => throw new NotImplementedException(); + public void Delete() => throw new NotImplementedException(); + public void Refresh() + { } + public void SetStartupMode(string v) => throw new NotImplementedException(); + public void Start() => throw new NotImplementedException(); + public void Stop() => throw new NotImplementedException(); + public void WaitForStatus(ServiceControllerStatus stopped, TimeSpan timeSpan) => throw new NotImplementedException(); + } + } +} diff --git a/src/ServiceControlInstaller.Engine/Instances/ServiceControlAuditInstance.cs b/src/ServiceControlInstaller.Engine/Instances/ServiceControlAuditInstance.cs index a1db5d7938..ccdbe768a4 100644 --- a/src/ServiceControlInstaller.Engine/Instances/ServiceControlAuditInstance.cs +++ b/src/ServiceControlInstaller.Engine/Instances/ServiceControlAuditInstance.cs @@ -46,8 +46,6 @@ public ServiceControlAuditInstance(IWindowsServiceController service) : base(ser public string ServiceControlQueueAddress { get; set; } - public PersistenceManifest PersistenceManifest { get; set; } - public string PersistenceConnectionString { get; set; } protected override void ApplySettingsChanges(KeyValueConfigurationCollection settings) diff --git a/src/ServiceControlInstaller.Engine/Instances/ServiceControlBaseService.cs b/src/ServiceControlInstaller.Engine/Instances/ServiceControlBaseService.cs index e2983cb02e..e3dd3341dc 100644 --- a/src/ServiceControlInstaller.Engine/Instances/ServiceControlBaseService.cs +++ b/src/ServiceControlInstaller.Engine/Instances/ServiceControlBaseService.cs @@ -40,6 +40,11 @@ protected ServiceControlBaseService(IWindowsServiceController service) public bool InMaintenanceMode { get; set; } public ReportCard ReportCard { get; set; } + public PersistenceManifest PersistenceManifest { get; set; } + + // False when this instance's storage lives outside anything ServiceControl manages locally + // (e.g. an external SQL Server/PostgreSQL database) - there's nothing on disk to clean up. + public bool HasCleanableStoragePaths => PersistenceManifest.SettingsWithPathsToCleanup.Length > 0; /// /// Raven management URL diff --git a/src/ServiceControlInstaller.Engine/Instances/ServiceControlInstance.cs b/src/ServiceControlInstaller.Engine/Instances/ServiceControlInstance.cs index 592dd38383..4a4541c358 100644 --- a/src/ServiceControlInstaller.Engine/Instances/ServiceControlInstance.cs +++ b/src/ServiceControlInstaller.Engine/Instances/ServiceControlInstance.cs @@ -52,8 +52,6 @@ public ServiceControlInstance(IWindowsServiceController service) : base(service) public List RemoteInstances { get; set; } = []; - public PersistenceManifest PersistenceManifest { get; set; } - public string PersistenceConnectionString { get; set; } public void AddRemoteInstance(string apiUri) @@ -242,26 +240,18 @@ protected override void Prepare(string zipFilePath, string destDir) protected override IEnumerable GetPersistencePathsToCleanUp() { - string[] keys = - [ - "Raven/IndexStoragePath", - "Raven/CompiledIndexCacheDirectory", - "Raven/Esent/LogsPath", - ServiceControlSettings.DBPath.Name - ]; - var settings = AppConfig.Config.AppSettings.Settings; - foreach (var key in keys) + foreach (var key in PersistenceManifest.SettingsWithPathsToCleanup) { if (!settings.AllKeys.Contains(key, StringComparer.OrdinalIgnoreCase)) { continue; } - var folderpath = settings[key].Value; - yield return folderpath.StartsWith("~") //Raven uses ~ to indicate path is relative to bin folder e.g. ~/Data/Logs - ? Path.Combine(InstallPath, folderpath.Remove(0, 1)) - : folderpath; + var folderPath = settings[key].Value; + yield return folderPath.StartsWith("~") //Raven uses ~ to indicate path is relative to bin folder e.g. ~/Data/Logs + ? Path.Combine(InstallPath, folderPath.Remove(0, 1)) + : folderPath; } } } From 02c8063eee4ddfe56f7ec021c42e8f53b6748151 Mon Sep 17 00:00:00 2001 From: Andreas Bednarz <110360248+abparticular@users.noreply.github.com> Date: Fri, 16 Oct 2026 12:55:23 +1100 Subject: [PATCH 04/14] Fix connection string box not appearing when SQL Server/PostgreSQL chosen ShowErrorStorageConnectionString/ShowAuditStorageConnectionString (and the database-path-visibility equivalents) delegated directly to a computed property on the ServiceControl/ServiceControlAudit sub-view-model. Fody's PropertyChanged weaving only infers dependencies within the same class, and the sub-view-model's own PropertyChanged events get relayed to the parent as "Error"/"Audit", which doesn't match the "Show...Error.../Show...Audit..." naming used here - so WPF never found out the value had changed, and the connection string box (and the database path fields) never showed/hid themselves after a storage type change. Fixed by computing these directly from the already-correctly-relayed ErrorSelectedStorageType/AuditSelectedStorageType properties instead, matching the existing ShowErrorForwardingQueue pattern elsewhere in this file. Added a test that asserts PropertyChanged actually fires for the properties the XAML's Visibility bindings listen to, not just that the computed values are correct - the previous tests only read values directly and couldn't catch this class of bug. Co-Authored-By: Claude Sonnet 5 --- .../ServiceControlAddScreenLoadedTests.cs | 20 +++++++++++++++++++ .../InstanceAdd/ServiceControlAddViewModel.cs | 12 +++++------ 2 files changed, 26 insertions(+), 6 deletions(-) diff --git a/src/ServiceControl.Config.Tests/ServiceControlAddScreenLoadedTests.cs b/src/ServiceControl.Config.Tests/ServiceControlAddScreenLoadedTests.cs index d6484c86e0..92236d7322 100644 --- a/src/ServiceControl.Config.Tests/ServiceControlAddScreenLoadedTests.cs +++ b/src/ServiceControl.Config.Tests/ServiceControlAddScreenLoadedTests.cs @@ -56,6 +56,26 @@ public void Storage_types_default_to_RavenDB_only_when_picker_is_not_enabled() } } + [Test] + public void Changing_storage_type_notifies_bound_visibility_properties() + { + var viewModel = new ServiceControlAddViewModel(); + var raisedPropertyNames = new System.Collections.Generic.HashSet(); + viewModel.PropertyChanged += (_, e) => raisedPropertyNames.Add(e.PropertyName); + + viewModel.ErrorSelectedStorageType = ServiceControlInstaller.Engine.Instances.ServiceControlPersisters.GetPrimaryPersistence("SQLServer"); + + using (Assert.EnterMultipleScope()) + { + // These are what the XAML's Visibility bindings actually listen to - if WPF never + // hears these changed, the connection string box silently never appears/disappears, + // even though the underlying ErrorSelectedStorageType value is correct. + Assert.That(raisedPropertyNames, Does.Contain(nameof(viewModel.ShowErrorStorageConnectionString))); + Assert.That(raisedPropertyNames, Does.Contain(nameof(viewModel.ShowErrorDatabasePathFields))); + Assert.That(viewModel.ShowErrorStorageConnectionString, Is.True); + } + } + [Test] public void Transport_connection_string_is_null() { diff --git a/src/ServiceControl.Config/UI/InstanceAdd/ServiceControlAddViewModel.cs b/src/ServiceControl.Config/UI/InstanceAdd/ServiceControlAddViewModel.cs index eebe0bcebf..a9c64636c6 100644 --- a/src/ServiceControl.Config/UI/InstanceAdd/ServiceControlAddViewModel.cs +++ b/src/ServiceControl.Config/UI/InstanceAdd/ServiceControlAddViewModel.cs @@ -214,11 +214,11 @@ public string ErrorStorageConnectionString set => ServiceControl.StorageConnectionString = value; } - public bool ShowErrorStorageTypePicker => ServiceControl.ShowStorageTypePicker; + public bool ShowErrorStorageTypePicker => ErrorStorageTypes.Count() > 1; - public bool ShowErrorStorageConnectionString => ServiceControl.ShowStorageConnectionString; + public bool ShowErrorStorageConnectionString => ErrorSelectedStorageType != null && ErrorSelectedStorageType.Settings.Any(s => s.Mandatory && s.Name.EndsWith("/ConnectionString")); - public bool ShowErrorDatabasePathFields => ServiceControl.ShowDatabasePathFields; + public bool ShowErrorDatabasePathFields => ErrorSelectedStorageType == null || ErrorSelectedStorageType.Settings.Any(s => s.Name.EndsWith("/DBPath")); public ICommand ErrorSelectDestinationPath => ServiceControl.SelectDestinationPath; @@ -401,11 +401,11 @@ public string AuditStorageConnectionString set => ServiceControlAudit.StorageConnectionString = value; } - public bool ShowAuditStorageTypePicker => ServiceControlAudit.ShowStorageTypePicker; + public bool ShowAuditStorageTypePicker => AuditStorageTypes.Count() > 1; - public bool ShowAuditStorageConnectionString => ServiceControlAudit.ShowStorageConnectionString; + public bool ShowAuditStorageConnectionString => AuditSelectedStorageType != null && AuditSelectedStorageType.Settings.Any(s => s.Mandatory && s.Name.EndsWith("/ConnectionString")); - public bool ShowAuditDatabasePathFields => ServiceControlAudit.ShowDatabasePathFields; + public bool ShowAuditDatabasePathFields => AuditSelectedStorageType == null || AuditSelectedStorageType.Settings.Any(s => s.Name.EndsWith("/DBPath")); public ICommand AuditSelectDestinationPath => ServiceControlAudit.SelectDestinationPath; From 94aa22f978247491899c1f923bced8612622d1a7 Mon Sep 17 00:00:00 2001 From: Andreas Bednarz <110360248+abparticular@users.noreply.github.com> Date: Tue, 20 Oct 2026 14:30:49 +1100 Subject: [PATCH 05/14] Show maintenance mode in SCMU only for embedded RavenDB instances Maintenance mode exists to start the embedded RavenDB server without the rest of the instance so RavenDB Studio can be used. An external RavenDB server, SQL Server, or PostgreSQL database is already reachable, so the user can stop the instance to do maintenance. The section stays visible while an instance is in maintenance mode so it can still be stopped. Co-Authored-By: Claude Opus 5.5 --- .../ServiceControlAdvancedViewModel.cs | 5 +++- .../Configuration/InstanceTests.cs | 27 +++++++++++++++++++ .../AuditInstanceSettingsList.cs | 1 + .../ServiceControl/SettingsList.cs | 1 + .../Instances/ServiceControlAuditInstance.cs | 4 +++ .../Instances/ServiceControlBaseService.cs | 11 ++++++++ .../Instances/ServiceControlInstance.cs | 4 +++ 7 files changed, 52 insertions(+), 1 deletion(-) diff --git a/src/ServiceControl.Config/UI/AdvancedOptions/ServiceControlAdvancedViewModel.cs b/src/ServiceControl.Config/UI/AdvancedOptions/ServiceControlAdvancedViewModel.cs index 638fad9e38..b0a921c104 100644 --- a/src/ServiceControl.Config/UI/AdvancedOptions/ServiceControlAdvancedViewModel.cs +++ b/src/ServiceControl.Config/UI/AdvancedOptions/ServiceControlAdvancedViewModel.cs @@ -65,7 +65,10 @@ ForceUpgradeAuditInstanceCommand forceUpgradeAuditCommand public ICommand StartServiceInMaintenanceModeCommand { get; set; } public ICommand StopMaintenanceModeCommand { get; set; } - public bool MaintenanceModeSupported => ServiceControlInstance.Version >= ServiceControlSettings.MaintenanceMode.SupportedFrom; + // Stays visible while an instance is already in maintenance mode so the user can still stop it. + public bool MaintenanceModeSupported => + ServiceControlInstance.Version >= ServiceControlSettings.MaintenanceMode.SupportedFrom + && (ServiceControlInstance.UsesEmbeddedDatabase || ServiceControlInstance.InMaintenanceMode); public ICommand DeleteCommand { get; set; } diff --git a/src/ServiceControlInstaller.Engine.UnitTests/Configuration/InstanceTests.cs b/src/ServiceControlInstaller.Engine.UnitTests/Configuration/InstanceTests.cs index c0c6952fe2..5fbb6974d8 100644 --- a/src/ServiceControlInstaller.Engine.UnitTests/Configuration/InstanceTests.cs +++ b/src/ServiceControlInstaller.Engine.UnitTests/Configuration/InstanceTests.cs @@ -4,6 +4,7 @@ namespace ServiceControlInstaller.Engine.UnitTests.Configuration using System.IO; using System.ServiceProcess; using NUnit.Framework; + using ServiceControlInstaller.Engine.Configuration.ServiceControl; using ServiceControlInstaller.Engine.Instances; using ServiceControlInstaller.Engine.Services; @@ -32,6 +33,7 @@ public void Should_remove_db_folder_on_uninstall_for_RavenDB() instance.Reload(); Assert.That(instance.HasCleanableStoragePaths, Is.True); + Assert.That(instance.UsesEmbeddedDatabase, Is.True); instance.RemoveLogsFolder(); instance.RemoveDataBaseFolder(); @@ -61,6 +63,7 @@ public void Should_not_attempt_local_cleanup_on_uninstall_for_external_storage() instance.Reload(); Assert.That(instance.HasCleanableStoragePaths, Is.False); + Assert.That(instance.UsesEmbeddedDatabase, Is.False); instance.RemoveLogsFolder(); instance.RemoveDataBaseFolder(); @@ -69,6 +72,30 @@ public void Should_not_attempt_local_cleanup_on_uninstall_for_external_storage() Assert.That(Directory.Exists(DbPath), Is.True, "Nothing local backs a SQL Server instance, so its unrelated DBPath folder must be left alone"); } + [Test] + public void Should_not_report_embedded_database_for_external_RavenDB() + { + var newInstance = ServiceControlNewInstance.CreateWithPersistence("RavenDB"); + + newInstance.InstallPath = InstallPath; + newInstance.TransportPackage = ServiceControlCoreTransports.Find("LearningTransport"); + newInstance.DBPath = DbPath; + newInstance.LogPath = LogPath; + newInstance.HostName = "localhost"; + newInstance.DatabaseMaintenancePort = 33333; + + newInstance.CopyFiles(zipResourceName); + newInstance.WriteConfigurationFile(); + + var instance = new ServiceControlInstance(new FakeWindowsServiceController(Path.Combine(InstallPath, "ServiceControl.exe"))); + instance.AppConfig.Config.AppSettings.Settings.Add(ServiceControlSettings.RavenDBConnectionString.Name, "http://ravendb.example:8080"); + instance.AppConfig.Config.Save(); + + instance.Reload(); + + Assert.That(instance.UsesEmbeddedDatabase, Is.False); + } + class FakeWindowsServiceController : IWindowsServiceController { public FakeWindowsServiceController(string exePath) diff --git a/src/ServiceControlInstaller.Engine/Configuration/ServiceControl/AuditInstanceSettingsList.cs b/src/ServiceControlInstaller.Engine/Configuration/ServiceControl/AuditInstanceSettingsList.cs index bf7ceb427a..15c28e4ce6 100644 --- a/src/ServiceControlInstaller.Engine/Configuration/ServiceControl/AuditInstanceSettingsList.cs +++ b/src/ServiceControlInstaller.Engine/Configuration/ServiceControl/AuditInstanceSettingsList.cs @@ -22,6 +22,7 @@ public static class AuditInstanceSettingsList public static readonly SettingInfo LogPath = new() { Name = "ServiceControl.Audit/LogPath" }; public static readonly SettingInfo DBPath = new() { Name = "ServiceControl.Audit/DBPath" }; public static readonly SettingInfo DatabaseConnectionString = new() { Name = "ServiceControl.Audit/Database/ConnectionString" }; + public static readonly SettingInfo RavenDBConnectionString = new() { Name = "ServiceControl.Audit/RavenDB/ConnectionString" }; public static readonly SettingInfo ForwardAuditMessages = new() { Name = "ServiceControl.Audit/ForwardAuditMessages" }; public static readonly SettingInfo TransportType = new() { Name = "ServiceControl.Audit/TransportType" }; public static readonly SettingInfo PersistenceType = new() { Name = "ServiceControl.Audit/PersistenceType" }; diff --git a/src/ServiceControlInstaller.Engine/Configuration/ServiceControl/SettingsList.cs b/src/ServiceControlInstaller.Engine/Configuration/ServiceControl/SettingsList.cs index 408eafc7d7..4ada813a94 100644 --- a/src/ServiceControlInstaller.Engine/Configuration/ServiceControl/SettingsList.cs +++ b/src/ServiceControlInstaller.Engine/Configuration/ServiceControl/SettingsList.cs @@ -28,6 +28,7 @@ public static class ServiceControlSettings public static readonly SettingInfo LogPath = new() { Name = "ServiceControl/LogPath" }; public static readonly SettingInfo DBPath = new() { Name = "ServiceControl/DBPath" }; public static readonly SettingInfo DatabaseConnectionString = new() { Name = "ServiceControl/Database/ConnectionString" }; + public static readonly SettingInfo RavenDBConnectionString = new() { Name = "ServiceControl/RavenDB/ConnectionString" }; public static readonly SettingInfo MessageBodyFileSystemStoragePath = new() { Name = "ServiceControl/MessageBody/FileSystem/StoragePath" }; public static readonly SettingInfo ForwardAuditMessages = new() { diff --git a/src/ServiceControlInstaller.Engine/Instances/ServiceControlAuditInstance.cs b/src/ServiceControlInstaller.Engine/Instances/ServiceControlAuditInstance.cs index ccdbe768a4..b1b554ba95 100644 --- a/src/ServiceControlInstaller.Engine/Instances/ServiceControlAuditInstance.cs +++ b/src/ServiceControlInstaller.Engine/Instances/ServiceControlAuditInstance.cs @@ -89,6 +89,10 @@ protected override TransportInfo DetermineTransportPackage() protected override string BaseServiceName => "ServiceControl.Audit"; + protected override SettingInfo DatabaseMaintenancePortSetting => AuditInstanceSettingsList.DatabaseMaintenancePort; + + protected override SettingInfo RavenDBConnectionStringSetting => AuditInstanceSettingsList.RavenDBConnectionString; + public override void Reload() { Service.Refresh(); diff --git a/src/ServiceControlInstaller.Engine/Instances/ServiceControlBaseService.cs b/src/ServiceControlInstaller.Engine/Instances/ServiceControlBaseService.cs index e3dd3341dc..6e97b1633c 100644 --- a/src/ServiceControlInstaller.Engine/Instances/ServiceControlBaseService.cs +++ b/src/ServiceControlInstaller.Engine/Instances/ServiceControlBaseService.cs @@ -46,6 +46,17 @@ protected ServiceControlBaseService(IWindowsServiceController service) // (e.g. an external SQL Server/PostgreSQL database) - there's nothing on disk to clean up. public bool HasCleanableStoragePaths => PersistenceManifest.SettingsWithPathsToCleanup.Length > 0; + // Only persisters that run an embedded database server declare a maintenance port. + public bool UsesEmbeddedDatabase => + PersistenceManifest?.Settings.Any(setting => setting.Name == DatabaseMaintenancePortSetting.Name) == true + && !UsesExternalRavenDB; + + bool UsesExternalRavenDB => !string.IsNullOrWhiteSpace(AppConfig?.Read(RavenDBConnectionStringSetting, null)); + + protected abstract SettingInfo DatabaseMaintenancePortSetting { get; } + + protected abstract SettingInfo RavenDBConnectionStringSetting { get; } + /// /// Raven management URL /// diff --git a/src/ServiceControlInstaller.Engine/Instances/ServiceControlInstance.cs b/src/ServiceControlInstaller.Engine/Instances/ServiceControlInstance.cs index 4a4541c358..76ca2e54f5 100644 --- a/src/ServiceControlInstaller.Engine/Instances/ServiceControlInstance.cs +++ b/src/ServiceControlInstaller.Engine/Instances/ServiceControlInstance.cs @@ -48,6 +48,10 @@ public ServiceControlInstance(IWindowsServiceController service) : base(service) protected override string BaseServiceName => "ServiceControl"; + protected override SettingInfo DatabaseMaintenancePortSetting => ServiceControlSettings.DatabaseMaintenancePort; + + protected override SettingInfo RavenDBConnectionStringSetting => ServiceControlSettings.RavenDBConnectionString; + public TimeSpan? AuditRetentionPeriod { get; set; } public List RemoteInstances { get; set; } = []; From f43e93d5ec243b2be5270e79bed5df21c64890aa Mon Sep 17 00:00:00 2001 From: Andreas Bednarz <110360248+abparticular@users.noreply.github.com> Date: Tue, 20 Oct 2026 14:31:46 +1100 Subject: [PATCH 06/14] Treat external RavenDB as unmanaged storage when removing an instance An instance with RavenDB/ConnectionString set keeps its data on a server ServiceControl doesn't manage. The remove dialog now shows the same "not managed by ServiceControl" message as SQL Server and PostgreSQL instead of the delete checkbox. RemoveDataBaseFolder also skips cleanup for unmanaged storage, so SCMU, PowerShell -RemoveDB, and unattended removal no longer delete the leftover local DBPath folder. Co-Authored-By: Claude Opus 5.5 --- .../DeleteServiceControlConfirmationView.xaml | 2 +- .../DeleteServiceControlConfirmationViewModel.cs | 1 - .../Configuration/InstanceTests.cs | 7 ++++++- .../Instances/ServiceControlBaseService.cs | 10 +++++++--- 4 files changed, 14 insertions(+), 6 deletions(-) diff --git a/src/ServiceControl.Config/UI/DeleteInstanceConfirmation/DeleteServiceControlConfirmationView.xaml b/src/ServiceControl.Config/UI/DeleteInstanceConfirmation/DeleteServiceControlConfirmationView.xaml index 1ec4afd236..06e1fd4f9d 100644 --- a/src/ServiceControl.Config/UI/DeleteInstanceConfirmation/DeleteServiceControlConfirmationView.xaml +++ b/src/ServiceControl.Config/UI/DeleteInstanceConfirmation/DeleteServiceControlConfirmationView.xaml @@ -87,7 +87,7 @@ VerticalAlignment="Center" TextWrapping="Wrap" Visibility="{Binding HasCleanableStoragePaths, Converter={StaticResource boolToVisInverted}}"> - + PersistenceManifest.SettingsWithPathsToCleanup.Length > 0; + // False when the data lives in a database server ServiceControl doesn't manage (SQL Server, PostgreSQL, external RavenDB). + public bool HasCleanableStoragePaths => PersistenceManifest?.SettingsWithPathsToCleanup.Length > 0 && !UsesExternalRavenDB; // Only persisters that run an embedded database server declare a maintenance port. public bool UsesEmbeddedDatabase => @@ -227,6 +226,11 @@ public void RemoveLogsFolder() public void RemoveDataBaseFolder() { + if (!HasCleanableStoragePaths) + { + return; + } + //Order by length descending in case they are nested paths var folders = GetPersistencePathsToCleanUp().ToList(); From 86d14be6ae6f3e442e43e1454c692efbe623cbae Mon Sep 17 00:00:00 2001 From: Andreas Bednarz <110360248+abparticular@users.noreply.github.com> Date: Tue, 20 Oct 2026 14:55:50 +1100 Subject: [PATCH 07/14] Configure message body storage explicitly for SQL Server and PostgreSQL ServiceControlNewInstance gains a MessageBodyStorage setting that selects FileSystem, AzureBlob, or S3 storage and carries the values needed to reach it. The installer writes only the selected type's settings and validates the same combinations ServiceControl checks at startup, so a bad combination fails at install time instead of when the service starts. The message body folder is no longer derived from DBPath. SQL Server and PostgreSQL instances no longer create an unused DBPath folder, and the body folder is created with the service account's access, checked for existing content, and removed on uninstall when it is used. The remove dialog now offers to delete message body files for these instances and names anything ServiceControl does not manage: the database and, for Azure Blob Storage or Amazon S3, the message bodies. Until the Add Instance wizard offers a choice, it keeps file system storage in the previous location. Co-Authored-By: Claude Opus 5.5 --- .../DeleteServiceControlInstanceCommand.cs | 33 ++++++- .../DeleteServiceControlConfirmationView.xaml | 50 +++++------ ...leteServiceControlConfirmationViewModel.cs | 8 +- .../ServiceControlAddAttachment.cs | 15 +++- .../persistence.manifest | 3 + .../persistence.manifest | 3 + ...pected_config_file.PostgreSQL.approved.txt | 4 +- ...xpected_config_file.SQLServer.approved.txt | 4 +- .../Configuration/InstanceTests.cs | 61 ++++++++++--- .../InstallationTests.cs | 12 ++- .../MessageBodyStorageValidationTests.cs | 89 +++++++++++++++++++ .../Validation/QueueValidationTests.cs | 2 + .../ServiceControl/ServiceControlAppConfig.cs | 16 ++-- .../ServiceControl/SettingsList.cs | 12 +++ .../Instances/MessageBodyStorage.cs | 72 +++++++++++++++ .../Instances/ServiceControlBaseService.cs | 4 +- .../ServiceControlInstallableBase.cs | 29 +++--- .../Instances/ServiceControlInstance.cs | 14 +++ .../Instances/ServiceControlNewInstance.cs | 21 +++++ .../Interfaces.cs | 3 + .../Validation/MessageBodyStorageValidator.cs | 79 ++++++++++++++++ .../Validation/PathsValidator.cs | 12 ++- 22 files changed, 473 insertions(+), 73 deletions(-) create mode 100644 src/ServiceControlInstaller.Engine.UnitTests/Validation/MessageBodyStorageValidationTests.cs create mode 100644 src/ServiceControlInstaller.Engine/Instances/MessageBodyStorage.cs create mode 100644 src/ServiceControlInstaller.Engine/Validation/MessageBodyStorageValidator.cs diff --git a/src/ServiceControl.Config/Commands/DeleteServiceControlInstanceCommand.cs b/src/ServiceControl.Config/Commands/DeleteServiceControlInstanceCommand.cs index 04e80ee0c2..6361f0d9a4 100644 --- a/src/ServiceControl.Config/Commands/DeleteServiceControlInstanceCommand.cs +++ b/src/ServiceControl.Config/Commands/DeleteServiceControlInstanceCommand.cs @@ -7,6 +7,7 @@ using Framework; using Framework.Commands; using Framework.Modules; + using ServiceControlInstaller.Engine.Instances; using UI.AdvancedOptions; using UI.DeleteInstanceConfirmation; @@ -31,8 +32,9 @@ public override async Task ExecuteAsync(ServiceControlAdvancedViewModel model) var confirmation = deleteInstanceConfirmation(); confirmation.InstanceName = model.Name; - confirmation.StorageDisplayName = model.ServiceControlInstance.PersistenceManifest.DisplayName; confirmation.HasCleanableStoragePaths = model.ServiceControlInstance.HasCleanableStoragePaths; + confirmation.RemoveDataLabel = model.ServiceControlInstance.UsesEmbeddedDatabase ? "Remove database files and data" : "Remove message body files"; + confirmation.UnmanagedStorageMessage = DescribeUnmanagedStorage(model.ServiceControlInstance); var isConfirmed = await windowManager.ShowDialogAsync(confirmation); if (isConfirmed == true) { @@ -54,6 +56,35 @@ public override async Task ExecuteAsync(ServiceControlAdvancedViewModel model) } } + static string DescribeUnmanagedStorage(ServiceControlBaseService instance) + { + var database = instance.UsesEmbeddedDatabase ? null : $"The {instance.PersistenceManifest.DisplayName} database"; + var bodyLocation = (instance as ServiceControlInstance)?.ConfiguredMessageBodyStorageType switch + { + MessageBodyStorageType.AzureBlob => "Azure Blob Storage", + MessageBodyStorageType.S3 => "Amazon S3", + MessageBodyStorageType.FileSystem => null, + null => null, + _ => null + }; + + var subject = (database, bodyLocation) switch + { + (null, null) => null, + (_, null) => $"{database} is", + (null, _) => $"The message bodies in {bodyLocation} are", + _ => $"{database} and the message bodies in {bodyLocation} are" + }; + + if (subject == null) + { + return null; + } + + var pronoun = bodyLocation == null ? "it" : "them"; + return $"{subject} not managed by ServiceControl and will not be removed. Delete {pronoun} manually if you no longer need {pronoun}."; + } + readonly Func deleteInstanceConfirmation; readonly IEventAggregator eventAggregator; readonly ServiceControlInstanceInstaller installer; diff --git a/src/ServiceControl.Config/UI/DeleteInstanceConfirmation/DeleteServiceControlConfirmationView.xaml b/src/ServiceControl.Config/UI/DeleteInstanceConfirmation/DeleteServiceControlConfirmationView.xaml index 06e1fd4f9d..907051a862 100644 --- a/src/ServiceControl.Config/UI/DeleteInstanceConfirmation/DeleteServiceControlConfirmationView.xaml +++ b/src/ServiceControl.Config/UI/DeleteInstanceConfirmation/DeleteServiceControlConfirmationView.xaml @@ -6,7 +6,7 @@ xmlns:mc="http://schemas.openxmlformats.org/markup-compatibility/2006" Title="DeleteInstanceConfirmationView" Width="500" - Height="370" + SizeToContent="Height" Background="{StaticResource Gray10Brush}" Foreground="{StaticResource WhiteBrush}" Padding="0" @@ -29,7 +29,7 @@ - + - - - - - - - - - - + + + + + + + + + !string.IsNullOrEmpty(UnmanagedStorageMessage); + public bool RemoveDatabase { get; set; } public bool RemoveLogs { get; set; } diff --git a/src/ServiceControl.Config/UI/InstanceAdd/ServiceControlAddAttachment.cs b/src/ServiceControl.Config/UI/InstanceAdd/ServiceControlAddAttachment.cs index 0a851a3984..6a76569cb9 100644 --- a/src/ServiceControl.Config/UI/InstanceAdd/ServiceControlAddAttachment.cs +++ b/src/ServiceControl.Config/UI/InstanceAdd/ServiceControlAddAttachment.cs @@ -1,6 +1,7 @@ namespace ServiceControl.Config.UI.InstanceAdd { using System; + using System.IO; using System.Threading; using System.Threading.Tasks; using Caliburn.Micro; @@ -61,7 +62,19 @@ async Task Add(CancellationToken cancellationToken) serviceControlNewInstance.Name = viewModel.ServiceControl.InstanceName.Replace(' ', '.'); serviceControlNewInstance.InstanceName = viewModel.ServiceControl.InstanceName.Replace(' ', '.'); serviceControlNewInstance.ServiceDescription = viewModel.ServiceControl.Description; - serviceControlNewInstance.DBPath = viewModel.ServiceControl.DatabasePath; + if (viewModel.ServiceControl.ShowDatabasePathFields) + { + serviceControlNewInstance.DBPath = viewModel.ServiceControl.DatabasePath; + } + else + { + serviceControlNewInstance.MessageBodyStorage = new MessageBodyStorage + { + StorageType = MessageBodyStorageType.FileSystem, + FileSystemStoragePath = Path.Combine(viewModel.ServiceControl.DatabasePath, "MessageBodies") + }; + } + serviceControlNewInstance.LogPath = viewModel.ServiceControl.LogPath; serviceControlNewInstance.InstallPath = viewModel.ServiceControl.DestinationPath; serviceControlNewInstance.HostName = viewModel.ServiceControl.HostName; diff --git a/src/ServiceControl.Persistence.EFCore.PostgreSql/persistence.manifest b/src/ServiceControl.Persistence.EFCore.PostgreSql/persistence.manifest index ae1e3a9851..7a38f7d0b3 100644 --- a/src/ServiceControl.Persistence.EFCore.PostgreSql/persistence.manifest +++ b/src/ServiceControl.Persistence.EFCore.PostgreSql/persistence.manifest @@ -74,5 +74,8 @@ "Name": "ServiceControl/MessageBody/MinCompressionSize", "Mandatory": false } + ], + "SettingsWithPathsToCleanup": [ + "ServiceControl/MessageBody/FileSystem/StoragePath" ] } diff --git a/src/ServiceControl.Persistence.EFCore.SqlServer/persistence.manifest b/src/ServiceControl.Persistence.EFCore.SqlServer/persistence.manifest index 2b56bc6a35..f054c4dc26 100644 --- a/src/ServiceControl.Persistence.EFCore.SqlServer/persistence.manifest +++ b/src/ServiceControl.Persistence.EFCore.SqlServer/persistence.manifest @@ -74,5 +74,8 @@ "Name": "ServiceControl/MessageBody/MinCompressionSize", "Mandatory": false } + ], + "SettingsWithPathsToCleanup": [ + "ServiceControl/MessageBody/FileSystem/StoragePath" ] } diff --git a/src/ServiceControlInstaller.Engine.UnitTests/ApprovalFiles/InstallationTests.ServiceControl_install_should_write_expected_config_file.PostgreSQL.approved.txt b/src/ServiceControlInstaller.Engine.UnitTests/ApprovalFiles/InstallationTests.ServiceControl_install_should_write_expected_config_file.PostgreSQL.approved.txt index e7aa23a337..f22dffeb38 100644 --- a/src/ServiceControlInstaller.Engine.UnitTests/ApprovalFiles/InstallationTests.ServiceControl_install_should_write_expected_config_file.PostgreSQL.approved.txt +++ b/src/ServiceControlInstaller.Engine.UnitTests/ApprovalFiles/InstallationTests.ServiceControl_install_should_write_expected_config_file.PostgreSQL.approved.txt @@ -11,10 +11,10 @@ - + value="value-not-asserted" /> + \ No newline at end of file diff --git a/src/ServiceControlInstaller.Engine.UnitTests/ApprovalFiles/InstallationTests.ServiceControl_install_should_write_expected_config_file.SQLServer.approved.txt b/src/ServiceControlInstaller.Engine.UnitTests/ApprovalFiles/InstallationTests.ServiceControl_install_should_write_expected_config_file.SQLServer.approved.txt index f5c6c1b0ba..68c60c0d3d 100644 --- a/src/ServiceControlInstaller.Engine.UnitTests/ApprovalFiles/InstallationTests.ServiceControl_install_should_write_expected_config_file.SQLServer.approved.txt +++ b/src/ServiceControlInstaller.Engine.UnitTests/ApprovalFiles/InstallationTests.ServiceControl_install_should_write_expected_config_file.SQLServer.approved.txt @@ -11,10 +11,10 @@ - + value="value-not-asserted" /> + \ No newline at end of file diff --git a/src/ServiceControlInstaller.Engine.UnitTests/Configuration/InstanceTests.cs b/src/ServiceControlInstaller.Engine.UnitTests/Configuration/InstanceTests.cs index 8410e2e67f..149f6ab048 100644 --- a/src/ServiceControlInstaller.Engine.UnitTests/Configuration/InstanceTests.cs +++ b/src/ServiceControlInstaller.Engine.UnitTests/Configuration/InstanceTests.cs @@ -43,17 +43,14 @@ public void Should_remove_db_folder_on_uninstall_for_RavenDB() } [Test] - public void Should_not_attempt_local_cleanup_on_uninstall_for_external_storage() + public void Should_remove_only_message_body_folder_on_uninstall_for_SQL_Server() { - var newInstance = ServiceControlNewInstance.CreateWithPersistence("SQLServer"); - - newInstance.InstallPath = InstallPath; - newInstance.TransportPackage = ServiceControlCoreTransports.Find("LearningTransport"); - newInstance.DBPath = DbPath; - newInstance.LogPath = LogPath; - newInstance.HostName = "localhost"; - newInstance.DatabaseMaintenancePort = 33333; - newInstance.PersistenceConnectionString = "placeholder-connection-string"; + var messageBodyPath = Path.Combine(Path.GetDirectoryName(DbPath), "bodies"); + var newInstance = CreateSqlServerInstance(new MessageBodyStorage + { + StorageType = MessageBodyStorageType.FileSystem, + FileSystemStoragePath = messageBodyPath + }); newInstance.CopyFiles(zipResourceName); newInstance.WriteConfigurationFile(); @@ -62,14 +59,52 @@ public void Should_not_attempt_local_cleanup_on_uninstall_for_external_storage() instance.Reload(); - Assert.That(instance.HasCleanableStoragePaths, Is.False); + Assert.That(Directory.Exists(DbPath), Is.False, "SQL Server instances have no local database folder"); + Assert.That(Directory.Exists(messageBodyPath), Is.True); + Assert.That(instance.HasCleanableStoragePaths, Is.True); Assert.That(instance.UsesEmbeddedDatabase, Is.False); + Assert.That(instance.ConfiguredMessageBodyStorageType, Is.EqualTo(MessageBodyStorageType.FileSystem)); instance.RemoveLogsFolder(); instance.RemoveDataBaseFolder(); - Assert.That(Directory.Exists(LogPath), Is.False, "Logs should still be removed regardless of storage type"); - Assert.That(Directory.Exists(DbPath), Is.True, "Nothing local backs a SQL Server instance, so its unrelated DBPath folder must be left alone"); + Assert.That(Directory.Exists(LogPath), Is.False); + Assert.That(Directory.Exists(messageBodyPath), Is.False); + } + + [Test] + public void Should_not_attempt_local_cleanup_on_uninstall_for_external_storage() + { + var newInstance = CreateSqlServerInstance(new MessageBodyStorage + { + StorageType = MessageBodyStorageType.S3, + S3BucketName = "bodies" + }); + + newInstance.CopyFiles(zipResourceName); + newInstance.WriteConfigurationFile(); + + var instance = new ServiceControlInstance(new FakeWindowsServiceController(Path.Combine(InstallPath, "ServiceControl.exe"))); + + instance.Reload(); + + Assert.That(instance.HasCleanableStoragePaths, Is.False); + Assert.That(instance.ConfiguredMessageBodyStorageType, Is.EqualTo(MessageBodyStorageType.S3)); + Assert.That(instance.AppConfig.Read(ServiceControlSettings.MessageBodyFileSystemStoragePath, null), Is.Null, "Only the selected body storage type's settings are written"); + } + + ServiceControlNewInstance CreateSqlServerInstance(MessageBodyStorage messageBodyStorage) + { + var newInstance = ServiceControlNewInstance.CreateWithPersistence("SQLServer"); + + newInstance.InstallPath = InstallPath; + newInstance.TransportPackage = ServiceControlCoreTransports.Find("LearningTransport"); + newInstance.LogPath = LogPath; + newInstance.HostName = "localhost"; + newInstance.PersistenceConnectionString = "placeholder-connection-string"; + newInstance.MessageBodyStorage = messageBodyStorage; + + return newInstance; } [Test] diff --git a/src/ServiceControlInstaller.Engine.UnitTests/InstallationTests.cs b/src/ServiceControlInstaller.Engine.UnitTests/InstallationTests.cs index 261370e987..12723d9788 100644 --- a/src/ServiceControlInstaller.Engine.UnitTests/InstallationTests.cs +++ b/src/ServiceControlInstaller.Engine.UnitTests/InstallationTests.cs @@ -97,6 +97,7 @@ public void ServiceControl_install_should_write_expected_config_file(string mani var installPath = Path.Combine(Path.GetTempPath(), TestContext.CurrentContext.Test.ID, "install"); var dbPath = Path.Combine(Path.GetTempPath(), TestContext.CurrentContext.Test.ID, "db"); var logPath = Path.Combine(Path.GetTempPath(), TestContext.CurrentContext.Test.ID, "log"); + var messageBodyPath = Path.Combine(Path.GetTempPath(), TestContext.CurrentContext.Test.ID, "bodies"); try { @@ -134,12 +135,21 @@ public void ServiceControl_install_should_write_expected_config_file(string mani newInstance.DatabaseMaintenancePort = 33333; newInstance.PersistenceConnectionString = ConnectionStringPlaceholder; + if (persistenceManifest.Settings.Any(setting => setting.Name == ServiceControlSettings.MessageBodyStorageType.Name)) + { + newInstance.MessageBodyStorage = new MessageBodyStorage + { + StorageType = MessageBodyStorageType.FileSystem, + FileSystemStoragePath = messageBodyPath + }; + } + newInstance.WriteConfigurationFile(); var configFile = File.ReadAllText(Path.Combine(installPath, "ServiceControl.exe.config")); var scenario = Path.GetFileName(Path.GetDirectoryName(manifestPath)); - Approver.Verify(configFile, input => input.Replace(dbPath, "value-not-asserted").Replace(logPath, "value-not-asserted"), scenario); + Approver.Verify(configFile, input => input.Replace(dbPath, "value-not-asserted").Replace(logPath, "value-not-asserted").Replace(messageBodyPath, "value-not-asserted"), scenario); } finally { diff --git a/src/ServiceControlInstaller.Engine.UnitTests/Validation/MessageBodyStorageValidationTests.cs b/src/ServiceControlInstaller.Engine.UnitTests/Validation/MessageBodyStorageValidationTests.cs new file mode 100644 index 0000000000..03c9218d01 --- /dev/null +++ b/src/ServiceControlInstaller.Engine.UnitTests/Validation/MessageBodyStorageValidationTests.cs @@ -0,0 +1,89 @@ +namespace ServiceControlInstaller.Engine.UnitTests.Validation +{ + using Engine.Validation; + using Instances; + using NUnit.Framework; + + [TestFixture] + public class MessageBodyStorageValidationTests + { + static PersistenceManifest SqlServer => ServiceControlPersisters.GetPrimaryPersistence("SQLServer"); + static PersistenceManifest RavenDB => ServiceControlPersisters.GetPrimaryPersistence("RavenDB"); + + [Test] + public void Should_reject_body_storage_for_RavenDB() + { + var storage = new MessageBodyStorage { StorageType = MessageBodyStorageType.FileSystem, FileSystemStoragePath = @"C:\bodies" }; + + Assert.Throws(() => MessageBodyStorageValidator.Validate(RavenDB, storage)); + } + + [Test] + public void Should_allow_no_body_storage_for_RavenDB() => + Assert.DoesNotThrow(() => MessageBodyStorageValidator.Validate(RavenDB, null)); + + [Test] + public void Should_require_body_storage_for_SQL_Server() => + Assert.Throws(() => MessageBodyStorageValidator.Validate(SqlServer, null)); + + [Test] + public void Should_require_path_for_file_system() + { + var storage = new MessageBodyStorage { StorageType = MessageBodyStorageType.FileSystem }; + + Assert.Throws(() => MessageBodyStorageValidator.Validate(SqlServer, storage)); + } + + [TestCase(null, null)] + [TestCase("UseDevelopmentStorage=true", "https://account.blob.core.windows.net")] + public void Should_require_exactly_one_Azure_authentication_method(string connectionString, string serviceUri) + { + var storage = new MessageBodyStorage { StorageType = MessageBodyStorageType.AzureBlob, AzureConnectionString = connectionString, AzureServiceUri = serviceUri }; + + Assert.Throws(() => MessageBodyStorageValidator.Validate(SqlServer, storage)); + } + + [Test] + public void Should_reject_relative_Azure_service_uri() + { + var storage = new MessageBodyStorage { StorageType = MessageBodyStorageType.AzureBlob, AzureServiceUri = "account.blob.core.windows.net" }; + + Assert.Throws(() => MessageBodyStorageValidator.Validate(SqlServer, storage)); + } + + [TestCase("UseDevelopmentStorage=true", null)] + [TestCase(null, "https://account.blob.core.windows.net")] + public void Should_accept_one_Azure_authentication_method(string connectionString, string serviceUri) + { + var storage = new MessageBodyStorage { StorageType = MessageBodyStorageType.AzureBlob, AzureConnectionString = connectionString, AzureServiceUri = serviceUri }; + + Assert.DoesNotThrow(() => MessageBodyStorageValidator.Validate(SqlServer, storage)); + } + + [Test] + public void Should_require_S3_bucket() + { + var storage = new MessageBodyStorage { StorageType = MessageBodyStorageType.S3 }; + + Assert.Throws(() => MessageBodyStorageValidator.Validate(SqlServer, storage)); + } + + [TestCase("key", null)] + [TestCase(null, "secret")] + public void Should_require_both_S3_keys_or_neither(string accessKeyId, string secretAccessKey) + { + var storage = new MessageBodyStorage { StorageType = MessageBodyStorageType.S3, S3BucketName = "bodies", S3AccessKeyId = accessKeyId, S3SecretAccessKey = secretAccessKey }; + + Assert.Throws(() => MessageBodyStorageValidator.Validate(SqlServer, storage)); + } + + [TestCase(null, null)] + [TestCase("key", "secret")] + public void Should_accept_S3_with_both_keys_or_neither(string accessKeyId, string secretAccessKey) + { + var storage = new MessageBodyStorage { StorageType = MessageBodyStorageType.S3, S3BucketName = "bodies", S3AccessKeyId = accessKeyId, S3SecretAccessKey = secretAccessKey }; + + Assert.DoesNotThrow(() => MessageBodyStorageValidator.Validate(SqlServer, storage)); + } + } +} diff --git a/src/ServiceControlInstaller.Engine.UnitTests/Validation/QueueValidationTests.cs b/src/ServiceControlInstaller.Engine.UnitTests/Validation/QueueValidationTests.cs index 392ccda2f0..83f203cc28 100644 --- a/src/ServiceControlInstaller.Engine.UnitTests/Validation/QueueValidationTests.cs +++ b/src/ServiceControlInstaller.Engine.UnitTests/Validation/QueueValidationTests.cs @@ -29,6 +29,8 @@ class FakeServiceControlInstance : IServiceControlInstance public List RemoteInstances { get; set; } + public MessageBodyStorage MessageBodyStorage { get; set; } + public bool EnableFullTextSearchOnBodies { get; set; } public int Port { get; set; } diff --git a/src/ServiceControlInstaller.Engine/Configuration/ServiceControl/ServiceControlAppConfig.cs b/src/ServiceControlInstaller.Engine/Configuration/ServiceControl/ServiceControlAppConfig.cs index 824996c768..3848f21675 100644 --- a/src/ServiceControlInstaller.Engine/Configuration/ServiceControl/ServiceControlAppConfig.cs +++ b/src/ServiceControlInstaller.Engine/Configuration/ServiceControl/ServiceControlAppConfig.cs @@ -39,6 +39,14 @@ protected override void UpdateSettings() settings.Set(ServiceControlSettings.RemoteInstances, RemoteInstanceConverter.ToJson(details.RemoteInstances), version); settings.Set(ServiceControlSettings.EnableIntegratedServicePulse, details.EnableIntegratedServicePulse.ToString(), version); + if (details.MessageBodyStorage != null) + { + foreach (var (setting, value) in details.MessageBodyStorage.GetSettings()) + { + settings.Set(setting, value); + } + } + foreach (var manifestSetting in details.PersistenceManifest.Settings) { if (!settings.AllKeys.Contains(manifestSetting.Name)) @@ -60,14 +68,6 @@ protected override void UpdateSettings() value = details.PersistenceConnectionString; } - // The manifest can't declare a per-instance default, and this setting is - // effectively mandatory at runtime once FileSystem storage is selected, - // even though it's marked optional here (other backends don't need it). - if (manifestSetting.Name == ServiceControlSettings.MessageBodyFileSystemStoragePath.Name && string.IsNullOrEmpty(value)) - { - value = Path.Combine(details.DBPath, "MessageBodies"); - } - if (!string.IsNullOrEmpty(value)) { settings.Add(manifestSetting.Name, value); diff --git a/src/ServiceControlInstaller.Engine/Configuration/ServiceControl/SettingsList.cs b/src/ServiceControlInstaller.Engine/Configuration/ServiceControl/SettingsList.cs index 4ada813a94..71ef01517f 100644 --- a/src/ServiceControlInstaller.Engine/Configuration/ServiceControl/SettingsList.cs +++ b/src/ServiceControlInstaller.Engine/Configuration/ServiceControl/SettingsList.cs @@ -29,7 +29,19 @@ public static class ServiceControlSettings public static readonly SettingInfo DBPath = new() { Name = "ServiceControl/DBPath" }; public static readonly SettingInfo DatabaseConnectionString = new() { Name = "ServiceControl/Database/ConnectionString" }; public static readonly SettingInfo RavenDBConnectionString = new() { Name = "ServiceControl/RavenDB/ConnectionString" }; + public static readonly SettingInfo MessageBodyStorageType = new() { Name = "ServiceControl/MessageBody/StorageType" }; public static readonly SettingInfo MessageBodyFileSystemStoragePath = new() { Name = "ServiceControl/MessageBody/FileSystem/StoragePath" }; + public static readonly SettingInfo MessageBodyAzureConnectionString = new() { Name = "ServiceControl/MessageBody/Azure/ConnectionString" }; + public static readonly SettingInfo MessageBodyAzureServiceUri = new() { Name = "ServiceControl/MessageBody/Azure/ServiceUri" }; + public static readonly SettingInfo MessageBodyAzureManagedIdentityClientId = new() { Name = "ServiceControl/MessageBody/Azure/ManagedIdentityClientId" }; + public static readonly SettingInfo MessageBodyAzureAuthorityHost = new() { Name = "ServiceControl/MessageBody/Azure/AuthorityHost" }; + public static readonly SettingInfo MessageBodyAzureContainerName = new() { Name = "ServiceControl/MessageBody/Azure/ContainerName" }; + public static readonly SettingInfo MessageBodyS3BucketName = new() { Name = "ServiceControl/MessageBody/S3/BucketName" }; + public static readonly SettingInfo MessageBodyS3KeyPrefix = new() { Name = "ServiceControl/MessageBody/S3/KeyPrefix" }; + public static readonly SettingInfo MessageBodyS3Region = new() { Name = "ServiceControl/MessageBody/S3/Region" }; + public static readonly SettingInfo MessageBodyS3ServiceUrl = new() { Name = "ServiceControl/MessageBody/S3/ServiceUrl" }; + public static readonly SettingInfo MessageBodyS3AccessKeyId = new() { Name = "ServiceControl/MessageBody/S3/AccessKeyId" }; + public static readonly SettingInfo MessageBodyS3SecretAccessKey = new() { Name = "ServiceControl/MessageBody/S3/SecretAccessKey" }; public static readonly SettingInfo ForwardAuditMessages = new() { Name = "ServiceControl/ForwardAuditMessages", diff --git a/src/ServiceControlInstaller.Engine/Instances/MessageBodyStorage.cs b/src/ServiceControlInstaller.Engine/Instances/MessageBodyStorage.cs new file mode 100644 index 0000000000..52aef360ee --- /dev/null +++ b/src/ServiceControlInstaller.Engine/Instances/MessageBodyStorage.cs @@ -0,0 +1,72 @@ +namespace ServiceControlInstaller.Engine.Instances +{ + using System.Collections.Generic; + using Configuration; + using Configuration.ServiceControl; + + // Names match ServiceControl's BodyStorageType so they are written to the config file as-is. + public enum MessageBodyStorageType + { + FileSystem, + AzureBlob, + S3 + } + + public class MessageBodyStorage + { + public MessageBodyStorageType StorageType { get; set; } + + public string FileSystemStoragePath { get; set; } + + public string AzureConnectionString { get; set; } + + public string AzureServiceUri { get; set; } + + public string AzureManagedIdentityClientId { get; set; } + + public string AzureAuthorityHost { get; set; } + + public string AzureContainerName { get; set; } + + public string S3BucketName { get; set; } + + public string S3KeyPrefix { get; set; } + + public string S3Region { get; set; } + + public string S3ServiceUrl { get; set; } + + public string S3AccessKeyId { get; set; } + + public string S3SecretAccessKey { get; set; } + + internal IEnumerable<(SettingInfo Setting, string Value)> GetSettings() + { + yield return (ServiceControlSettings.MessageBodyStorageType, StorageType.ToString()); + + switch (StorageType) + { + case MessageBodyStorageType.FileSystem: + yield return (ServiceControlSettings.MessageBodyFileSystemStoragePath, FileSystemStoragePath); + break; + case MessageBodyStorageType.AzureBlob: + yield return (ServiceControlSettings.MessageBodyAzureConnectionString, AzureConnectionString); + yield return (ServiceControlSettings.MessageBodyAzureServiceUri, AzureServiceUri); + yield return (ServiceControlSettings.MessageBodyAzureManagedIdentityClientId, AzureManagedIdentityClientId); + yield return (ServiceControlSettings.MessageBodyAzureAuthorityHost, AzureAuthorityHost); + yield return (ServiceControlSettings.MessageBodyAzureContainerName, AzureContainerName); + break; + case MessageBodyStorageType.S3: + yield return (ServiceControlSettings.MessageBodyS3BucketName, S3BucketName); + yield return (ServiceControlSettings.MessageBodyS3KeyPrefix, S3KeyPrefix); + yield return (ServiceControlSettings.MessageBodyS3Region, S3Region); + yield return (ServiceControlSettings.MessageBodyS3ServiceUrl, S3ServiceUrl); + yield return (ServiceControlSettings.MessageBodyS3AccessKeyId, S3AccessKeyId); + yield return (ServiceControlSettings.MessageBodyS3SecretAccessKey, S3SecretAccessKey); + break; + default: + break; + } + } + } +} diff --git a/src/ServiceControlInstaller.Engine/Instances/ServiceControlBaseService.cs b/src/ServiceControlInstaller.Engine/Instances/ServiceControlBaseService.cs index 34339836f1..d9eace1363 100644 --- a/src/ServiceControlInstaller.Engine/Instances/ServiceControlBaseService.cs +++ b/src/ServiceControlInstaller.Engine/Instances/ServiceControlBaseService.cs @@ -42,8 +42,8 @@ protected ServiceControlBaseService(IWindowsServiceController service) public ReportCard ReportCard { get; set; } public PersistenceManifest PersistenceManifest { get; set; } - // False when the data lives in a database server ServiceControl doesn't manage (SQL Server, PostgreSQL, external RavenDB). - public bool HasCleanableStoragePaths => PersistenceManifest?.SettingsWithPathsToCleanup.Length > 0 && !UsesExternalRavenDB; + // False when all data lives somewhere ServiceControl doesn't manage, such as a SQL Server database or an S3 bucket. + public bool HasCleanableStoragePaths => AppConfig?.Config != null && !UsesExternalRavenDB && GetPersistencePathsToCleanUp().Any(); // Only persisters that run an embedded database server declare a maintenance port. public bool UsesEmbeddedDatabase => diff --git a/src/ServiceControlInstaller.Engine/Instances/ServiceControlInstallableBase.cs b/src/ServiceControlInstaller.Engine/Instances/ServiceControlInstallableBase.cs index 45757fd93c..d2a907c9de 100644 --- a/src/ServiceControlInstaller.Engine/Instances/ServiceControlInstallableBase.cs +++ b/src/ServiceControlInstaller.Engine/Instances/ServiceControlInstallableBase.cs @@ -3,6 +3,7 @@ using System; using System.Collections.Generic; using System.IO; + using System.Linq; using System.Security.AccessControl; using System.Security.Principal; using System.Threading; @@ -52,20 +53,21 @@ public string AclMaintenanceUrl } } - string[] FlagFiles + IEnumerable FlagFiles { get { const string flagFileName = ".notconfigured"; - return - [ - Path.Combine(InstallPath, flagFileName), - Path.Combine(DBPath, flagFileName), - Path.Combine(LogPath, flagFileName) - ]; + return new[] { InstallPath, LogPath } + .Concat(DataPaths) + .Where(path => !string.IsNullOrWhiteSpace(path)) + .Select(path => Path.Combine(path, flagFileName)); } } + // Directories the service writes data to. Each is created with modify access for the service account. + protected virtual IEnumerable DataPaths => [DBPath]; + public string LogPath { get; set; } public string DBPath { get; set; } @@ -150,9 +152,9 @@ public virtual void CopyFiles(string zipResourceName) FileUtils.CreateDirectoryAndSetAcl(LogPath, modifyAccessRule); } - if (!string.IsNullOrWhiteSpace(DBPath)) + foreach (var dataPath in DataPaths.Where(path => !string.IsNullOrWhiteSpace(path))) { - FileUtils.CreateDirectoryAndSetAcl(DBPath, modifyAccessRule); + FileUtils.CreateDirectoryAndSetAcl(dataPath, modifyAccessRule); } // Mark these directories with a flag @@ -240,6 +242,7 @@ public async Task Validate(Func> promptT RunValidation(ValidateQueueNames); RunValidation(ValidateServiceAccount); RunValidation(ValidateConnectionString); + RunValidation(ValidatePersistence); } public void RunValidation(Action action) @@ -262,6 +265,10 @@ protected virtual void ValidateConnectionString() { } + protected virtual void ValidatePersistence() + { + } + protected virtual void ValidateServiceAccount() { } @@ -272,9 +279,11 @@ protected virtual void ValidateQueueNames() protected virtual Task ValidatePaths(Func> promptToProceed, CancellationToken cancellationToken = default) { - return new PathsValidator(this).RunValidation(true, promptToProceed, cancellationToken); + return new PathsValidator(this, AdditionalPaths).RunValidation(true, promptToProceed, cancellationToken); } + protected virtual IEnumerable AdditionalPaths => []; + protected virtual void ValidateMaintenancePort() { } diff --git a/src/ServiceControlInstaller.Engine/Instances/ServiceControlInstance.cs b/src/ServiceControlInstaller.Engine/Instances/ServiceControlInstance.cs index 76ca2e54f5..8bd40fbfe5 100644 --- a/src/ServiceControlInstaller.Engine/Instances/ServiceControlInstance.cs +++ b/src/ServiceControlInstaller.Engine/Instances/ServiceControlInstance.cs @@ -58,6 +58,14 @@ public ServiceControlInstance(IWindowsServiceController service) : base(service) public string PersistenceConnectionString { get; set; } + public MessageBodyStorage MessageBodyStorage => null; + + // Null for storage types that keep message bodies in the database itself. + public MessageBodyStorageType? ConfiguredMessageBodyStorageType => + Enum.TryParse(AppConfig?.Read(ServiceControlSettings.MessageBodyStorageType, null), ignoreCase: true, out var storageType) + ? storageType + : null; + public void AddRemoteInstance(string apiUri) { if (RemoteInstances.All(x => string.Compare(x.ApiUri, apiUri, StringComparison.InvariantCultureIgnoreCase) != 0)) @@ -252,6 +260,12 @@ protected override IEnumerable GetPersistencePathsToCleanUp() continue; } + if (string.Equals(key, ServiceControlSettings.MessageBodyFileSystemStoragePath.Name, StringComparison.OrdinalIgnoreCase) + && ConfiguredMessageBodyStorageType != MessageBodyStorageType.FileSystem) + { + continue; + } + var folderPath = settings[key].Value; yield return folderPath.StartsWith("~") //Raven uses ~ to indicate path is relative to bin folder e.g. ~/Data/Logs ? Path.Combine(InstallPath, folderPath.Remove(0, 1)) diff --git a/src/ServiceControlInstaller.Engine/Instances/ServiceControlNewInstance.cs b/src/ServiceControlInstaller.Engine/Instances/ServiceControlNewInstance.cs index 62fded61dd..6ae5919aaf 100644 --- a/src/ServiceControlInstaller.Engine/Instances/ServiceControlNewInstance.cs +++ b/src/ServiceControlInstaller.Engine/Instances/ServiceControlNewInstance.cs @@ -40,8 +40,24 @@ public override void WriteConfigurationFile() public PersistenceManifest PersistenceManifest { get; } + public MessageBodyStorage MessageBodyStorage { get; set; } + public override string DirectoryName => "ServiceControl"; + string MessageBodyStoragePath => MessageBodyStorage?.StorageType == MessageBodyStorageType.FileSystem ? MessageBodyStorage.FileSystemStoragePath : null; + + protected override IEnumerable DataPaths => [DBPath, MessageBodyStoragePath]; + + protected override IEnumerable AdditionalPaths => + [ + new PathInfo + { + Name = "message body path", + Path = MessageBodyStoragePath, + CheckIfEmpty = true + } + ]; + public List RemoteInstances { get; set; } = []; public void AddRemoteInstance(string apiUri) @@ -110,6 +126,11 @@ protected override void ValidateConnectionString() ConnectionStringValidator.Validate(this); } + protected override void ValidatePersistence() + { + MessageBodyStorageValidator.Validate(PersistenceManifest, MessageBodyStorage); + } + public static ServiceControlNewInstance Load(string path) { ServiceControlNewInstance instanceData; diff --git a/src/ServiceControlInstaller.Engine/Interfaces.cs b/src/ServiceControlInstaller.Engine/Interfaces.cs index 0a19a30a33..fc681f8c0e 100644 --- a/src/ServiceControlInstaller.Engine/Interfaces.cs +++ b/src/ServiceControlInstaller.Engine/Interfaces.cs @@ -117,5 +117,8 @@ public interface IServiceControlInstance : IServiceControlBaseInstance, IURLInfo TimeSpan ErrorRetentionPeriod { get; } TimeSpan? AuditRetentionPeriod { get; set; } List RemoteInstances { get; } + + // Only populated when creating a new instance, like PersistenceConnectionString. + MessageBodyStorage MessageBodyStorage { get; } } } \ No newline at end of file diff --git a/src/ServiceControlInstaller.Engine/Validation/MessageBodyStorageValidator.cs b/src/ServiceControlInstaller.Engine/Validation/MessageBodyStorageValidator.cs new file mode 100644 index 0000000000..327af876da --- /dev/null +++ b/src/ServiceControlInstaller.Engine/Validation/MessageBodyStorageValidator.cs @@ -0,0 +1,79 @@ +namespace ServiceControlInstaller.Engine.Validation +{ + using System; + using System.Linq; + using Configuration.ServiceControl; + using Instances; + + // Mirrors the checks ServiceControl runs at startup, so a bad combination fails at install time instead. + static class MessageBodyStorageValidator + { + public static void Validate(PersistenceManifest manifest, MessageBodyStorage storage) + { + var supportsBodyStorage = manifest.Settings.Any(s => s.Name == ServiceControlSettings.MessageBodyStorageType.Name); + + if (!supportsBodyStorage) + { + if (storage != null) + { + throw new EngineValidationException($"The {manifest.DisplayName} storage type does not support configuring message body storage."); + } + + return; + } + + if (storage == null) + { + throw new EngineValidationException($"Message body storage must be configured for the {manifest.DisplayName} storage type."); + } + + switch (storage.StorageType) + { + case MessageBodyStorageType.FileSystem: + if (IsEmpty(storage.FileSystemStoragePath)) + { + throw new EngineValidationException("A message body path is required when message bodies are stored on the file system."); + } + + break; + case MessageBodyStorageType.AzureBlob: + var hasConnectionString = !IsEmpty(storage.AzureConnectionString); + var hasServiceUri = !IsEmpty(storage.AzureServiceUri); + + if (hasConnectionString == hasServiceUri) + { + throw new EngineValidationException("Azure Blob Storage requires either a connection string or a service URI for managed identity, but not both."); + } + + CheckAbsoluteUri(storage.AzureServiceUri, "Azure Blob Storage service URI"); + CheckAbsoluteUri(storage.AzureAuthorityHost, "Azure authority host"); + break; + case MessageBodyStorageType.S3: + if (IsEmpty(storage.S3BucketName)) + { + throw new EngineValidationException("An S3 bucket name is required when message bodies are stored in Amazon S3."); + } + + if (IsEmpty(storage.S3AccessKeyId) != IsEmpty(storage.S3SecretAccessKey)) + { + throw new EngineValidationException("Specify both the S3 access key ID and secret access key, or neither to use the default AWS credentials."); + } + + CheckAbsoluteUri(storage.S3ServiceUrl, "S3 service URL"); + break; + default: + throw new EngineValidationException($"Message body storage type '{storage.StorageType}' is not supported."); + } + } + + static void CheckAbsoluteUri(string value, string name) + { + if (!IsEmpty(value) && !Uri.TryCreate(value, UriKind.Absolute, out _)) + { + throw new EngineValidationException($"The {name} '{value}' is not a valid absolute URI."); + } + } + + static bool IsEmpty(string value) => string.IsNullOrWhiteSpace(value); + } +} diff --git a/src/ServiceControlInstaller.Engine/Validation/PathsValidator.cs b/src/ServiceControlInstaller.Engine/Validation/PathsValidator.cs index edfa7a5c5f..e041dfcea9 100644 --- a/src/ServiceControlInstaller.Engine/Validation/PathsValidator.cs +++ b/src/ServiceControlInstaller.Engine/Validation/PathsValidator.cs @@ -28,7 +28,11 @@ public PathsValidator(IServicePaths instance) paths = pathList.Where(p => !string.IsNullOrWhiteSpace(p.Path)).ToList(); } - public PathsValidator(IServiceControlPaths instance) + public PathsValidator(IServiceControlPaths instance) : this(instance, []) + { + } + + public PathsValidator(IServiceControlPaths instance, IEnumerable additionalPaths) { var pathList = new List { @@ -50,6 +54,12 @@ public PathsValidator(IServiceControlPaths instance) CheckIfEmpty = true } }; + pathList.AddRange(additionalPaths.Select(p => new PathInfo + { + Name = p.Name, + Path = Environment.ExpandEnvironmentVariables(p.Path ?? string.Empty), + CheckIfEmpty = p.CheckIfEmpty + })); paths = pathList.Where(p => !string.IsNullOrWhiteSpace(p.Path)).ToList(); } From 102133cd06622ec5f34a4c22b6f3869f89eff934 Mon Sep 17 00:00:00 2001 From: Andreas Bednarz <110360248+abparticular@users.noreply.github.com> Date: Tue, 20 Oct 2026 14:59:33 +1100 Subject: [PATCH 08/14] Let the Add Instance wizard choose where message bodies are stored For SQL Server and PostgreSQL Error instances, the wizard now offers a message body storage choice of file system, Azure Blob Storage, or Amazon S3, and shows the settings needed to reach the selected storage: - File system: the message body path, defaulting to a MessageBodies folder next to the instance's DB and Logs folders. - Azure Blob Storage: a connection string, or a blob service URI with an optional managed identity client ID and authority host, plus the container name. - Amazon S3: bucket name, key prefix, region, service URL for S3-compatible storage, and an optional access key pair. Size and limit settings such as compression size and the free disk space threshold remain config-file only. Co-Authored-By: Claude Opus 5.5 --- .../ServiceControlAddScreenLoadedTests.cs | 79 +++++++++++++ .../AddErrorInstanceValidationTests.cs | 85 ++++++++++++++ .../InstanceAdd/MessageBodyStorageOption.cs | 16 +++ .../ServiceControlAddAttachment.cs | 16 +-- .../UI/InstanceAdd/ServiceControlAddView.xaml | 65 +++++++++++ .../InstanceAdd/ServiceControlAddViewModel.cs | 105 ++++++++++++++++++ .../ServiceControlAddViewModelValidator.cs | 46 ++++++++ .../InstanceAdd/ServiceControlInformation.cs | 94 ++++++++++++++++ .../Validation/Validations.cs | 4 + 9 files changed, 496 insertions(+), 14 deletions(-) create mode 100644 src/ServiceControl.Config/UI/InstanceAdd/MessageBodyStorageOption.cs diff --git a/src/ServiceControl.Config.Tests/ServiceControlAddScreenLoadedTests.cs b/src/ServiceControl.Config.Tests/ServiceControlAddScreenLoadedTests.cs index 92236d7322..afc775cf95 100644 --- a/src/ServiceControl.Config.Tests/ServiceControlAddScreenLoadedTests.cs +++ b/src/ServiceControl.Config.Tests/ServiceControlAddScreenLoadedTests.cs @@ -76,6 +76,85 @@ public void Changing_storage_type_notifies_bound_visibility_properties() } } + [Test] + public void Message_body_storage_is_shown_only_for_storage_types_that_support_it() + { + var viewModel = new ServiceControlAddViewModel(); + + viewModel.ErrorSelectedStorageType = ServiceControlInstaller.Engine.Instances.ServiceControlPersisters.GetPrimaryPersistence("RavenDB"); + Assert.That(viewModel.ShowErrorMessageBodyStorage, Is.False); + + viewModel.ErrorSelectedStorageType = ServiceControlInstaller.Engine.Instances.ServiceControlPersisters.GetPrimaryPersistence("SQLServer"); + + using (Assert.EnterMultipleScope()) + { + Assert.That(viewModel.ShowErrorMessageBodyStorage, Is.True); + Assert.That(viewModel.ErrorSelectedMessageBodyStorageType.Value, Is.EqualTo(ServiceControlInstaller.Engine.Instances.MessageBodyStorageType.FileSystem)); + Assert.That(viewModel.ShowErrorMessageBodyPath, Is.True); + Assert.That(viewModel.ErrorMessageBodyPath, Does.StartWith(programDataPath).And.EndWith("MessageBodies")); + } + } + + [Test] + public void Changing_message_body_storage_type_notifies_bound_visibility_properties() + { + var viewModel = new ServiceControlAddViewModel + { + ErrorSelectedStorageType = ServiceControlInstaller.Engine.Instances.ServiceControlPersisters.GetPrimaryPersistence("SQLServer") + }; + var raisedPropertyNames = new System.Collections.Generic.HashSet(); + viewModel.PropertyChanged += (_, e) => raisedPropertyNames.Add(e.PropertyName); + + viewModel.ErrorSelectedMessageBodyStorageType = viewModel.ErrorMessageBodyStorageTypes.Single(o => o.Value == ServiceControlInstaller.Engine.Instances.MessageBodyStorageType.AzureBlob); + viewModel.ErrorSelectedAzureAuthentication = viewModel.ErrorAzureAuthenticationOptions.Single(o => o.UsesManagedIdentity); + + using (Assert.EnterMultipleScope()) + { + Assert.That(raisedPropertyNames, Does.Contain(nameof(viewModel.ShowErrorMessageBodyPath))); + Assert.That(raisedPropertyNames, Does.Contain(nameof(viewModel.ShowErrorAzureBlobFields))); + Assert.That(raisedPropertyNames, Does.Contain(nameof(viewModel.ShowErrorAzureManagedIdentity))); + Assert.That(raisedPropertyNames, Does.Contain(nameof(viewModel.ShowErrorAzureConnectionString))); + Assert.That(viewModel.ShowErrorMessageBodyPath, Is.False); + Assert.That(viewModel.ShowErrorAzureManagedIdentity, Is.True); + Assert.That(viewModel.ShowErrorAzureConnectionString, Is.False); + } + } + + [Test] + public void Message_body_storage_only_includes_values_for_the_selected_options() + { + var viewModel = new ServiceControlAddViewModel + { + ErrorSelectedStorageType = ServiceControlInstaller.Engine.Instances.ServiceControlPersisters.GetPrimaryPersistence("SQLServer"), + ErrorAzureConnectionString = "UseDevelopmentStorage=true", + ErrorAzureServiceUri = "https://account.blob.core.windows.net", + ErrorS3BucketName = "bodies" + }; + viewModel.ErrorSelectedMessageBodyStorageType = viewModel.ErrorMessageBodyStorageTypes.Single(o => o.Value == ServiceControlInstaller.Engine.Instances.MessageBodyStorageType.AzureBlob); + viewModel.ErrorSelectedAzureAuthentication = viewModel.ErrorAzureAuthenticationOptions.Single(o => o.UsesManagedIdentity); + + var storage = viewModel.ServiceControl.CreateMessageBodyStorage(); + + using (Assert.EnterMultipleScope()) + { + Assert.That(storage.StorageType, Is.EqualTo(ServiceControlInstaller.Engine.Instances.MessageBodyStorageType.AzureBlob)); + Assert.That(storage.AzureServiceUri, Is.EqualTo("https://account.blob.core.windows.net")); + Assert.That(storage.AzureConnectionString, Is.Null, "The connection string belongs to the authentication option that is not selected"); + Assert.That(storage.S3BucketName, Is.Null); + } + } + + [Test] + public void Message_body_storage_is_not_created_for_RavenDB() + { + var viewModel = new ServiceControlAddViewModel + { + ErrorSelectedStorageType = ServiceControlInstaller.Engine.Instances.ServiceControlPersisters.GetPrimaryPersistence("RavenDB") + }; + + Assert.That(viewModel.ServiceControl.CreateMessageBodyStorage(), Is.Null); + } + [Test] public void Transport_connection_string_is_null() { diff --git a/src/ServiceControl.Config.Tests/Validation/AddErrorInstanceValidationTests.cs b/src/ServiceControl.Config.Tests/Validation/AddErrorInstanceValidationTests.cs index 75b4058e2d..adcf2c6a60 100644 --- a/src/ServiceControl.Config.Tests/Validation/AddErrorInstanceValidationTests.cs +++ b/src/ServiceControl.Config.Tests/Validation/AddErrorInstanceValidationTests.cs @@ -188,6 +188,91 @@ public void Storage_connection_string_cannot_be_empty_for_storage_types_that_req Assert.That(errors, Is.Not.Empty); } + [Test] + public void Message_body_path_cannot_be_empty_for_file_system_storage() + { + var viewModel = new ServiceControlAddViewModel + { + InstallErrorInstance = true, + ErrorSelectedStorageType = ServiceControlPersisters.GetPrimaryPersistence("SQLServer"), + SubmitAttempted = true + }; + viewModel.ErrorMessageBodyPath = string.Empty; + + var errors = GetNotifyErrorInfo(viewModel).GetErrors(nameof(viewModel.ErrorMessageBodyPath)); + + Assert.That(errors, Is.Not.Empty); + } + + [Test] + public void Azure_service_uri_must_be_absolute() + { + var viewModel = new ServiceControlAddViewModel + { + InstallErrorInstance = true, + ErrorSelectedStorageType = ServiceControlPersisters.GetPrimaryPersistence("SQLServer") + }; + viewModel.ErrorSelectedMessageBodyStorageType = viewModel.ErrorMessageBodyStorageTypes.Single(o => o.Value == MessageBodyStorageType.AzureBlob); + viewModel.ErrorSelectedAzureAuthentication = viewModel.ErrorAzureAuthenticationOptions.Single(o => o.UsesManagedIdentity); + viewModel.ErrorAzureServiceUri = "account.blob.core.windows.net"; + + var errors = GetNotifyErrorInfo(viewModel).GetErrors(nameof(viewModel.ErrorAzureServiceUri)); + + Assert.That(errors, Is.Not.Empty); + } + + [Test] + public void Azure_connection_string_is_required_when_selected() + { + var viewModel = new ServiceControlAddViewModel + { + InstallErrorInstance = true, + ErrorSelectedStorageType = ServiceControlPersisters.GetPrimaryPersistence("SQLServer"), + SubmitAttempted = true + }; + viewModel.ErrorSelectedMessageBodyStorageType = viewModel.ErrorMessageBodyStorageTypes.Single(o => o.Value == MessageBodyStorageType.AzureBlob); + viewModel.ErrorAzureConnectionString = string.Empty; + + var errors = GetNotifyErrorInfo(viewModel).GetErrors(nameof(viewModel.ErrorAzureConnectionString)); + + Assert.That(errors, Is.Not.Empty); + } + + [Test] + public void S3_bucket_is_required() + { + var viewModel = new ServiceControlAddViewModel + { + InstallErrorInstance = true, + ErrorSelectedStorageType = ServiceControlPersisters.GetPrimaryPersistence("SQLServer"), + SubmitAttempted = true + }; + viewModel.ErrorSelectedMessageBodyStorageType = viewModel.ErrorMessageBodyStorageTypes.Single(o => o.Value == MessageBodyStorageType.S3); + viewModel.ErrorS3BucketName = string.Empty; + + var errors = GetNotifyErrorInfo(viewModel).GetErrors(nameof(viewModel.ErrorS3BucketName)); + + Assert.That(errors, Is.Not.Empty); + } + + [Test] + public void S3_secret_access_key_is_required_with_an_access_key_id() + { + var viewModel = new ServiceControlAddViewModel + { + InstallErrorInstance = true, + ErrorSelectedStorageType = ServiceControlPersisters.GetPrimaryPersistence("SQLServer"), + SubmitAttempted = true + }; + viewModel.ErrorSelectedMessageBodyStorageType = viewModel.ErrorMessageBodyStorageTypes.Single(o => o.Value == MessageBodyStorageType.S3); + viewModel.ErrorS3AccessKeyId = "key"; + viewModel.ErrorS3SecretAccessKey = string.Empty; + + var errors = GetNotifyErrorInfo(viewModel).GetErrors(nameof(viewModel.ErrorS3SecretAccessKey)); + + Assert.That(errors, Is.Not.Empty); + } + [Test] public void Storage_connection_string_can_be_empty_for_RavenDB_when_adding_error_instance() { diff --git a/src/ServiceControl.Config/UI/InstanceAdd/MessageBodyStorageOption.cs b/src/ServiceControl.Config/UI/InstanceAdd/MessageBodyStorageOption.cs new file mode 100644 index 0000000000..20a9e91bfb --- /dev/null +++ b/src/ServiceControl.Config/UI/InstanceAdd/MessageBodyStorageOption.cs @@ -0,0 +1,16 @@ +namespace ServiceControl.Config.UI.InstanceAdd +{ + using ServiceControlInstaller.Engine.Instances; + + public class MessageBodyStorageOption + { + public string Name { get; set; } + public MessageBodyStorageType Value { get; set; } + } + + public class AzureAuthenticationOption + { + public string Name { get; set; } + public bool UsesManagedIdentity { get; set; } + } +} diff --git a/src/ServiceControl.Config/UI/InstanceAdd/ServiceControlAddAttachment.cs b/src/ServiceControl.Config/UI/InstanceAdd/ServiceControlAddAttachment.cs index 6a76569cb9..ab4f936ed6 100644 --- a/src/ServiceControl.Config/UI/InstanceAdd/ServiceControlAddAttachment.cs +++ b/src/ServiceControl.Config/UI/InstanceAdd/ServiceControlAddAttachment.cs @@ -1,7 +1,6 @@ namespace ServiceControl.Config.UI.InstanceAdd { using System; - using System.IO; using System.Threading; using System.Threading.Tasks; using Caliburn.Micro; @@ -62,19 +61,8 @@ async Task Add(CancellationToken cancellationToken) serviceControlNewInstance.Name = viewModel.ServiceControl.InstanceName.Replace(' ', '.'); serviceControlNewInstance.InstanceName = viewModel.ServiceControl.InstanceName.Replace(' ', '.'); serviceControlNewInstance.ServiceDescription = viewModel.ServiceControl.Description; - if (viewModel.ServiceControl.ShowDatabasePathFields) - { - serviceControlNewInstance.DBPath = viewModel.ServiceControl.DatabasePath; - } - else - { - serviceControlNewInstance.MessageBodyStorage = new MessageBodyStorage - { - StorageType = MessageBodyStorageType.FileSystem, - FileSystemStoragePath = Path.Combine(viewModel.ServiceControl.DatabasePath, "MessageBodies") - }; - } - + serviceControlNewInstance.DBPath = viewModel.ServiceControl.ShowDatabasePathFields ? viewModel.ServiceControl.DatabasePath : null; + serviceControlNewInstance.MessageBodyStorage = viewModel.ServiceControl.CreateMessageBodyStorage(); serviceControlNewInstance.LogPath = viewModel.ServiceControl.LogPath; serviceControlNewInstance.InstallPath = viewModel.ServiceControl.DestinationPath; serviceControlNewInstance.HostName = viewModel.ServiceControl.HostName; diff --git a/src/ServiceControl.Config/UI/InstanceAdd/ServiceControlAddView.xaml b/src/ServiceControl.Config/UI/InstanceAdd/ServiceControlAddView.xaml index 218c7a34c8..58d382ca64 100644 --- a/src/ServiceControl.Config/UI/InstanceAdd/ServiceControlAddView.xaml +++ b/src/ServiceControl.Config/UI/InstanceAdd/ServiceControlAddView.xaml @@ -159,6 +159,71 @@ TextWrapping="Wrap" Visibility="{Binding ShowErrorStorageConnectionString, Converter={StaticResource boolToVis}}" /> + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/src/ServiceControl.Config/UI/InstanceAdd/ServiceControlAddViewModel.cs b/src/ServiceControl.Config/UI/InstanceAdd/ServiceControlAddViewModel.cs index a9c64636c6..fe07927ad7 100644 --- a/src/ServiceControl.Config/UI/InstanceAdd/ServiceControlAddViewModel.cs +++ b/src/ServiceControl.Config/UI/InstanceAdd/ServiceControlAddViewModel.cs @@ -8,6 +8,7 @@ namespace ServiceControl.Config.UI.InstanceAdd using System.Windows.Input; using PropertyChanged; using ServiceControl.Config.Extensions; + using ServiceControlInstaller.Engine.Configuration.ServiceControl; using ServiceControlInstaller.Engine.Instances; using Validar; using Xaml.Controls; @@ -127,6 +128,8 @@ protected void OnErrorInstanceNameChanged() "Particular Software", ErrorInstanceName); ErrorDatabasePath = Path.Combine(Environment.GetFolderPath(Environment.SpecialFolder.CommonApplicationData), "Particular", "ServiceControl", ErrorInstanceName, "DB"); + ErrorMessageBodyPath = Path.Combine(Environment.GetFolderPath(Environment.SpecialFolder.CommonApplicationData), + "Particular", "ServiceControl", ErrorInstanceName, "MessageBodies"); ErrorLogPath = Path.Combine(Environment.GetFolderPath(Environment.SpecialFolder.CommonApplicationData), "Particular", "ServiceControl", ErrorInstanceName, "Logs"); } @@ -220,6 +223,108 @@ public string ErrorStorageConnectionString public bool ShowErrorDatabasePathFields => ErrorSelectedStorageType == null || ErrorSelectedStorageType.Settings.Any(s => s.Name.EndsWith("/DBPath")); + public bool ShowErrorMessageBodyStorage => ErrorSelectedStorageType?.Settings.Any(s => s.Name == ServiceControlSettings.MessageBodyStorageType.Name) == true; + + public IEnumerable ErrorMessageBodyStorageTypes => ServiceControl.MessageBodyStorageTypes; + + public MessageBodyStorageOption ErrorSelectedMessageBodyStorageType + { + get => ServiceControl.SelectedMessageBodyStorageType; + set => ServiceControl.SelectedMessageBodyStorageType = value; + } + + public bool ShowErrorMessageBodyPath => ShowErrorMessageBodyStorage && ErrorSelectedMessageBodyStorageType?.Value == MessageBodyStorageType.FileSystem; + + public bool ShowErrorAzureBlobFields => ShowErrorMessageBodyStorage && ErrorSelectedMessageBodyStorageType?.Value == MessageBodyStorageType.AzureBlob; + + public bool ShowErrorS3Fields => ShowErrorMessageBodyStorage && ErrorSelectedMessageBodyStorageType?.Value == MessageBodyStorageType.S3; + + public ICommand ErrorSelectMessageBodyPath => ServiceControl.SelectMessageBodyPath; + + public string ErrorMessageBodyPath + { + get => ServiceControl.MessageBodyPath; + set => ServiceControl.MessageBodyPath = value.SanitizeFilePath(); + } + + public IEnumerable ErrorAzureAuthenticationOptions => ServiceControl.AzureAuthenticationOptions; + + public AzureAuthenticationOption ErrorSelectedAzureAuthentication + { + get => ServiceControl.SelectedAzureAuthentication; + set => ServiceControl.SelectedAzureAuthentication = value; + } + + public bool ShowErrorAzureConnectionString => ShowErrorAzureBlobFields && ErrorSelectedAzureAuthentication?.UsesManagedIdentity == false; + + public bool ShowErrorAzureManagedIdentity => ShowErrorAzureBlobFields && ErrorSelectedAzureAuthentication?.UsesManagedIdentity == true; + + public string ErrorAzureConnectionString + { + get => ServiceControl.AzureConnectionString; + set => ServiceControl.AzureConnectionString = value; + } + + public string ErrorAzureServiceUri + { + get => ServiceControl.AzureServiceUri; + set => ServiceControl.AzureServiceUri = value; + } + + public string ErrorAzureManagedIdentityClientId + { + get => ServiceControl.AzureManagedIdentityClientId; + set => ServiceControl.AzureManagedIdentityClientId = value; + } + + public string ErrorAzureAuthorityHost + { + get => ServiceControl.AzureAuthorityHost; + set => ServiceControl.AzureAuthorityHost = value; + } + + public string ErrorAzureContainerName + { + get => ServiceControl.AzureContainerName; + set => ServiceControl.AzureContainerName = value; + } + + public string ErrorS3BucketName + { + get => ServiceControl.S3BucketName; + set => ServiceControl.S3BucketName = value; + } + + public string ErrorS3KeyPrefix + { + get => ServiceControl.S3KeyPrefix; + set => ServiceControl.S3KeyPrefix = value; + } + + public string ErrorS3Region + { + get => ServiceControl.S3Region; + set => ServiceControl.S3Region = value; + } + + public string ErrorS3ServiceUrl + { + get => ServiceControl.S3ServiceUrl; + set => ServiceControl.S3ServiceUrl = value; + } + + public string ErrorS3AccessKeyId + { + get => ServiceControl.S3AccessKeyId; + set => ServiceControl.S3AccessKeyId = value; + } + + public string ErrorS3SecretAccessKey + { + get => ServiceControl.S3SecretAccessKey; + set => ServiceControl.S3SecretAccessKey = value; + } + public ICommand ErrorSelectDestinationPath => ServiceControl.SelectDestinationPath; public string ErrorDestinationPath diff --git a/src/ServiceControl.Config/UI/InstanceAdd/ServiceControlAddViewModelValidator.cs b/src/ServiceControl.Config/UI/InstanceAdd/ServiceControlAddViewModelValidator.cs index d4d49ec01e..ccced949aa 100644 --- a/src/ServiceControl.Config/UI/InstanceAdd/ServiceControlAddViewModelValidator.cs +++ b/src/ServiceControl.Config/UI/InstanceAdd/ServiceControlAddViewModelValidator.cs @@ -55,6 +55,50 @@ public ServiceControlAddViewModelValidator() .WithMessage(Validation.Validations.MSG_THIS_STORAGE_TYPE_REQUIRES_A_CONNECTION_STRING) .When(x => x.InstallErrorInstance && x.ShowErrorStorageConnectionString && x.SubmitAttempted); + RuleFor(x => x.ErrorMessageBodyPath) + .NotEmpty() + .ValidPath() + .MustNotBeIn(x => Validations.UsedPaths(x.ErrorInstanceName)) + .WithMessage(string.Format(Validation.Validations.MSG_MUST_BE_UNIQUE, "Message Body Paths")) + .When(x => x.InstallErrorInstance && x.ShowErrorMessageBodyPath); + + RuleFor(x => x.ErrorAzureConnectionString) + .NotEmpty() + .When(x => x.InstallErrorInstance && x.ShowErrorAzureConnectionString && x.SubmitAttempted); + + RuleFor(x => x.ErrorAzureServiceUri) + .NotEmpty() + .When(x => x.InstallErrorInstance && x.ShowErrorAzureManagedIdentity && x.SubmitAttempted); + + RuleFor(x => x.ErrorAzureServiceUri) + .Must(BeEmptyOrAbsoluteUri) + .WithMessage(Validation.Validations.MSG_MUST_BE_ABSOLUTE_URI) + .When(x => x.InstallErrorInstance && x.ShowErrorAzureManagedIdentity); + + RuleFor(x => x.ErrorAzureAuthorityHost) + .Must(BeEmptyOrAbsoluteUri) + .WithMessage(Validation.Validations.MSG_MUST_BE_ABSOLUTE_URI) + .When(x => x.InstallErrorInstance && x.ShowErrorAzureManagedIdentity); + + RuleFor(x => x.ErrorS3BucketName) + .NotEmpty() + .When(x => x.InstallErrorInstance && x.ShowErrorS3Fields && x.SubmitAttempted); + + RuleFor(x => x.ErrorS3ServiceUrl) + .Must(BeEmptyOrAbsoluteUri) + .WithMessage(Validation.Validations.MSG_MUST_BE_ABSOLUTE_URI) + .When(x => x.InstallErrorInstance && x.ShowErrorS3Fields); + + RuleFor(x => x.ErrorS3AccessKeyId) + .NotEmpty() + .WithMessage(Validation.Validations.MSG_S3_KEYS_GO_TOGETHER) + .When(x => x.InstallErrorInstance && x.ShowErrorS3Fields && x.SubmitAttempted && !string.IsNullOrWhiteSpace(x.ErrorS3SecretAccessKey)); + + RuleFor(x => x.ErrorS3SecretAccessKey) + .NotEmpty() + .WithMessage(Validation.Validations.MSG_S3_KEYS_GO_TOGETHER) + .When(x => x.InstallErrorInstance && x.ShowErrorS3Fields && x.SubmitAttempted && !string.IsNullOrWhiteSpace(x.ErrorS3AccessKeyId)); + RuleFor(x => x.ErrorServiceAccount) .NotEmpty() .When(x => x.InstallErrorInstance && x.SubmitAttempted); @@ -266,5 +310,7 @@ public ServiceControlAddViewModelValidator() .When(x => x.InstallAuditInstance) .When(x => x.AuditForwarding?.Value ?? false); } + + static bool BeEmptyOrAbsoluteUri(string value) => string.IsNullOrWhiteSpace(value) || System.Uri.TryCreate(value, System.UriKind.Absolute, out _); } } \ No newline at end of file diff --git a/src/ServiceControl.Config/UI/InstanceAdd/ServiceControlInformation.cs b/src/ServiceControl.Config/UI/InstanceAdd/ServiceControlInformation.cs index 26ea4c12c5..3cc1b6d215 100644 --- a/src/ServiceControl.Config/UI/InstanceAdd/ServiceControlInformation.cs +++ b/src/ServiceControl.Config/UI/InstanceAdd/ServiceControlInformation.cs @@ -3,6 +3,9 @@ using System; using System.Collections.Generic; using System.Linq; +using System.Windows.Input; +using Commands; +using Extensions; using ServiceControlInstaller.Engine.Configuration.ServiceControl; using ServiceControlInstaller.Engine.Instances; using SharedInstanceEditor; @@ -70,6 +73,10 @@ public ServiceControlInformation(ServiceControlEditorViewModel viewModelParent) { SelectedStorageType = StorageTypes.First(); } + + SelectedMessageBodyStorageType = MessageBodyStorageTypes.First(); + SelectedAzureAuthentication = AzureAuthenticationOptions.First(); + SelectMessageBodyPath = new SelectPathCommand(p => MessageBodyPath = p, isFolderPicker: true, defaultPath: MessageBodyPath); } public int MaximumErrorRetentionPeriod => SettingConstants.ErrorRetentionPeriodMaxInDays; @@ -102,6 +109,92 @@ public PersistenceManifest SelectedStorageType public bool ShowDatabasePathFields => SelectedStorageType == null || SelectedStorageType.Settings.Any(s => s.Name.EndsWith("/DBPath")); + public bool SupportsMessageBodyStorage => SelectedStorageType?.Settings.Any(s => s.Name == ServiceControlSettings.MessageBodyStorageType.Name) == true; + + public IEnumerable MessageBodyStorageTypes { get; } = + [ + new MessageBodyStorageOption { Name = "File system", Value = MessageBodyStorageType.FileSystem }, + new MessageBodyStorageOption { Name = "Azure Blob Storage", Value = MessageBodyStorageType.AzureBlob }, + new MessageBodyStorageOption { Name = "Amazon S3", Value = MessageBodyStorageType.S3 } + ]; + + public MessageBodyStorageOption SelectedMessageBodyStorageType { get; set; } + + public string MessageBodyPath + { + get => messageBodyPath; + set => messageBodyPath = value.SanitizeFilePath(); + } + + public ICommand SelectMessageBodyPath { get; } + + public IEnumerable AzureAuthenticationOptions { get; } = + [ + new AzureAuthenticationOption { Name = "Connection string", UsesManagedIdentity = false }, + new AzureAuthenticationOption { Name = "Managed identity", UsesManagedIdentity = true } + ]; + + public AzureAuthenticationOption SelectedAzureAuthentication { get; set; } + + public string AzureConnectionString { get; set; } + + public string AzureServiceUri { get; set; } + + public string AzureManagedIdentityClientId { get; set; } + + public string AzureAuthorityHost { get; set; } + + public string AzureContainerName { get; set; } + + public string S3BucketName { get; set; } + + public string S3KeyPrefix { get; set; } + + public string S3Region { get; set; } + + public string S3ServiceUrl { get; set; } + + public string S3AccessKeyId { get; set; } + + public string S3SecretAccessKey { get; set; } + + // Returns null for storage types that keep message bodies in the database. + public MessageBodyStorage CreateMessageBodyStorage() + { + if (!SupportsMessageBodyStorage) + { + return null; + } + + var storageType = SelectedMessageBodyStorageType.Value; + var usesManagedIdentity = SelectedAzureAuthentication.UsesManagedIdentity; + + return storageType switch + { + MessageBodyStorageType.FileSystem => new MessageBodyStorage { StorageType = storageType, FileSystemStoragePath = MessageBodyPath }, + MessageBodyStorageType.AzureBlob => new MessageBodyStorage + { + StorageType = storageType, + AzureConnectionString = usesManagedIdentity ? null : AzureConnectionString, + AzureServiceUri = usesManagedIdentity ? AzureServiceUri : null, + AzureManagedIdentityClientId = usesManagedIdentity ? AzureManagedIdentityClientId : null, + AzureAuthorityHost = usesManagedIdentity ? AzureAuthorityHost : null, + AzureContainerName = AzureContainerName + }, + MessageBodyStorageType.S3 => new MessageBodyStorage + { + StorageType = storageType, + S3BucketName = S3BucketName, + S3KeyPrefix = S3KeyPrefix, + S3Region = S3Region, + S3ServiceUrl = S3ServiceUrl, + S3AccessKeyId = S3AccessKeyId, + S3SecretAccessKey = S3SecretAccessKey + }, + _ => throw new InvalidOperationException($"Unknown message body storage type '{storageType}'.") + }; + } + public double ErrorRetention { get; set; } public TimeSpan ErrorRetentionPeriod => ErrorRetentionUnits == TimeSpanUnits.Days ? TimeSpan.FromDays(ErrorRetention) : TimeSpan.FromHours(ErrorRetention); @@ -174,4 +267,5 @@ public void UpdateFromInstance(ServiceControlInstance instance) ForwardingOption errorForwarding; PersistenceManifest selectedStorageType; + string messageBodyPath; } \ No newline at end of file diff --git a/src/ServiceControl.Config/Validation/Validations.cs b/src/ServiceControl.Config/Validation/Validations.cs index 23e68f6f52..1d0404bd22 100644 --- a/src/ServiceControl.Config/Validation/Validations.cs +++ b/src/ServiceControl.Config/Validation/Validations.cs @@ -223,6 +223,10 @@ public static IRuleBuilderOptions ValidHostname(this IRuleBuilder< public const string MSG_THIS_STORAGE_TYPE_REQUIRES_A_CONNECTION_STRING = "This storage type requires a database connection string."; + public const string MSG_MUST_BE_ABSOLUTE_URI = "Must be an absolute URI, for example https://example.com."; + + public const string MSG_S3_KEYS_GO_TOGETHER = "Specify both the access key ID and secret access key, or neither to use the default AWS credentials."; + public const string MSG_CANTCONTAINWHITESPACE = "{0} cannot contain white space."; public const string MSG_SELECTAUDITFORWARDING = "Must select audit forwarding."; From 2e9501db0517b54b056b46bc1d27da756b965c00 Mon Sep 17 00:00:00 2001 From: Andreas Bednarz <110360248+abparticular@users.noreply.github.com> Date: Tue, 20 Oct 2026 15:02:28 +1100 Subject: [PATCH 09/14] Require the database maintenance port only for RavenDB instances The maintenance port is the embedded RavenDB server's port. The installer validated it for every new instance, so adding a SQL Server or PostgreSQL instance could fail when the default port was in use, and the wizard asked for a value those instances never use. The installer now validates the port only when the storage type declares it, and the wizard hides the field and its rules otherwise. Co-Authored-By: Claude Opus 5.5 --- .../ServiceControlAddScreenLoadedTests.cs | 18 ++++++++++++++++++ .../InstanceAdd/ServiceControlAddAttachment.cs | 4 ++-- .../UI/InstanceAdd/ServiceControlAddView.xaml | 8 ++++++-- .../InstanceAdd/ServiceControlAddViewModel.cs | 4 ++++ .../ServiceControlAddViewModelValidator.cs | 4 ++-- .../ServiceControlAuditNewInstance.cs | 6 +++++- .../Instances/ServiceControlNewInstance.cs | 5 ++++- 7 files changed, 41 insertions(+), 8 deletions(-) diff --git a/src/ServiceControl.Config.Tests/ServiceControlAddScreenLoadedTests.cs b/src/ServiceControl.Config.Tests/ServiceControlAddScreenLoadedTests.cs index afc775cf95..19a9e27122 100644 --- a/src/ServiceControl.Config.Tests/ServiceControlAddScreenLoadedTests.cs +++ b/src/ServiceControl.Config.Tests/ServiceControlAddScreenLoadedTests.cs @@ -76,6 +76,24 @@ public void Changing_storage_type_notifies_bound_visibility_properties() } } + [TestCase("RavenDB", true)] + [TestCase("SQLServer", false)] + [TestCase("PostgreSQL", false)] + public void Database_maintenance_port_is_shown_only_for_RavenDB(string storageTypeName, bool expected) + { + var viewModel = new ServiceControlAddViewModel + { + ErrorSelectedStorageType = ServiceControlInstaller.Engine.Instances.ServiceControlPersisters.GetPrimaryPersistence(storageTypeName), + AuditSelectedStorageType = ServiceControlInstaller.Engine.Instances.ServiceControlPersisters.GetAuditPersistence(storageTypeName) + }; + + using (Assert.EnterMultipleScope()) + { + Assert.That(viewModel.ShowErrorDatabaseMaintenancePort, Is.EqualTo(expected)); + Assert.That(viewModel.ShowAuditDatabaseMaintenancePort, Is.EqualTo(expected)); + } + } + [Test] public void Message_body_storage_is_shown_only_for_storage_types_that_support_it() { diff --git a/src/ServiceControl.Config/UI/InstanceAdd/ServiceControlAddAttachment.cs b/src/ServiceControl.Config/UI/InstanceAdd/ServiceControlAddAttachment.cs index ab4f936ed6..ff285caa35 100644 --- a/src/ServiceControl.Config/UI/InstanceAdd/ServiceControlAddAttachment.cs +++ b/src/ServiceControl.Config/UI/InstanceAdd/ServiceControlAddAttachment.cs @@ -67,7 +67,7 @@ async Task Add(CancellationToken cancellationToken) serviceControlNewInstance.InstallPath = viewModel.ServiceControl.DestinationPath; serviceControlNewInstance.HostName = viewModel.ServiceControl.HostName; serviceControlNewInstance.Port = Convert.ToInt32(viewModel.ServiceControl.PortNumber); - serviceControlNewInstance.DatabaseMaintenancePort = Convert.ToInt32(viewModel.ServiceControl.DatabaseMaintenancePortNumber); + serviceControlNewInstance.DatabaseMaintenancePort = viewModel.ShowErrorDatabaseMaintenancePort ? Convert.ToInt32(viewModel.ServiceControl.DatabaseMaintenancePortNumber) : null; serviceControlNewInstance.VirtualDirectory = null; serviceControlNewInstance.ErrorQueue = viewModel.ServiceControl.ErrorQueueName; serviceControlNewInstance.ErrorLogQueue = viewModel.ServiceControl.ErrorForwarding.Value ? viewModel.ServiceControl.ErrorForwardingQueueName : null; @@ -94,7 +94,7 @@ async Task Add(CancellationToken cancellationToken) auditNewInstance.InstallPath = viewModel.ServiceControlAudit.DestinationPath; auditNewInstance.HostName = viewModel.ServiceControlAudit.HostName; auditNewInstance.Port = Convert.ToInt32(viewModel.ServiceControlAudit.PortNumber); - auditNewInstance.DatabaseMaintenancePort = Convert.ToInt32(viewModel.ServiceControlAudit.DatabaseMaintenancePortNumber); + auditNewInstance.DatabaseMaintenancePort = viewModel.ShowAuditDatabaseMaintenancePort ? Convert.ToInt32(viewModel.ServiceControlAudit.DatabaseMaintenancePortNumber) : null; auditNewInstance.AuditLogQueue = viewModel.ServiceControlAudit.AuditForwarding.Value ? viewModel.ServiceControlAudit.AuditForwardingQueueName : null; auditNewInstance.AuditQueue = viewModel.ServiceControlAudit.AuditQueueName; auditNewInstance.ForwardAuditMessages = viewModel.ServiceControlAudit.AuditForwarding.Value; diff --git a/src/ServiceControl.Config/UI/InstanceAdd/ServiceControlAddView.xaml b/src/ServiceControl.Config/UI/InstanceAdd/ServiceControlAddView.xaml index 58d382ca64..ddf74fb21a 100644 --- a/src/ServiceControl.Config/UI/InstanceAdd/ServiceControlAddView.xaml +++ b/src/ServiceControl.Config/UI/InstanceAdd/ServiceControlAddView.xaml @@ -135,7 +135,9 @@ - + - + ErrorSelectedStorageType == null || ErrorSelectedStorageType.Settings.Any(s => s.Name.EndsWith("/DBPath")); + public bool ShowErrorDatabaseMaintenancePort => ErrorSelectedStorageType == null || ErrorSelectedStorageType.Settings.Any(s => s.Name == ServiceControlSettings.DatabaseMaintenancePort.Name); + public bool ShowErrorMessageBodyStorage => ErrorSelectedStorageType?.Settings.Any(s => s.Name == ServiceControlSettings.MessageBodyStorageType.Name) == true; public IEnumerable ErrorMessageBodyStorageTypes => ServiceControl.MessageBodyStorageTypes; @@ -512,6 +514,8 @@ public string AuditStorageConnectionString public bool ShowAuditDatabasePathFields => AuditSelectedStorageType == null || AuditSelectedStorageType.Settings.Any(s => s.Name.EndsWith("/DBPath")); + public bool ShowAuditDatabaseMaintenancePort => AuditSelectedStorageType == null || AuditSelectedStorageType.Settings.Any(s => s.Name == AuditInstanceSettingsList.DatabaseMaintenancePort.Name); + public ICommand AuditSelectDestinationPath => ServiceControlAudit.SelectDestinationPath; public string AuditDestinationPath diff --git a/src/ServiceControl.Config/UI/InstanceAdd/ServiceControlAddViewModelValidator.cs b/src/ServiceControl.Config/UI/InstanceAdd/ServiceControlAddViewModelValidator.cs index ccced949aa..15bdbeaafc 100644 --- a/src/ServiceControl.Config/UI/InstanceAdd/ServiceControlAddViewModelValidator.cs +++ b/src/ServiceControl.Config/UI/InstanceAdd/ServiceControlAddViewModelValidator.cs @@ -138,7 +138,7 @@ public ServiceControlAddViewModelValidator() .WithMessage(string.Format(Validation.Validations.MSG_PORTS_NOT_EQUAL, "Database Maintenance", "Audit")) .NotEqual(x => x.AuditDatabaseMaintenancePortNumber) .WithMessage(string.Format(Validation.Validations.MSG_PORTS_NOT_EQUAL, "Database Maintenance", "Audit Database Maintenance")) - .When(x => x.InstallErrorInstance); + .When(x => x.InstallErrorInstance && x.ShowErrorDatabaseMaintenancePort); RuleFor(x => x.ErrorDestinationPath) .NotEmpty() @@ -258,7 +258,7 @@ public ServiceControlAddViewModelValidator() .WithMessage(string.Format(Validation.Validations.MSG_PORTS_NOT_EQUAL, "Audit Database Maintenance", "ServiceControl")) .NotEqual(x => x.ErrorDatabaseMaintenancePortNumber) .WithMessage(string.Format(Validation.Validations.MSG_PORTS_NOT_EQUAL, "Audit Database Maintenance", "ServiceControl Database Maintenance")) - .When(x => x.InstallAuditInstance); + .When(x => x.InstallAuditInstance && x.ShowAuditDatabaseMaintenancePort); RuleFor(x => x.AuditDestinationPath) .NotEmpty() diff --git a/src/ServiceControlInstaller.Engine/Instances/ServiceControlAuditNewInstance.cs b/src/ServiceControlInstaller.Engine/Instances/ServiceControlAuditNewInstance.cs index ff192d4cca..82ee1d6f23 100644 --- a/src/ServiceControlInstaller.Engine/Instances/ServiceControlAuditNewInstance.cs +++ b/src/ServiceControlInstaller.Engine/Instances/ServiceControlAuditNewInstance.cs @@ -2,6 +2,7 @@ { using System; using System.IO; + using System.Linq; using System.Xml.Serialization; using Configuration.ServiceControl; using Setup; @@ -64,7 +65,10 @@ protected override void RunSetup() protected override void ValidateMaintenancePort() { - DatabaseMaintenancePortValidator.Validate(this); + if (PersistenceManifest.Settings.Any(s => s.Name == AuditInstanceSettingsList.DatabaseMaintenancePort.Name)) + { + DatabaseMaintenancePortValidator.Validate(this); + } } protected override void ValidateQueueNames() diff --git a/src/ServiceControlInstaller.Engine/Instances/ServiceControlNewInstance.cs b/src/ServiceControlInstaller.Engine/Instances/ServiceControlNewInstance.cs index 6ae5919aaf..8fa1ac3d49 100644 --- a/src/ServiceControlInstaller.Engine/Instances/ServiceControlNewInstance.cs +++ b/src/ServiceControlInstaller.Engine/Instances/ServiceControlNewInstance.cs @@ -108,7 +108,10 @@ protected override void RunSetup() protected override void ValidateMaintenancePort() { - DatabaseMaintenancePortValidator.Validate(this); + if (PersistenceManifest.Settings.Any(s => s.Name == ServiceControlSettings.DatabaseMaintenancePort.Name)) + { + DatabaseMaintenancePortValidator.Validate(this); + } } protected override void ValidateQueueNames() From 46c4f5373da2c8c4e1397ab3585e21138c50a111 Mon Sep 17 00:00:00 2001 From: Andreas Bednarz <110360248+abparticular@users.noreply.github.com> Date: Tue, 20 Oct 2026 15:06:52 +1100 Subject: [PATCH 10/14] Let PowerShell create SQL Server and PostgreSQL instances New-ServiceControlInstance and New-ServiceControlAuditInstance gain -PersistenceType, defaulting to RavenDB, and -DatabaseConnectionString. -DBPath and -DatabaseMaintenancePort are now required only for RavenDB. The choices are gated by ServiceControl_IncludePersistencePicker, like the SCMU storage type picker. New-ServiceControlInstance also gains -MessageBodyStorageType and the parameters needed to reach each storage type: -MessageBodyPath for the file system, -AzureBlob* for Azure Blob Storage, and -S3* for Amazon S3. Each cmdlet rejects parameters the selected persistence or message body storage type does not use, matching the existing transport connection string check. The help for New-ServiceControlInstance documents the new parameters. Co-Authored-By: Claude Opus 5.5 --- .../NewServiceControlAuditInstance.cs | 22 +- .../NewServiceControlInstance.cs | 93 +++++- ...Control.Management.PowerShell.dll-help.xml | 301 +++++++++++++++++- .../Validation/PersistenceParameters.cs | 87 +++++ .../PersistenceTypeValuesGenerator.cs | 20 ++ 5 files changed, 507 insertions(+), 16 deletions(-) create mode 100644 src/ServiceControl.Management.PowerShell/Validation/PersistenceParameters.cs create mode 100644 src/ServiceControl.Management.PowerShell/Validation/PersistenceTypeValuesGenerator.cs diff --git a/src/ServiceControl.Management.PowerShell/Cmdlets/AuditInstances/NewServiceControlAuditInstance.cs b/src/ServiceControl.Management.PowerShell/Cmdlets/AuditInstances/NewServiceControlAuditInstance.cs index 768041419f..f8cc7bb7b6 100644 --- a/src/ServiceControl.Management.PowerShell/Cmdlets/AuditInstances/NewServiceControlAuditInstance.cs +++ b/src/ServiceControl.Management.PowerShell/Cmdlets/AuditInstances/NewServiceControlAuditInstance.cs @@ -23,11 +23,19 @@ public class NewServiceControlAuditInstance : PSCmdlet [ValidatePath] public string InstallPath { get; set; } - [Parameter(Mandatory = true, HelpMessage = "Specify the directory that will contain the RavenDB database for this Audit Instance")] + [Parameter(Mandatory = false, HelpMessage = "Specify the persistence type to use. Defaults to RavenDB")] + [ValidateSet(typeof(AuditPersistenceTypeValuesGenerator))] + public string PersistenceType { get; set; } + + [Parameter(Mandatory = false, HelpMessage = "Specify the directory that will contain the RavenDB database for this Audit Instance. Required for RavenDB")] [ValidateNotNullOrEmpty] [ValidatePath] public string DBPath { get; set; } + [Parameter(Mandatory = false, HelpMessage = "Specify the database connection string. Required for SQLServer and PostgreSQL")] + [ValidateNotNullOrEmpty] + public string DatabaseConnectionString { get; set; } + [Parameter(Mandatory = true, HelpMessage = "Specify the directory to use for this Audit Instance Logs")] [ValidateNotNullOrEmpty] [ValidatePath] @@ -41,9 +49,9 @@ public class NewServiceControlAuditInstance : PSCmdlet [ValidateRange(1, 49151)] public int Port { get; set; } - [Parameter(Mandatory = true, HelpMessage = "Specify the database maintenance port number to listen on")] + [Parameter(Mandatory = false, HelpMessage = "Specify the database maintenance port number to listen on. Required for RavenDB")] [ValidateRange(1, 49151)] - public int DatabaseMaintenancePort { get; set; } + public int? DatabaseMaintenancePort { get; set; } [Parameter(Mandatory = false, HelpMessage = "Specify AuditQueue name to consume messages from. Default is audit")] [ValidateNotNullOrEmpty] @@ -99,6 +107,9 @@ public class NewServiceControlAuditInstance : PSCmdlet protected override void BeginProcessing() { + persistenceManifest = ServiceControlPersisters.GetAuditPersistence(string.IsNullOrWhiteSpace(PersistenceType) ? StorageEngineNames.RavenDB : PersistenceType); + PersistenceParameters.Validate(persistenceManifest, DBPath, DatabaseMaintenancePort, DatabaseConnectionString); + var transport = ServiceControlCoreTransports.Find(Transport); var requiresConnectionString = !string.IsNullOrEmpty(transport.SampleConnectionString); @@ -140,7 +151,7 @@ protected override void BeginProcessing() protected override void ProcessRecord() { - var newAuditInstance = ServiceControlAuditNewInstance.CreateWithDefaultPersistence(); + var newAuditInstance = ServiceControlAuditNewInstance.CreateWithPersistence(persistenceManifest.Name); newAuditInstance.InstallPath = InstallPath; newAuditInstance.LogPath = LogPath; @@ -154,6 +165,7 @@ protected override void ProcessRecord() newAuditInstance.HostName = HostName; newAuditInstance.Port = Port; newAuditInstance.DatabaseMaintenancePort = DatabaseMaintenancePort; + newAuditInstance.PersistenceConnectionString = DatabaseConnectionString; newAuditInstance.AuditQueue = AuditQueue; newAuditInstance.AuditLogQueue = string.IsNullOrWhiteSpace(AuditLogQueue) ? null : AuditLogQueue; newAuditInstance.ForwardAuditMessages = ForwardAuditMessages.ToBool(); @@ -202,6 +214,8 @@ protected override void ProcessRecord() } } + PersistenceManifest persistenceManifest; + Task PromptToProceed(PathInfo pathInfo, CancellationToken cancellationToken) { if (!pathInfo.CheckIfEmpty) diff --git a/src/ServiceControl.Management.PowerShell/Cmdlets/ServiceControlInstances/NewServiceControlInstance.cs b/src/ServiceControl.Management.PowerShell/Cmdlets/ServiceControlInstances/NewServiceControlInstance.cs index eb2e792fcf..48e8130788 100644 --- a/src/ServiceControl.Management.PowerShell/Cmdlets/ServiceControlInstances/NewServiceControlInstance.cs +++ b/src/ServiceControl.Management.PowerShell/Cmdlets/ServiceControlInstances/NewServiceControlInstance.cs @@ -1,6 +1,7 @@ namespace ServiceControl.Management.PowerShell { using System; + using System.Collections.Generic; using System.IO; using System.Linq; using System.Management.Automation; @@ -25,11 +26,72 @@ public class NewServiceControlInstance : PSCmdlet [ValidatePath] public string InstallPath { get; set; } - [Parameter(Mandatory = true, HelpMessage = "Specify the directory that will contain the RavenDB database for this ServiceControl Instance")] + [Parameter(Mandatory = false, HelpMessage = "Specify the persistence type to use. Defaults to RavenDB")] + [ValidateSet(typeof(PrimaryPersistenceTypeValuesGenerator))] + public string PersistenceType { get; set; } + + [Parameter(Mandatory = false, HelpMessage = "Specify the directory that will contain the RavenDB database for this ServiceControl Instance. Required for RavenDB")] [ValidateNotNullOrEmpty] [ValidatePath] public string DBPath { get; set; } + [Parameter(Mandatory = false, HelpMessage = "Specify the database connection string. Required for SQLServer and PostgreSQL")] + [ValidateNotNullOrEmpty] + public string DatabaseConnectionString { get; set; } + + [Parameter(Mandatory = false, HelpMessage = "Specify where message bodies are stored for SQLServer and PostgreSQL. Defaults to FileSystem")] + [ValidateSet("FileSystem", "AzureBlob", "S3")] + public string MessageBodyStorageType { get; set; } + + [Parameter(Mandatory = false, HelpMessage = "Specify the directory that will contain message bodies. Required for FileSystem message body storage")] + [ValidateNotNullOrEmpty] + [ValidatePath] + public string MessageBodyPath { get; set; } + + [Parameter(Mandatory = false, HelpMessage = "Specify the Azure Blob Storage connection string. Use this or AzureBlobServiceUri")] + [ValidateNotNullOrEmpty] + public string AzureBlobConnectionString { get; set; } + + [Parameter(Mandatory = false, HelpMessage = "Specify the Azure Blob Storage service URI to connect with a managed identity. Use this or AzureBlobConnectionString")] + [ValidateNotNullOrEmpty] + public string AzureBlobServiceUri { get; set; } + + [Parameter(Mandatory = false, HelpMessage = "Specify the client ID of a user-assigned managed identity")] + [ValidateNotNullOrEmpty] + public string AzureBlobManagedIdentityClientId { get; set; } + + [Parameter(Mandatory = false, HelpMessage = "Specify the Microsoft Entra authority host for sovereign clouds")] + [ValidateNotNullOrEmpty] + public string AzureBlobAuthorityHost { get; set; } + + [Parameter(Mandatory = false, HelpMessage = "Specify the Azure Blob Storage container name. Defaults to error-bodies")] + [ValidateNotNullOrEmpty] + public string AzureBlobContainerName { get; set; } + + [Parameter(Mandatory = false, HelpMessage = "Specify the S3 bucket name. Required for S3 message body storage")] + [ValidateNotNullOrEmpty] + public string S3BucketName { get; set; } + + [Parameter(Mandatory = false, HelpMessage = "Specify the S3 key prefix. Defaults to error-bodies/")] + [ValidateNotNullOrEmpty] + public string S3KeyPrefix { get; set; } + + [Parameter(Mandatory = false, HelpMessage = "Specify the S3 region")] + [ValidateNotNullOrEmpty] + public string S3Region { get; set; } + + [Parameter(Mandatory = false, HelpMessage = "Specify the service URL of S3-compatible storage")] + [ValidateNotNullOrEmpty] + public string S3ServiceUrl { get; set; } + + [Parameter(Mandatory = false, HelpMessage = "Specify the S3 access key ID. Use with S3SecretAccessKey, or omit both to use the default AWS credentials")] + [ValidateNotNullOrEmpty] + public string S3AccessKeyId { get; set; } + + [Parameter(Mandatory = false, HelpMessage = "Specify the S3 secret access key. Use with S3AccessKeyId")] + [ValidateNotNullOrEmpty] + public string S3SecretAccessKey { get; set; } + [Parameter(Mandatory = true, HelpMessage = "Specify the directory to use for this ServiceControl Logs")] [ValidateNotNullOrEmpty] [ValidatePath] @@ -43,9 +105,9 @@ public class NewServiceControlInstance : PSCmdlet [ValidateRange(1, 49151)] public int Port { get; set; } - [Parameter(Mandatory = true, HelpMessage = "Specify the database maintenance port number to listen on. If this is the only ServiceControl instance then 33334 is recommended")] + [Parameter(Mandatory = false, HelpMessage = "Specify the database maintenance port number to listen on. If this is the only ServiceControl instance then 33334 is recommended. Required for RavenDB")] [ValidateRange(1, 49151)] - public int DatabaseMaintenancePort { get; set; } + public int? DatabaseMaintenancePort { get; set; } [Parameter(Mandatory = false, HelpMessage = "Specify ErrorQueue name to consume messages from. Default is error")] [ValidateNotNullOrEmpty] @@ -106,6 +168,24 @@ public class NewServiceControlInstance : PSCmdlet protected override void BeginProcessing() { + persistenceManifest = ServiceControlPersisters.GetPrimaryPersistence(string.IsNullOrWhiteSpace(PersistenceType) ? StorageEngineNames.RavenDB : PersistenceType); + PersistenceParameters.Validate(persistenceManifest, DBPath, DatabaseMaintenancePort, DatabaseConnectionString); + messageBodyStorage = PersistenceParameters.CreateMessageBodyStorage(persistenceManifest, MessageBodyStorageType, new Dictionary + { + [nameof(MessageBodyPath)] = MessageBodyPath, + [nameof(AzureBlobConnectionString)] = AzureBlobConnectionString, + [nameof(AzureBlobServiceUri)] = AzureBlobServiceUri, + [nameof(AzureBlobManagedIdentityClientId)] = AzureBlobManagedIdentityClientId, + [nameof(AzureBlobAuthorityHost)] = AzureBlobAuthorityHost, + [nameof(AzureBlobContainerName)] = AzureBlobContainerName, + [nameof(S3BucketName)] = S3BucketName, + [nameof(S3KeyPrefix)] = S3KeyPrefix, + [nameof(S3Region)] = S3Region, + [nameof(S3ServiceUrl)] = S3ServiceUrl, + [nameof(S3AccessKeyId)] = S3AccessKeyId, + [nameof(S3SecretAccessKey)] = S3SecretAccessKey + }); + var transport = ServiceControlCoreTransports.Find(Transport); var requiresConnectionString = !string.IsNullOrEmpty(transport.SampleConnectionString); @@ -153,7 +233,7 @@ protected override void BeginProcessing() protected override void ProcessRecord() { - var details = ServiceControlNewInstance.CreateWithDefaultPersistence(); + var details = ServiceControlNewInstance.CreateWithPersistence(persistenceManifest.Name); details.InstallPath = InstallPath; details.LogPath = LogPath; @@ -167,6 +247,8 @@ protected override void ProcessRecord() details.HostName = HostName; details.Port = Port; details.DatabaseMaintenancePort = DatabaseMaintenancePort; + details.PersistenceConnectionString = DatabaseConnectionString; + details.MessageBodyStorage = messageBodyStorage; details.VirtualDirectory = VirtualDirectory; details.ErrorQueue = ErrorQueue; details.ErrorLogQueue = string.IsNullOrWhiteSpace(ErrorLogQueue) ? null : ErrorLogQueue; @@ -229,6 +311,9 @@ protected override void ProcessRecord() } } + PersistenceManifest persistenceManifest; + MessageBodyStorage messageBodyStorage; + Task PromptToProceed(PathInfo pathInfo, CancellationToken cancellationToken) { if (!pathInfo.CheckIfEmpty) diff --git a/src/ServiceControl.Management.PowerShell/ServiceControl.Management.PowerShell.dll-help.xml b/src/ServiceControl.Management.PowerShell/ServiceControl.Management.PowerShell.dll-help.xml index 42ad697714..7e7f71454f 100644 --- a/src/ServiceControl.Management.PowerShell/ServiceControl.Management.PowerShell.dll-help.xml +++ b/src/ServiceControl.Management.PowerShell/ServiceControl.Management.PowerShell.dll-help.xml @@ -2514,10 +2514,10 @@ String - + DBPath - The directory that will contain the embedded database files for this ServiceControl Instance. This should be unique for each instance of ServiceControl and should not be a subfolder of the InstallPath or LogPath. + The directory that will contain the embedded database files for this ServiceControl Instance. This should be unique for each instance of ServiceControl and should not be a subfolder of the InstallPath or LogPath. Required when PersistenceType is RavenDB, and not used otherwise. String @@ -2548,13 +2548,118 @@ Int32 - + DatabaseMaintenancePort - + The port the embedded RavenDB database listens on. If this is the only ServiceControl instance then 33334 is recommended. Required when PersistenceType is RavenDB, and not used otherwise. Int32 + + PersistenceType + + The persistence type to use: RavenDB, SQLServer, or PostgreSQL. SQLServer and PostgreSQL are only available when the ServiceControl_IncludePersistencePicker environment variable is set to true. + + String + + + DatabaseConnectionString + + The connection string of the database that stores the instance data. Required when PersistenceType is SQLServer or PostgreSQL, and not used otherwise. + + String + + + MessageBodyStorageType + + Where message bodies are stored when PersistenceType is SQLServer or PostgreSQL: FileSystem, AzureBlob, or S3. Not used for RavenDB, which stores message bodies in its database. + + String + + + MessageBodyPath + + The directory that will contain message bodies. Required when MessageBodyStorageType is FileSystem. This should be unique for each instance of ServiceControl and should not be a subfolder of the InstallPath or LogPath. + + String + + + AzureBlobConnectionString + + The Azure Blob Storage connection string, using a shared key or SAS token. Specify this or AzureBlobServiceUri when MessageBodyStorageType is AzureBlob. + + String + + + AzureBlobServiceUri + + The Azure Blob Storage service URI, for example https://account.blob.core.windows.net, to connect with a managed identity. Specify this or AzureBlobConnectionString when MessageBodyStorageType is AzureBlob. + + String + + + AzureBlobManagedIdentityClientId + + The client ID of a user-assigned managed identity. Used with AzureBlobServiceUri. Leave empty to use the system-assigned identity. + + String + + + AzureBlobAuthorityHost + + The Microsoft Entra authority host for sovereign clouds. Used with AzureBlobServiceUri. + + String + + + AzureBlobContainerName + + The Azure Blob Storage container that stores message bodies. + + String + + + S3BucketName + + The S3 bucket that stores message bodies. Required when MessageBodyStorageType is S3. + + String + + + S3KeyPrefix + + The prefix for message body object keys in the S3 bucket. + + String + + + S3Region + + The AWS region of the S3 bucket. + + String + + + S3ServiceUrl + + The service URL of S3-compatible storage. Leave empty to use Amazon S3. + + String + + + S3AccessKeyId + + The S3 access key ID. Specify with S3SecretAccessKey, or omit both to use the default AWS credentials, such as an IAM role. + + String + + + S3SecretAccessKey + + The S3 secret access key. Specify with S3AccessKeyId. + + String + ErrorQueue @@ -2696,10 +2801,10 @@ - + DBPath - The directory that will contain the embedded database files for this ServiceControl Instance. This should be unique for each instance of ServiceControl and should not be a subfolder of the InstallPath or LogPath. + The directory that will contain the embedded database files for this ServiceControl Instance. This should be unique for each instance of ServiceControl and should not be a subfolder of the InstallPath or LogPath. Required when PersistenceType is RavenDB, and not used otherwise. String @@ -2750,10 +2855,10 @@ - + DatabaseMaintenancePort - + The port the embedded RavenDB database listens on. If this is the only ServiceControl instance then 33334 is recommended. Required when PersistenceType is RavenDB, and not used otherwise. Int32 @@ -2762,6 +2867,186 @@ + + PersistenceType + + The persistence type to use: RavenDB, SQLServer, or PostgreSQL. SQLServer and PostgreSQL are only available when the ServiceControl_IncludePersistencePicker environment variable is set to true. + + String + + String + + + RavenDB + + + DatabaseConnectionString + + The connection string of the database that stores the instance data. Required when PersistenceType is SQLServer or PostgreSQL, and not used otherwise. + + String + + String + + + + + + MessageBodyStorageType + + Where message bodies are stored when PersistenceType is SQLServer or PostgreSQL: FileSystem, AzureBlob, or S3. Not used for RavenDB, which stores message bodies in its database. + + String + + String + + + FileSystem + + + MessageBodyPath + + The directory that will contain message bodies. Required when MessageBodyStorageType is FileSystem. This should be unique for each instance of ServiceControl and should not be a subfolder of the InstallPath or LogPath. + + String + + String + + + + + + AzureBlobConnectionString + + The Azure Blob Storage connection string, using a shared key or SAS token. Specify this or AzureBlobServiceUri when MessageBodyStorageType is AzureBlob. + + String + + String + + + + + + AzureBlobServiceUri + + The Azure Blob Storage service URI, for example https://account.blob.core.windows.net, to connect with a managed identity. Specify this or AzureBlobConnectionString when MessageBodyStorageType is AzureBlob. + + String + + String + + + + + + AzureBlobManagedIdentityClientId + + The client ID of a user-assigned managed identity. Used with AzureBlobServiceUri. Leave empty to use the system-assigned identity. + + String + + String + + + + + + AzureBlobAuthorityHost + + The Microsoft Entra authority host for sovereign clouds. Used with AzureBlobServiceUri. + + String + + String + + + + + + AzureBlobContainerName + + The Azure Blob Storage container that stores message bodies. + + String + + String + + + error-bodies + + + S3BucketName + + The S3 bucket that stores message bodies. Required when MessageBodyStorageType is S3. + + String + + String + + + + + + S3KeyPrefix + + The prefix for message body object keys in the S3 bucket. + + String + + String + + + error-bodies/ + + + S3Region + + The AWS region of the S3 bucket. + + String + + String + + + + + + S3ServiceUrl + + The service URL of S3-compatible storage. Leave empty to use Amazon S3. + + String + + String + + + + + + S3AccessKeyId + + The S3 access key ID. Specify with S3SecretAccessKey, or omit both to use the default AWS credentials, such as an IAM role. + + String + + String + + + + + + S3SecretAccessKey + + The S3 secret access key. Specify with S3AccessKeyId. + + String + + String + + + + ErrorQueue diff --git a/src/ServiceControl.Management.PowerShell/Validation/PersistenceParameters.cs b/src/ServiceControl.Management.PowerShell/Validation/PersistenceParameters.cs new file mode 100644 index 0000000000..5524b6644f --- /dev/null +++ b/src/ServiceControl.Management.PowerShell/Validation/PersistenceParameters.cs @@ -0,0 +1,87 @@ +namespace ServiceControl.Management.PowerShell.Validation +{ + using System; + using System.Collections.Generic; + using System.Linq; + using ServiceControlInstaller.Engine.Instances; + + // Checks that only the parameters the selected storage type uses are given, like the transport connection string check. + static class PersistenceParameters + { + public static void Validate(PersistenceManifest manifest, string dbPath, int? databaseMaintenancePort, string databaseConnectionString) + { + Check(manifest, "DBPath", manifest.Settings.Any(s => s.Name.EndsWith("/DBPath")), !string.IsNullOrWhiteSpace(dbPath)); + Check(manifest, "DatabaseMaintenancePort", manifest.Settings.Any(s => s.Name.EndsWith("/DatabaseMaintenancePort")), databaseMaintenancePort.HasValue); + Check(manifest, "DatabaseConnectionString", manifest.Settings.Any(s => s.Mandatory && s.Name.EndsWith("/Database/ConnectionString")), !string.IsNullOrWhiteSpace(databaseConnectionString)); + } + + // Returns null for persistence types that keep message bodies in the database. + public static MessageBodyStorage CreateMessageBodyStorage(PersistenceManifest manifest, string storageTypeName, IReadOnlyDictionary parameters) + { + var supportsBodyStorage = manifest.Settings.Any(s => s.Name.EndsWith("/MessageBody/StorageType")); + var specified = parameters.Where(p => !string.IsNullOrWhiteSpace(p.Value)).Select(p => p.Key).ToList(); + + if (!supportsBodyStorage) + { + if (storageTypeName != null) + { + specified.Insert(0, "MessageBodyStorageType"); + } + + if (specified.Count > 0) + { + throw new Exception($"The '{manifest.Name}' persistence type does not use {string.Join(", ", specified)}."); + } + + return null; + } + + var storageType = storageTypeName == null ? MessageBodyStorageType.FileSystem : Enum.Parse(storageTypeName); + var parameterPrefix = storageType switch + { + MessageBodyStorageType.FileSystem => "MessageBodyPath", + MessageBodyStorageType.AzureBlob => "AzureBlob", + MessageBodyStorageType.S3 => "S3", + _ => throw new Exception($"Message body storage type '{storageType}' is not supported.") + }; + + var unused = specified.Where(name => !name.StartsWith(parameterPrefix)).ToList(); + if (unused.Count > 0) + { + throw new Exception($"The '{storageType}' message body storage type does not use {string.Join(", ", unused)}."); + } + + string Value(string name) => parameters.TryGetValue(name, out var value) && !string.IsNullOrWhiteSpace(value) ? value : null; + + return new MessageBodyStorage + { + StorageType = storageType, + FileSystemStoragePath = Value("MessageBodyPath"), + AzureConnectionString = Value("AzureBlobConnectionString"), + AzureServiceUri = Value("AzureBlobServiceUri"), + AzureManagedIdentityClientId = Value("AzureBlobManagedIdentityClientId"), + AzureAuthorityHost = Value("AzureBlobAuthorityHost"), + AzureContainerName = Value("AzureBlobContainerName"), + S3BucketName = Value("S3BucketName"), + S3KeyPrefix = Value("S3KeyPrefix"), + S3Region = Value("S3Region"), + S3ServiceUrl = Value("S3ServiceUrl"), + S3AccessKeyId = Value("S3AccessKeyId"), + S3SecretAccessKey = Value("S3SecretAccessKey") + }; + } + + static void Check(PersistenceManifest manifest, string parameterName, bool isUsed, bool isSpecified) + { + if (isUsed && !isSpecified) + { + throw new Exception($"{parameterName} is mandatory for the '{manifest.Name}' persistence type."); + } + + if (!isUsed && isSpecified) + { + throw new Exception($"The '{manifest.Name}' persistence type does not use {parameterName}."); + } + } + } +} diff --git a/src/ServiceControl.Management.PowerShell/Validation/PersistenceTypeValuesGenerator.cs b/src/ServiceControl.Management.PowerShell/Validation/PersistenceTypeValuesGenerator.cs new file mode 100644 index 0000000000..88bf696cbe --- /dev/null +++ b/src/ServiceControl.Management.PowerShell/Validation/PersistenceTypeValuesGenerator.cs @@ -0,0 +1,20 @@ +namespace ServiceControl.Management.PowerShell.Validation +{ + using System.Linq; + using System.Management.Automation; + using ServiceControlInstaller.Engine.Instances; + + public class PrimaryPersistenceTypeValuesGenerator : IValidateSetValuesGenerator + { + public string[] GetValidValues() => ServiceControlPersisters.GetSupportedPrimaryManifests() + .Select(m => m.Name) + .ToArray(); + } + + public class AuditPersistenceTypeValuesGenerator : IValidateSetValuesGenerator + { + public string[] GetValidValues() => ServiceControlPersisters.GetSupportedAuditManifests() + .Select(m => m.Name) + .ToArray(); + } +} From 0efe809df931fc2dffbfd4a2d93850271bf87dae Mon Sep 17 00:00:00 2001 From: Andreas Bednarz <110360248+abparticular@users.noreply.github.com> Date: Tue, 20 Oct 2026 15:20:32 +1100 Subject: [PATCH 11/14] Offer SQL Server and PostgreSQL without a feature flag The ServiceControl_IncludePersistencePicker environment variable hid the SQL Server and PostgreSQL storage types in SCMU and PowerShell. They are now always offered alongside RavenDB, since this work ships with the next major version. Co-Authored-By: Claude Opus 5.5 --- .../ServiceControlAddScreenLoadedTests.cs | 14 +++++----- .../Properties/launchSettings.json | 10 ------- ...Control.Management.PowerShell.dll-help.xml | 4 +-- .../Configuration/PersistenceManifestTests.cs | 9 ++---- .../Instances/ServiceControlPersisters.cs | 28 ++----------------- 5 files changed, 14 insertions(+), 51 deletions(-) delete mode 100644 src/ServiceControl.Config/Properties/launchSettings.json diff --git a/src/ServiceControl.Config.Tests/ServiceControlAddScreenLoadedTests.cs b/src/ServiceControl.Config.Tests/ServiceControlAddScreenLoadedTests.cs index 19a9e27122..8e11deee77 100644 --- a/src/ServiceControl.Config.Tests/ServiceControlAddScreenLoadedTests.cs +++ b/src/ServiceControl.Config.Tests/ServiceControlAddScreenLoadedTests.cs @@ -38,20 +38,20 @@ public void Transports_are_populated() } [Test] - public void Storage_types_default_to_RavenDB_only_when_picker_is_not_enabled() + public void Storage_types_are_offered_with_no_default_selection() { var viewModel = new ServiceControlAddViewModel(); using (Assert.EnterMultipleScope()) { - Assert.That(viewModel.ErrorStorageTypes.Select(m => m.Name), Is.EqualTo(new[] { "RavenDB" })); - Assert.That(viewModel.ErrorSelectedStorageType?.Name, Is.EqualTo("RavenDB")); - Assert.That(viewModel.ShowErrorStorageTypePicker, Is.False); + Assert.That(viewModel.ErrorStorageTypes.Select(m => m.Name), Is.EquivalentTo(new[] { "PostgreSQL", "RavenDB", "SQLServer" })); + Assert.That(viewModel.ErrorSelectedStorageType, Is.Null); + Assert.That(viewModel.ShowErrorStorageTypePicker, Is.True); Assert.That(viewModel.ShowErrorStorageConnectionString, Is.False); - Assert.That(viewModel.AuditStorageTypes.Select(m => m.Name), Is.EqualTo(new[] { "RavenDB" })); - Assert.That(viewModel.AuditSelectedStorageType?.Name, Is.EqualTo("RavenDB")); - Assert.That(viewModel.ShowAuditStorageTypePicker, Is.False); + Assert.That(viewModel.AuditStorageTypes.Select(m => m.Name), Is.EquivalentTo(new[] { "PostgreSQL", "RavenDB", "SQLServer" })); + Assert.That(viewModel.AuditSelectedStorageType, Is.Null); + Assert.That(viewModel.ShowAuditStorageTypePicker, Is.True); Assert.That(viewModel.ShowAuditStorageConnectionString, Is.False); } } diff --git a/src/ServiceControl.Config/Properties/launchSettings.json b/src/ServiceControl.Config/Properties/launchSettings.json deleted file mode 100644 index aec5f20960..0000000000 --- a/src/ServiceControl.Config/Properties/launchSettings.json +++ /dev/null @@ -1,10 +0,0 @@ -{ - "profiles": { - "ServiceControl.Config": { - "commandName": "Project", - "environmentVariables": { - "ServiceControl_IncludePersistencePicker": "true" - } - } - } -} diff --git a/src/ServiceControl.Management.PowerShell/ServiceControl.Management.PowerShell.dll-help.xml b/src/ServiceControl.Management.PowerShell/ServiceControl.Management.PowerShell.dll-help.xml index 7e7f71454f..251cf5eb11 100644 --- a/src/ServiceControl.Management.PowerShell/ServiceControl.Management.PowerShell.dll-help.xml +++ b/src/ServiceControl.Management.PowerShell/ServiceControl.Management.PowerShell.dll-help.xml @@ -2558,7 +2558,7 @@ PersistenceType - The persistence type to use: RavenDB, SQLServer, or PostgreSQL. SQLServer and PostgreSQL are only available when the ServiceControl_IncludePersistencePicker environment variable is set to true. + The persistence type to use: RavenDB, SQLServer, or PostgreSQL. String @@ -2870,7 +2870,7 @@ PersistenceType - The persistence type to use: RavenDB, SQLServer, or PostgreSQL. SQLServer and PostgreSQL are only available when the ServiceControl_IncludePersistencePicker environment variable is set to true. + The persistence type to use: RavenDB, SQLServer, or PostgreSQL. String diff --git a/src/ServiceControlInstaller.Engine.UnitTests/Configuration/PersistenceManifestTests.cs b/src/ServiceControlInstaller.Engine.UnitTests/Configuration/PersistenceManifestTests.cs index 8da5af27d7..e7b0112f56 100644 --- a/src/ServiceControlInstaller.Engine.UnitTests/Configuration/PersistenceManifestTests.cs +++ b/src/ServiceControlInstaller.Engine.UnitTests/Configuration/PersistenceManifestTests.cs @@ -57,15 +57,12 @@ public void Raven35IsDefault() } [Test] - public void SupportedManifestsDefaultToRavenDBOnly() + public void SupportedManifestsExcludeLegacyRavenDB35() { - // ServiceControl_IncludePersistencePicker is read once in the static constructor, so this only - // exercises the flag-off (default) path. See ServiceControlCoreTransports' equivalent - // ServiceControl_IncludeLearningTransport flag, which has the same untested-when-on limitation. Assert.Multiple(() => { - Assert.That(ServiceControlPersisters.GetSupportedPrimaryManifests().Select(m => m.Name), Is.EqualTo(new[] { "RavenDB" })); - Assert.That(ServiceControlPersisters.GetSupportedAuditManifests().Select(m => m.Name), Is.EqualTo(new[] { "RavenDB" })); + Assert.That(ServiceControlPersisters.GetSupportedPrimaryManifests().Select(m => m.Name), Is.EquivalentTo(new[] { "PostgreSQL", "RavenDB", "SQLServer" })); + Assert.That(ServiceControlPersisters.GetSupportedAuditManifests().Select(m => m.Name), Is.EquivalentTo(new[] { "PostgreSQL", "RavenDB", "SQLServer" })); }); } diff --git a/src/ServiceControlInstaller.Engine/Instances/ServiceControlPersisters.cs b/src/ServiceControlInstaller.Engine/Instances/ServiceControlPersisters.cs index 77756aaaba..e6cc661f00 100644 --- a/src/ServiceControlInstaller.Engine/Instances/ServiceControlPersisters.cs +++ b/src/ServiceControlInstaller.Engine/Instances/ServiceControlPersisters.cs @@ -36,14 +36,12 @@ static ServiceControlPersisters() .OrderBy(m => m.Name) .ToArray(); - // Filtered by environment variable in SCMU, needs to be available all the time for PowerShell. // RavenDB35 is never a user choice - it's a detection-only entry for unconfigured legacy instances. - var includePersistencePicker = IncludePersistencePicker(); supportedPrimaryPersistenceManifests = primaryPersistenceManifests - .Where(m => m.Name != RavenDB35 && (includePersistencePicker || m.Name == StorageEngineNames.RavenDB)) + .Where(m => m.Name != RavenDB35) .ToArray(); supportedAuditPersistenceManifests = auditPersistenceManifests - .Where(m => m.Name != RavenDB35 && (includePersistencePicker || m.Name == StorageEngineNames.RavenDB)) + .Where(m => m.Name != RavenDB35) .ToArray(); } @@ -79,27 +77,5 @@ static PersistenceManifest Load(Assembly assembly, string resourceName) var manifest = JsonSerializer.Deserialize(stream); return manifest; } - - static bool IncludePersistencePicker() - { - try - { - var environmentValue = Environment.GetEnvironmentVariable("ServiceControl_IncludePersistencePicker"); - - if (environmentValue != null) - { - environmentValue = Environment.ExpandEnvironmentVariables(environmentValue); - if (bool.TryParse(environmentValue, out var enabled)) - { - return enabled; - } - } - return false; - } - catch (Exception) - { - return false; - } - } } } From d84cb7bbd956b35230ecd2a6ec64638f90dd19fb Mon Sep 17 00:00:00 2001 From: Andreas Bednarz <110360248+abparticular@users.noreply.github.com> Date: Tue, 20 Oct 2026 15:32:35 +1100 Subject: [PATCH 12/14] Show the message body path instead of DB path in instance details Instance details showed a DB path for every Error and Audit instance, even when nothing uses it: SQL Server and PostgreSQL instances have no local database folder, and an external RavenDB server keeps its data on the server. The DB path now appears only for embedded RavenDB. An Error instance that stores message bodies on the file system shows its message body path in the same place. Co-Authored-By: Claude Opus 5.5 --- .../InstanceDetails/InstanceDetailsView.xaml | 23 +++++++++++++++++++ .../InstanceDetailsViewModel.cs | 20 ++++++++-------- .../Configuration/InstanceTests.cs | 2 ++ .../Instances/ServiceControlInstance.cs | 6 +++++ 4 files changed, 41 insertions(+), 10 deletions(-) diff --git a/src/ServiceControl.Config/UI/InstanceDetails/InstanceDetailsView.xaml b/src/ServiceControl.Config/UI/InstanceDetails/InstanceDetailsView.xaml index 477d4de761..c04b113eed 100644 --- a/src/ServiceControl.Config/UI/InstanceDetails/InstanceDetailsView.xaml +++ b/src/ServiceControl.Config/UI/InstanceDetails/InstanceDetailsView.xaml @@ -273,6 +273,29 @@ + + + + + + + + + + + + + !string.IsNullOrEmpty(DBPath); + public string MessageBodyPath => IsServiceControlInstance ? ServiceControlInstance?.MessageBodyStoragePath : null; + + public bool HasBrowsableMessageBodyPath => !string.IsNullOrEmpty(MessageBodyPath); + public string LogPath => ((IServicePaths)ServiceInstance).LogPath; public SemanticVersion Version => ServiceInstance.Version; @@ -370,6 +367,9 @@ public Task HandleAsync(PostRefreshInstances message, CancellationToken cancella NotifyOfPropertyChange("Host"); NotifyOfPropertyChange("InstallPath"); NotifyOfPropertyChange("DBPath"); + NotifyOfPropertyChange(nameof(HasBrowsableDBPath)); + NotifyOfPropertyChange(nameof(MessageBodyPath)); + NotifyOfPropertyChange(nameof(HasBrowsableMessageBodyPath)); NotifyOfPropertyChange("LogPath"); NotifyOfPropertyChange("Version"); NotifyOfPropertyChange("NewVersion"); diff --git a/src/ServiceControlInstaller.Engine.UnitTests/Configuration/InstanceTests.cs b/src/ServiceControlInstaller.Engine.UnitTests/Configuration/InstanceTests.cs index 149f6ab048..4ebac06112 100644 --- a/src/ServiceControlInstaller.Engine.UnitTests/Configuration/InstanceTests.cs +++ b/src/ServiceControlInstaller.Engine.UnitTests/Configuration/InstanceTests.cs @@ -64,6 +64,7 @@ public void Should_remove_only_message_body_folder_on_uninstall_for_SQL_Server() Assert.That(instance.HasCleanableStoragePaths, Is.True); Assert.That(instance.UsesEmbeddedDatabase, Is.False); Assert.That(instance.ConfiguredMessageBodyStorageType, Is.EqualTo(MessageBodyStorageType.FileSystem)); + Assert.That(instance.MessageBodyStoragePath, Is.EqualTo(messageBodyPath)); instance.RemoveLogsFolder(); instance.RemoveDataBaseFolder(); @@ -90,6 +91,7 @@ public void Should_not_attempt_local_cleanup_on_uninstall_for_external_storage() Assert.That(instance.HasCleanableStoragePaths, Is.False); Assert.That(instance.ConfiguredMessageBodyStorageType, Is.EqualTo(MessageBodyStorageType.S3)); + Assert.That(instance.MessageBodyStoragePath, Is.Null); Assert.That(instance.AppConfig.Read(ServiceControlSettings.MessageBodyFileSystemStoragePath, null), Is.Null, "Only the selected body storage type's settings are written"); } diff --git a/src/ServiceControlInstaller.Engine/Instances/ServiceControlInstance.cs b/src/ServiceControlInstaller.Engine/Instances/ServiceControlInstance.cs index 8bd40fbfe5..f8f99eae12 100644 --- a/src/ServiceControlInstaller.Engine/Instances/ServiceControlInstance.cs +++ b/src/ServiceControlInstaller.Engine/Instances/ServiceControlInstance.cs @@ -66,6 +66,12 @@ public ServiceControlInstance(IWindowsServiceController service) : base(service) ? storageType : null; + // Null unless message bodies are stored on the file system. + public string MessageBodyStoragePath => + ConfiguredMessageBodyStorageType == MessageBodyStorageType.FileSystem + ? AppConfig.Read(ServiceControlSettings.MessageBodyFileSystemStoragePath, null) + : null; + public void AddRemoteInstance(string apiUri) { if (RemoteInstances.All(x => string.Compare(x.ApiUri, apiUri, StringComparison.InvariantCultureIgnoreCase) != 0)) From 0073c3cde14edc31eeee757d006ffe2d0bbaf93c Mon Sep 17 00:00:00 2001 From: Andreas Bednarz <110360248+abparticular@users.noreply.github.com> Date: Wed, 21 Oct 2026 12:48:04 +1100 Subject: [PATCH 13/14] Stop the Add Instance wizard sending a DB path for SQL Server Audit instances The wizard passed the hidden database path for every Audit instance. A SQL Server or PostgreSQL Audit instance then got an unused DB folder, a "directory not empty" prompt for a field the user cannot see, and a folder that removal never cleans up. The Audit instance now gets a DB path only when its storage type uses one, as the Error instance does. Co-Authored-By: Claude Opus 5.5 --- .../ServiceControlAddAttachment.cs | 2 +- .../Configuration/AuditInstanceTests.cs | 23 +++++++++++++++++++ 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/src/ServiceControl.Config/UI/InstanceAdd/ServiceControlAddAttachment.cs b/src/ServiceControl.Config/UI/InstanceAdd/ServiceControlAddAttachment.cs index ff285caa35..0ec11e3211 100644 --- a/src/ServiceControl.Config/UI/InstanceAdd/ServiceControlAddAttachment.cs +++ b/src/ServiceControl.Config/UI/InstanceAdd/ServiceControlAddAttachment.cs @@ -89,7 +89,7 @@ async Task Add(CancellationToken cancellationToken) auditNewInstance.Name = viewModel.ServiceControlAudit.InstanceName.Replace(' ', '.'); auditNewInstance.InstanceName = viewModel.ServiceControlAudit.InstanceName.Replace(' ', '.'); auditNewInstance.ServiceDescription = viewModel.ServiceControlAudit.Description; - auditNewInstance.DBPath = viewModel.ServiceControlAudit.DatabasePath; + auditNewInstance.DBPath = viewModel.ServiceControlAudit.ShowDatabasePathFields ? viewModel.ServiceControlAudit.DatabasePath : null; auditNewInstance.LogPath = viewModel.ServiceControlAudit.LogPath; auditNewInstance.InstallPath = viewModel.ServiceControlAudit.DestinationPath; auditNewInstance.HostName = viewModel.ServiceControlAudit.HostName; diff --git a/src/ServiceControlInstaller.Engine.UnitTests/Configuration/AuditInstanceTests.cs b/src/ServiceControlInstaller.Engine.UnitTests/Configuration/AuditInstanceTests.cs index c143ceb24c..fcc81f13fa 100644 --- a/src/ServiceControlInstaller.Engine.UnitTests/Configuration/AuditInstanceTests.cs +++ b/src/ServiceControlInstaller.Engine.UnitTests/Configuration/AuditInstanceTests.cs @@ -67,6 +67,29 @@ public void Should_remove_log_and_db_folders_on_uninstall() Assert.That(Directory.Exists(DbPath), Is.False); } + [Test] + public void Should_not_create_or_clean_up_a_database_folder_for_SQL_Server() + { + var newInstance = ServiceControlAuditNewInstance.CreateWithPersistence("SQLServer"); + + newInstance.InstallPath = InstallPath; + newInstance.TransportPackage = ServiceControlCoreTransports.Find("LearningTransport"); + newInstance.LogPath = LogPath; + newInstance.HostName = "localhost"; + newInstance.PersistenceConnectionString = "placeholder-connection-string"; + + newInstance.CopyFiles(zipResourceName); + newInstance.WriteConfigurationFile(); + + var instance = new ServiceControlAuditInstance(new FakeWindowsServiceController(Path.Combine(InstallPath, "ServiceControl.Audit.exe"))); + + instance.Reload(); + + Assert.That(Directory.Exists(DbPath), Is.False); + Assert.That(instance.HasCleanableStoragePaths, Is.False); + Assert.That(instance.UsesEmbeddedDatabase, Is.False); + } + class FakeWindowsServiceController : IWindowsServiceController { public FakeWindowsServiceController(string exePath) From a7ce8a924394485007fd58af1d7cc5c49d2ea881 Mon Sep 17 00:00:00 2001 From: Andreas Bednarz <110360248+abparticular@users.noreply.github.com> Date: Wed, 21 Oct 2026 12:50:48 +1100 Subject: [PATCH 14/14] Require the maintenance port on the edit screens only for RavenDB Both edit screens showed and required the database maintenance port, so saving a SQL Server or PostgreSQL instance failed validation: those instances have no port, which left the field empty. The Error edit view already bound the field's visibility to DatabaseMaintenancePortNumberRequired, but no such property existed, so the field always showed. Both edit view models now define that property from the instance's storage type. The Audit edit view binds to it too, both validators skip the port rules when it is false, and saving no longer converts an empty port to a number. Co-Authored-By: Claude Opus 5.5 --- .../EditAuditInstanceValidationTests.cs | 23 ++++++++++ .../EditErrorInstanceValidationTests.cs | 43 +++++++++++++++++++ .../ServiceControlAuditEditView.xaml | 3 +- .../ServiceControlAuditEditViewModel.cs | 9 +++- ...rviceControlAuditEditViewModelValidator.cs | 2 +- .../ServiceControlEditViewModel.cs | 9 +++- .../ServiceControlEditViewModelValidator.cs | 2 +- 7 files changed, 86 insertions(+), 5 deletions(-) diff --git a/src/ServiceControl.Config.Tests/Validation/EditAuditInstanceValidationTests.cs b/src/ServiceControl.Config.Tests/Validation/EditAuditInstanceValidationTests.cs index 8a23c110b3..c5d836315c 100644 --- a/src/ServiceControl.Config.Tests/Validation/EditAuditInstanceValidationTests.cs +++ b/src/ServiceControl.Config.Tests/Validation/EditAuditInstanceValidationTests.cs @@ -92,6 +92,29 @@ public void Transport_connection_string_can_be_null_if_sample_connection_string_ #region hostname + [TestCase("SQLServer", false)] + [TestCase("PostgreSQL", false)] + [TestCase("RavenDB", true)] + public void Database_maintenance_port_is_required_only_for_RavenDB_when_editing_audit_instance(string storageTypeName, bool required) + { + var viewModel = new ServiceControlAuditEditViewModel + { + PersistenceManifest = ServiceControlPersisters.GetAuditPersistence(storageTypeName), + SubmitAttempted = true, + DatabaseMaintenancePortNumber = null + }; + + viewModel.NotifyOfPropertyChange(nameof(viewModel.DatabaseMaintenancePortNumber)); + + var errors = GetNotifyErrorInfo(viewModel).GetErrors(nameof(viewModel.DatabaseMaintenancePortNumber)); + + using (Assert.EnterMultipleScope()) + { + Assert.That(viewModel.DatabaseMaintenancePortNumberRequired, Is.EqualTo(required)); + Assert.That(errors, required ? Is.Not.Empty : Is.Empty); + } + } + [Test] public void Audit_hostname_can_not_be_empty_when_editing_audit_instance() { diff --git a/src/ServiceControl.Config.Tests/Validation/EditErrorInstanceValidationTests.cs b/src/ServiceControl.Config.Tests/Validation/EditErrorInstanceValidationTests.cs index 52b8d82417..5d45883e24 100644 --- a/src/ServiceControl.Config.Tests/Validation/EditErrorInstanceValidationTests.cs +++ b/src/ServiceControl.Config.Tests/Validation/EditErrorInstanceValidationTests.cs @@ -244,6 +244,49 @@ public void Error_port_is_not_equal_to_database_port_number_when_editing_error_i #endregion #region DatabaseManintenancePortnumber + [TestCase("SQLServer")] + [TestCase("PostgreSQL")] + public void Database_maintenance_port_is_not_required_when_editing_error_instance_without_one(string storageTypeName) + { + var viewModel = new ServiceControlEditViewModel + { + PersistenceManifest = ServiceControlPersisters.GetPrimaryPersistence(storageTypeName), + SubmitAttempted = true, + DatabaseMaintenancePortNumber = null + }; + + viewModel.NotifyOfPropertyChange(nameof(viewModel.DatabaseMaintenancePortNumber)); + + var errors = GetNotifyErrorInfo(viewModel).GetErrors(nameof(viewModel.DatabaseMaintenancePortNumber)); + + using (Assert.EnterMultipleScope()) + { + Assert.That(viewModel.DatabaseMaintenancePortNumberRequired, Is.False); + Assert.That(errors, Is.Empty); + } + } + + [Test] + public void Database_maintenance_port_is_required_when_editing_RavenDB_error_instance() + { + var viewModel = new ServiceControlEditViewModel + { + PersistenceManifest = ServiceControlPersisters.GetPrimaryPersistence("RavenDB"), + SubmitAttempted = true, + DatabaseMaintenancePortNumber = null + }; + + viewModel.NotifyOfPropertyChange(nameof(viewModel.DatabaseMaintenancePortNumber)); + + var errors = GetNotifyErrorInfo(viewModel).GetErrors(nameof(viewModel.DatabaseMaintenancePortNumber)); + + using (Assert.EnterMultipleScope()) + { + Assert.That(viewModel.DatabaseMaintenancePortNumberRequired, Is.True); + Assert.That(errors, Is.Not.Empty); + } + } + [Test] public void Database_maintenance_port_cannot_be_empty_when_editing_error_instance() { diff --git a/src/ServiceControl.Config/UI/InstanceEdit/ServiceControlAuditEditView.xaml b/src/ServiceControl.Config/UI/InstanceEdit/ServiceControlAuditEditView.xaml index bff0eec344..3d90c2313b 100644 --- a/src/ServiceControl.Config/UI/InstanceEdit/ServiceControlAuditEditView.xaml +++ b/src/ServiceControl.Config/UI/InstanceEdit/ServiceControlAuditEditView.xaml @@ -87,7 +87,8 @@ Warning="{Binding HostNameWarning}" /> + Text="{Binding DatabaseMaintenancePortNumber}" + Visibility="{Binding DatabaseMaintenancePortNumberRequired, Converter={StaticResource boolToVis}}" /> PersistenceManifest?.Settings.Any(s => s.Name == AuditInstanceSettingsList.DatabaseMaintenancePort.Name) ?? true; + public void UpdateInstanceFromViewModel(ServiceControlAuditInstance instance) { instance.HostName = ServiceControlAudit.HostName; @@ -39,7 +46,7 @@ public void UpdateInstanceFromViewModel(ServiceControlAuditInstance instance) instance.AuditRetentionPeriod = ServiceControlAudit.AuditRetentionPeriod; instance.ForwardAuditMessages = ServiceControlAudit.AuditForwarding.Value; instance.ConnectionString = ConnectionString; - instance.DatabaseMaintenancePort = Convert.ToInt32(ServiceControlAudit.DatabaseMaintenancePortNumber); + instance.DatabaseMaintenancePort = string.IsNullOrWhiteSpace(ServiceControlAudit.DatabaseMaintenancePortNumber) ? null : Convert.ToInt32(ServiceControlAudit.DatabaseMaintenancePortNumber); instance.EnableFullTextSearchOnBodies = ServiceControlAudit.EnableFullTextSearchOnBodies.Value; } diff --git a/src/ServiceControl.Config/UI/InstanceEdit/ServiceControlAuditEditViewModelValidator.cs b/src/ServiceControl.Config/UI/InstanceEdit/ServiceControlAuditEditViewModelValidator.cs index 9741d57238..af5ed8f819 100644 --- a/src/ServiceControl.Config/UI/InstanceEdit/ServiceControlAuditEditViewModelValidator.cs +++ b/src/ServiceControl.Config/UI/InstanceEdit/ServiceControlAuditEditViewModelValidator.cs @@ -40,7 +40,7 @@ public ServiceControlAuditEditViewModelValidator() .WithMessage(string.Format(Validation.Validations.MSG_MUST_BE_UNIQUE, "Audit Database Maintenance Port")) .NotEqual(x => x.PortNumber) .WithMessage(string.Format(Validation.Validations.MSG_PORTS_NOT_EQUAL, "Database Maintenance", "Audit")) - .When(x => x.SubmitAttempted); + .When(x => x.SubmitAttempted && x.DatabaseMaintenancePortNumberRequired); RuleFor(x => x.LogPath) .NotEmpty() diff --git a/src/ServiceControl.Config/UI/InstanceEdit/ServiceControlEditViewModel.cs b/src/ServiceControl.Config/UI/InstanceEdit/ServiceControlEditViewModel.cs index dcbeb0fd46..fb0424da91 100644 --- a/src/ServiceControl.Config/UI/InstanceEdit/ServiceControlEditViewModel.cs +++ b/src/ServiceControl.Config/UI/InstanceEdit/ServiceControlEditViewModel.cs @@ -2,10 +2,12 @@ namespace ServiceControl.Config.UI.InstanceEdit { using System; using System.Collections.Generic; + using System.Linq; using System.Windows.Input; using InstanceAdd; using PropertyChanged; using Extensions; + using ServiceControlInstaller.Engine.Configuration.ServiceControl; using ServiceControlInstaller.Engine.Instances; using Validar; using Xaml.Controls; @@ -17,6 +19,7 @@ public ServiceControlEditViewModel(ServiceControlInstance instance) { DisplayName = "EDIT SERVICECONTROL INSTANCE"; ServiceControlInstance = instance; + PersistenceManifest = instance.PersistenceManifest; ServiceControl.UpdateFromInstance(instance); SelectedTransport = instance.TransportPackage; ConnectionString = instance.ConnectionString; @@ -30,6 +33,10 @@ internal ServiceControlEditViewModel() public ServiceControlInstance ServiceControlInstance { get; set; } + public PersistenceManifest PersistenceManifest { get; set; } + + public bool DatabaseMaintenancePortNumberRequired => PersistenceManifest?.Settings.Any(s => s.Name == ServiceControlSettings.DatabaseMaintenancePort.Name) ?? true; + public void UpdateInstanceFromViewModel(ServiceControlInstance instance) { instance.HostName = ServiceControl.HostName; @@ -40,7 +47,7 @@ public void UpdateInstanceFromViewModel(ServiceControlInstance instance) instance.ErrorRetentionPeriod = ServiceControl.ErrorRetentionPeriod; instance.ForwardErrorMessages = ServiceControl.ErrorForwarding.Value; instance.ConnectionString = ConnectionString; - instance.DatabaseMaintenancePort = Convert.ToInt32(ServiceControl.DatabaseMaintenancePortNumber); + instance.DatabaseMaintenancePort = string.IsNullOrWhiteSpace(ServiceControl.DatabaseMaintenancePortNumber) ? null : Convert.ToInt32(ServiceControl.DatabaseMaintenancePortNumber); instance.EnableFullTextSearchOnBodies = ServiceControl.EnableFullTextSearchOnBodies.Value; instance.EnableIntegratedServicePulse = ServiceControl.EnableIntegratedServicePulse.Value; } diff --git a/src/ServiceControl.Config/UI/InstanceEdit/ServiceControlEditViewModelValidator.cs b/src/ServiceControl.Config/UI/InstanceEdit/ServiceControlEditViewModelValidator.cs index 0e9f6bfe8f..e3e6354065 100644 --- a/src/ServiceControl.Config/UI/InstanceEdit/ServiceControlEditViewModelValidator.cs +++ b/src/ServiceControl.Config/UI/InstanceEdit/ServiceControlEditViewModelValidator.cs @@ -44,7 +44,7 @@ public ServiceControlEditViewModelValidator() .NotEqual(x => x.PortNumber) .WithMessage(string.Format(Validation.Validations.MSG_PORTS_NOT_EQUAL, "Database Maintenance", "ServiceControl")) - .When(x => x.SubmitAttempted); + .When(x => x.SubmitAttempted && x.DatabaseMaintenancePortNumberRequired); RuleFor(x => x.LogPath) .NotEmpty()