LNK-4436: Terminology service uses Azure App Config (ACA) - #1232
Conversation
…tion source from the Shared library, rather than repeating the same code across all projects. * Adding external configuration source to the Terminology service so that it can use Azure App Config
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis pull request consolidates external configuration loading across multiple microservices by introducing a centralized Changes
Sequence DiagramsequenceDiagram
participant Old as Old Flow (Service)
participant New as New Flow (Service)
participant Shared
participant Config as Azure App Config
rect rgb(200, 220, 255)
Note over Old: Before Refactoring
Old->>Old: Check ExternalConfigurationSource setting
alt ExternalConfigurationSource == "AzureAppConfiguration"
Old->>Config: Connect & configure
Old->>Config: Load keys (null, serviceName, serviceName:env)
Config-->>Old: Configuration loaded
end
Old->>Old: Register services
end
rect rgb(220, 255, 220)
Note over New: After Refactoring
New->>Shared: AddExternalConfiguration(serviceName)
Shared->>Shared: Read ExternalConfigurationSource
alt ExternalConfigurationSource == "AzureAppConfiguration"
Shared->>Config: Connect & configure
Shared->>Config: Load keys (null, serviceName, serviceName:env)
Config-->>Shared: Configuration loaded
end
Shared-->>New: Builder returned
New->>New: Register services
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
DotNet/Census/Program.cs (1)
291-299: Note: Duplicate hosted service registrations detected.
PatientListsAcquiredListeneris registered twice (line 204 and line 291), and the retry services block (lines 294-299) duplicates the registration from lines 206-211. This appears to be a pre-existing issue unrelated to the current PR's external configuration changes.Consider removing the duplicate registrations:
- builder.Services.AddHostedService<PatientListsAcquiredListener>(); - - if (consumerSettings == null || !consumerSettings.DisableRetryConsumer) { - builder.Services.AddHostedService<ScheduleService>(); - builder.Services.AddSingleton(new RetryListenerSettings(CensusConstants.ServiceName, [KafkaTopic.PatientListsAcquiredRetry.GetStringValue()])); - builder.Services.AddHostedService<RetryListener>(); + // Already registered above at lines 206-211 }
🧹 Nitpick comments (3)
DotNet/DataAcquisition.Domain/Settings/DataAcquisitionConstants.cs (1)
28-28: Remove commented-out code instead of leaving it in place.Several constants are commented out rather than removed. If these are no longer needed, they should be deleted entirely to reduce code clutter and maintain clarity.
Apply this diff to remove the commented-out code:
public static class LoggingIds { public const int GenerateItems = 1000; - //public const int ListItems = 1001; public const int GetItem = 1002; public const int InsertItem = 1003; public const int UpdateItem = 1004; public const int DeleteItem = 1005; - //public const int GetItemNotFound = 1006; - //public const int UpdateItemNotFound = 1007; - //public const int KafkaConsumer = 10008; - //public const int KafkaProducer = 10009; - //public const int HealthCheck = 10010; } public static class Auth { public const string Bearer = "Bearer"; - //public const string Basic = "basic"; - //public const string FormUrlEncoded = "application/x-www-form-urlencoded"; }Also applies to: 33-37, 43-44
DotNet/Shared/Application/Extensions/ExternalConfigurationExtension.cs (2)
12-45: Consider adding observability and validation to the configuration loader.The extension silently handles several edge cases without logging or validation:
- Missing serviceName validation: The
serviceNameparameter is not validated and could be null or empty, potentially causing issues downstream.- Silent configuration skipping: When
externalConfigurationSourceis null or the connection string is empty, configuration loading is silently skipped. This could lead to difficult-to-debug issues in production if the configuration is expected but not loaded.- No success logging: There's no indication when external configuration is successfully loaded, making it difficult to verify correct behavior during deployment.
Consider these improvements:
public static WebApplicationBuilder AddExternalConfiguration(this WebApplicationBuilder builder, string serviceName) { + if (string.IsNullOrWhiteSpace(serviceName)) + throw new ArgumentException("Service name cannot be null or empty.", nameof(serviceName)); + var externalConfigurationSource = builder.Configuration.GetSection(ConfigurationConstants.AppSettings.ExternalConfigurationSource).Get<string>(); if (externalConfigurationSource is not null) { switch (externalConfigurationSource) { case "AzureAppConfiguration": builder.Configuration.AddAzureAppConfiguration(options => { string? connectionString = builder.Configuration.GetConnectionString(ConfigurationConstants.DatabaseConnections.AzureAppConfiguration); - if (!string.IsNullOrEmpty(connectionString)) + if (string.IsNullOrEmpty(connectionString)) + { + throw new InvalidOperationException( + $"Azure App Configuration is enabled for {serviceName} but connection string '{ConfigurationConstants.DatabaseConnections.AzureAppConfiguration}' is not configured."); + } - { options.Connect(connectionString) // Load configuration values with no label .Select("*", LabelFilter.Null) // Load configuration values for service name .Select("*", serviceName) // Load configuration values for service name and environment .Select("*", serviceName + ":" + builder.Environment); options.ConfigureKeyVault(kv => { kv.SetCredential(new DefaultAzureCredential()); }); - } }); + Console.WriteLine($"External configuration loaded from Azure App Configuration for service: {serviceName}"); break; } } return builder; }
18-41: Simplify single-case switch or document extensibility intent.The switch statement currently handles only one case ("AzureAppConfiguration"). This could be simplified to an if statement for better readability, or if future configuration sources are planned, add a comment documenting the extensibility intent.
If no other sources are planned soon:
- switch (externalConfigurationSource) - { - case "AzureAppConfiguration": - builder.Configuration.AddAzureAppConfiguration(options => - { - // ... configuration code ... - }); - break; - } + if (externalConfigurationSource == "AzureAppConfiguration") + { + builder.Configuration.AddAzureAppConfiguration(options => + { + // ... configuration code ... + }); + }Or if extensibility is intended, add a comment:
+ // Switch pattern used to support future external configuration sources (e.g., AWS Secrets Manager, HashiCorp Vault) switch (externalConfigurationSource) { case "AzureAppConfiguration":
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (36)
DotNet/Account/Account.csproj(0 hunks)DotNet/Account/Program.cs(1 hunks)DotNet/Account/Settings/AccountConstants.cs(0 hunks)DotNet/Admin.BFF/Admin.BFF.csproj(0 hunks)DotNet/Admin.BFF/Infrastructure/Extensions/ExternalServices/ExternalConfigurationExtension.cs(0 hunks)DotNet/Admin.BFF/Program.cs(1 hunks)DotNet/Admin.BFF/Settings/LinkAdminConstants.cs(0 hunks)DotNet/Audit/Audit.csproj(0 hunks)DotNet/Audit/Program.cs(1 hunks)DotNet/Audit/Settings/AuditConstants.cs(1 hunks)DotNet/Census/Application/Settings/CensusConstants.cs(0 hunks)DotNet/Census/Census.csproj(0 hunks)DotNet/Census/Program.cs(1 hunks)DotNet/DataAcquisition.Domain/DataAcquisition.Domain.csproj(0 hunks)DotNet/DataAcquisition.Domain/Extensions/GeneralStartupExtensions.cs(1 hunks)DotNet/DataAcquisition.Domain/Settings/DataAcquisitionConstants.cs(1 hunks)DotNet/Normalization/Application/Settings/NormalizationConstants.cs(0 hunks)DotNet/Normalization/Normalization.csproj(0 hunks)DotNet/Normalization/Program.cs(1 hunks)DotNet/QueryDispatch/Application/Settings/QueryDispatchConstants.cs(0 hunks)DotNet/QueryDispatch/Program.cs(1 hunks)DotNet/QueryDispatch/QueryDispatch.csproj(0 hunks)DotNet/Report/Application/Settings/ReportConstants.cs(0 hunks)DotNet/Report/Program.cs(1 hunks)DotNet/Report/Report.csproj(0 hunks)DotNet/Shared/Application/Extensions/ExternalConfigurationExtension.cs(1 hunks)DotNet/Shared/Settings/ConfigurationConstants.cs(1 hunks)DotNet/Shared/Shared.csproj(2 hunks)DotNet/Submission/Program.cs(1 hunks)DotNet/Submission/Settings/SubmissionConstants.cs(0 hunks)DotNet/Submission/Submission.csproj(0 hunks)DotNet/Tenant/Config/TenantConstants.cs(0 hunks)DotNet/Tenant/Program.cs(1 hunks)DotNet/Tenant/Tenant.csproj(0 hunks)DotNet/Terminology/Application/Settings/TerminologyConstants.cs(0 hunks)DotNet/Terminology/Program.cs(1 hunks)
💤 Files with no reviewable changes (20)
- DotNet/Audit/Audit.csproj
- DotNet/Tenant/Config/TenantConstants.cs
- DotNet/QueryDispatch/QueryDispatch.csproj
- DotNet/QueryDispatch/Application/Settings/QueryDispatchConstants.cs
- DotNet/Normalization/Normalization.csproj
- DotNet/Normalization/Application/Settings/NormalizationConstants.cs
- DotNet/Census/Application/Settings/CensusConstants.cs
- DotNet/Submission/Settings/SubmissionConstants.cs
- DotNet/Tenant/Tenant.csproj
- DotNet/Terminology/Application/Settings/TerminologyConstants.cs
- DotNet/Report/Report.csproj
- DotNet/DataAcquisition.Domain/DataAcquisition.Domain.csproj
- DotNet/Admin.BFF/Settings/LinkAdminConstants.cs
- DotNet/Account/Settings/AccountConstants.cs
- DotNet/Admin.BFF/Infrastructure/Extensions/ExternalServices/ExternalConfigurationExtension.cs
- DotNet/Report/Application/Settings/ReportConstants.cs
- DotNet/Submission/Submission.csproj
- DotNet/Account/Account.csproj
- DotNet/Admin.BFF/Admin.BFF.csproj
- DotNet/Census/Census.csproj
🧰 Additional context used
📓 Path-based instructions (2)
**/*.cs
⚙️ CodeRabbit configuration file
**/*.cs: TheHtmlInputSanitizerclass'sSanitize()andSanitizeAndRemove()methods should be used when dealing withstringquery parameters from REST requests.
Files:
DotNet/Admin.BFF/Program.csDotNet/Report/Program.csDotNet/Submission/Program.csDotNet/Shared/Settings/ConfigurationConstants.csDotNet/Terminology/Program.csDotNet/Shared/Application/Extensions/ExternalConfigurationExtension.csDotNet/Census/Program.csDotNet/Normalization/Program.csDotNet/Audit/Settings/AuditConstants.csDotNet/Account/Program.csDotNet/Tenant/Program.csDotNet/QueryDispatch/Program.csDotNet/DataAcquisition.Domain/Settings/DataAcquisitionConstants.csDotNet/DataAcquisition.Domain/Extensions/GeneralStartupExtensions.csDotNet/Audit/Program.cs
**
⚙️ CodeRabbit configuration file
**: Pull requests that have "TECH_DEBT" in the title should only contain changes related to typos, unused code, linter/IDE suggestions, swagger specification updates,
and logging improvements. These TECH_DEBT PRs must not affect core functionality. All PRs that are not considered technical debt must include information on what
testing was performed in the description of the PR. If it does not, ask the author to provide details on what testing was performed.
When reviewing code, suggest unit tests using XUnit in the following scenarios:
- If/Else or Switch/Case blocks are introduced or modified — ensure each branch has a corresponding unit test.
- Logic that depends on service or interface configuration — suggest tests to validate different implementations are correctly resolved.
- No network activity (HTTP calls, sockets, etc.) should appear in unit tests. Recommend using mocks (via Moq) for any external communication.
Large unit tests should be avoided; keeping unit tests small and focused on targeted business logic (i.e. string sanitization)
**: Pull requests that have DOCS in the title should only contain changes related to documentation within the /docs folder or in .md files through-out the code-base. The description
of the PR should specify what documentation was updated. Documentation updates should use EventCatalog.dev structure, where service-specific functionality should be described
in the service's index.mdx (i.e. /services/XXX/index.mdx or /domains/XXX/services/YYY/index.mdx). Configurations that are shared by multiple services should be
reflected in the /docs/docs/config files.
Files:
DotNet/Admin.BFF/Program.csDotNet/Report/Program.csDotNet/Submission/Program.csDotNet/Shared/Settings/ConfigurationConstants.csDotNet/Terminology/Program.csDotNet/Shared/Application/Extensions/ExternalConfigurationExtension.csDotNet/Census/Program.csDotNet/Normalization/Program.csDotNet/Shared/Shared.csprojDotNet/Audit/Settings/AuditConstants.csDotNet/Account/Program.csDotNet/Tenant/Program.csDotNet/QueryDispatch/Program.csDotNet/DataAcquisition.Domain/Settings/DataAcquisitionConstants.csDotNet/DataAcquisition.Domain/Extensions/GeneralStartupExtensions.csDotNet/Audit/Program.cs
🧠 Learnings (4)
📓 Common learnings
Learnt from: seanmcilvenna
Repo: lantanagroup/link-cloud PR: 669
File: docs/domains/KnowledgeArtifactManagement/services/TerminologyService/openapi.yml:519-527
Timestamp: 2025-09-24T21:01:05.197Z
Learning: In the Link Cloud project, OpenAPI specifications for all services (including TerminologyService) are auto-generated from the service code and should not be manually modified. Any schema corrections or improvements should be addressed in the source service implementation rather than in the generated OpenAPI files.
Learnt from: smailliwcs
Repo: lantanagroup/link-cloud PR: 1070
File: DotNet/Report/Application/Extensions/KafkaProducerRegistration.cs:18-18
Timestamp: 2025-09-12T14:22:06.888Z
Learning: The team prefers to consolidate duplicate DI registrations (like IKafkaProducerFactory<string, AuditEventMessage>) into shared extension methods as part of larger DI refactoring efforts, rather than addressing them piecemeal in feature PRs.
📚 Learning: 2024-12-27T20:05:54.249Z
Learnt from: seanmcilvenna
Repo: lantanagroup/link-cloud PR: 593
File: docs/service_specs/submission.md:21-21
Timestamp: 2024-12-27T20:05:54.249Z
Learning: The Submission service uses MongoDB (configured via `builder.Services.Configure<MongoConnection>`). The `DatabaseProvider` property from `appsettings.json` has been removed, and documentation references to SQL Server have been replaced with MongoDB.
Applied to files:
DotNet/Submission/Program.cs
📚 Learning: 2025-09-09T19:15:39.938Z
Learnt from: smailliwcs
Repo: lantanagroup/link-cloud PR: 1070
File: DotNet/Report/KafkaProducers/AuditableEventOccurredProducer.cs:18-22
Timestamp: 2025-09-09T19:15:39.938Z
Learning: "X-Correlation-Id" is defined as a constant in KafkaConstants.HeaderConstants.CorrelationId in DotNet/Shared/Settings/KafkaConstants.cs and is used consistently across all Kafka producers in the LantanaGroup.Link system.
Applied to files:
DotNet/DataAcquisition.Domain/Settings/DataAcquisitionConstants.cs
📚 Learning: 2025-09-12T14:22:06.888Z
Learnt from: smailliwcs
Repo: lantanagroup/link-cloud PR: 1070
File: DotNet/Report/Application/Extensions/KafkaProducerRegistration.cs:18-18
Timestamp: 2025-09-12T14:22:06.888Z
Learning: The team prefers to consolidate duplicate DI registrations (like IKafkaProducerFactory<string, AuditEventMessage>) into shared extension methods as part of larger DI refactoring efforts, rather than addressing them piecemeal in feature PRs.
Applied to files:
DotNet/DataAcquisition.Domain/Extensions/GeneralStartupExtensions.cs
🧬 Code graph analysis (11)
DotNet/Admin.BFF/Program.cs (1)
DotNet/Admin.BFF/Settings/LinkAdminConstants.cs (1)
LinkAdminConstants(3-53)
DotNet/Report/Program.cs (1)
DotNet/Report/Application/Settings/ReportConstants.cs (1)
ReportConstants(3-43)
DotNet/Submission/Program.cs (1)
DotNet/Submission/Settings/SubmissionConstants.cs (1)
SubmissionConstants(3-13)
DotNet/Terminology/Program.cs (1)
DotNet/Terminology/Application/Settings/TerminologyConstants.cs (1)
TerminologyConstants(8-18)
DotNet/Shared/Application/Extensions/ExternalConfigurationExtension.cs (1)
DotNet/Shared/Settings/ConfigurationConstants.cs (3)
ConfigurationConstants(4-41)AppSettings(10-27)DatabaseConnections(29-34)
DotNet/Census/Program.cs (1)
DotNet/Census/Application/Settings/CensusConstants.cs (1)
CensusConstants(3-44)
DotNet/Normalization/Program.cs (1)
DotNet/Normalization/Application/Settings/NormalizationConstants.cs (1)
NormalizationConstants(4-18)
DotNet/Account/Program.cs (1)
DotNet/Account/Settings/AccountConstants.cs (1)
AccountConstants(3-65)
DotNet/Tenant/Program.cs (1)
DotNet/Tenant/Config/TenantConstants.cs (1)
TenantConstants(3-39)
DotNet/QueryDispatch/Program.cs (1)
DotNet/QueryDispatch/Application/Settings/QueryDispatchConstants.cs (1)
QueryDispatchConstants(3-38)
DotNet/Audit/Program.cs (1)
DotNet/Audit/Settings/AuditConstants.cs (1)
AuditConstants(3-41)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Integration Tests
- GitHub Check: Unit Tests for DotNet
- GitHub Check: Unit Tests for Java
- GitHub Check: Smoke Test with Docker Compose
- GitHub Check: Analyze (csharp)
- GitHub Check: Analyze (java-kotlin)
🔇 Additional comments (13)
DotNet/Shared/Settings/ConfigurationConstants.cs (1)
26-26: LGTM! Centralized configuration constants added.The new constants
ExternalConfigurationSourceandAzureAppConfigurationsupport the centralized external configuration loading pattern across all services.Also applies to: 33-33
DotNet/Audit/Settings/AuditConstants.cs (1)
7-12: LGTM! Constants cleanup aligns with centralized configuration.The removal of
ExternalConfigurationSourceandIdentityProviderConfigconstants is correct, as external configuration loading is now handled by the centralizedAddExternalConfigurationextension.DotNet/Terminology/Program.cs (1)
21-22: LGTM! PR objective achieved - Terminology service now uses Azure App Config.This change directly addresses the linked issue LNK-4436 by enabling the Terminology service to load external configuration from Azure App Config, aligning it with other services in the system.
DotNet/Admin.BFF/Program.cs (1)
54-55: LGTM! Consistent migration to centralized external configuration.The Admin BFF service has been successfully migrated to use the centralized
AddExternalConfigurationextension, reducing code duplication and ensuring consistency across services.DotNet/Submission/Program.cs (1)
57-58: LGTM! Submission service migrated to centralized external configuration.The migration follows the established pattern and correctly replaces the previous Azure App Configuration loading logic with the centralized extension method.
DotNet/Report/Program.cs (1)
63-64: LGTM! Report service successfully migrated to centralized external configuration.The migration is consistent with the refactoring pattern applied across all services, completing the consolidation of external configuration loading into a single shared extension method.
DotNet/Tenant/Program.cs (1)
63-64: Extension method verified; implementation is correct.The
AddExternalConfigurationextension method exists atDotNet/Shared/Application/Extensions/ExternalConfigurationExtension.csand is correctly implemented. The method properly:
- Reads the external configuration source from settings
- Validates the Azure App Configuration connection string before use
- Applies appropriate label filters (null label, service name, and environment-specific labels)
- Configures Key Vault with DefaultAzureCredential
- Returns the builder for chaining
The call in
Program.cspassesTenantConstants.ServiceName(a constant), not user input, so no sanitization is required per coding guidelines. The centralized external configuration loading is appropriately implemented.DotNet/Audit/Program.cs (1)
60-61: LGTM! External configuration loading successfully consolidated.The replacement of the inline Azure App Configuration setup with the centralized
AddExternalConfigurationmethod successfully reduces code duplication and improves maintainability. The comment accurately reflects the conditional nature of external configuration loading.DotNet/Census/Program.cs (1)
63-64: LGTM! External configuration loading successfully consolidated.The centralized configuration approach is correctly implemented, matching the pattern used across other services.
DotNet/QueryDispatch/Program.cs (1)
54-55: LGTM! External configuration loading successfully consolidated.The implementation is consistent with other services and correctly placed before the ServiceInformation retrieval.
DotNet/DataAcquisition.Domain/Extensions/GeneralStartupExtensions.cs (1)
55-56: LGTM! External configuration consolidated at the shared extension level.Moving the external configuration loading to the centralized extension is a good design decision, especially for a shared registration method like
RegisterAllthat's used by multiple data acquisition services. This reduces duplication at the domain level.DotNet/Normalization/Program.cs (1)
57-58: LGTM! External configuration loading successfully consolidated.The implementation is consistent with the pattern established across all other services.
DotNet/Account/Program.cs (1)
60-61: LGTM! External configuration loading successfully consolidated.The implementation is consistent with all other services and correctly uses
AccountConstants.ServiceName.
seanmcilvenna
changed the title
seanmcilvenna
changed the title
… Title it's testing... Doing this because I can't figure out why the PR Title Check is failing.
# Conflicts: # DotNet/Submission/Submission.csproj
2 participants
🛠️ Description of Changes
Added support for the Terminology service to use Azure App Config.
While doing so, refactored other services so that they all use a single
AddExternalConfigurationmethod/extension in the Shared project rather than repeating the same code across all services.🧪 Testing Performed
Limited in capacity to extensively test this since Azure App Config is only used at deployment time. Primarily ensured that Docker Compose continues to build successfully. Will have to defer remaining developer testing until I can build the services and deploy them to the DEV environment.
🧑🔬 Unit Testing
N/A
📓 Documentation Updated
Terminology service already declared "Azure App Config" in docs... No changes necessary, this is just adding the fucntionality that was supposed to be there already.
Summary by CodeRabbit
Release Notes
Refactor
Chores