chore: runtime configuration improvements#1661
Conversation
martinothamar
added
kind/chore
other
📝 WalkthroughWalkthroughAdds runtime secrets JSON loading from a configurable directory and two AppSettings properties; modifies OpenTelemetry exporter selection and excludes /health from ASP.NET Core tracing; adds unit tests for runtime config loading behavior and updates public API verification. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
test/Altinn.App.Api.Tests/Extensions/WebHostBuilderExtensionsTests.cs
Dismissed
Show dismissed
Hide dismissed
test/Altinn.App.Api.Tests/Extensions/WebHostBuilderExtensionsTests.cs
Dismissed
Show dismissed
Hide dismissed
martinothamar
force-pushed
the
chore/runtime-config-imp
branch
from
91b5a85 to
ea4d578
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Altinn.App.Api/Extensions/ServiceCollectionExtensions.cs (1)
262-272:⚠️ Potential issue | 🟡 MinorSilent OTLP fallback when neither collector nor AppInsights is configured.
When
useOpenTelemetryCollectorisfalse/nullandappInsightsConnectionStringis empty, theelsebranch activates the OTLP exporter targeting the default endpoint (localhost:4317). In a non-test environment without an OTel collector running, this will silently fail or produce connection errors.Consider either:
- Logging a warning when falling back to OTLP without an explicit opt-in, or
- Skipping exporter registration entirely when no backend is configured.
This same pattern applies to metrics (Line 285) and logs (Line 307).
🧹 Nitpick comments (2)
src/Altinn.App.Api/Extensions/WebHostBuilderExtensions.cs (2)
101-143: Two-pass approach for override ordering is clear and correct.Non-override files load first (sorted), then override files load second (sorted), ensuring overrides always win regardless of alphabetical position. The
existingJsonFilePathscheck prevents double-loading files already registered (e.g., maskinporten settings). Well structured.One edge-case note: any file whose name contains the substring
"override"anywhere (e.g.,no-override-here.json) will be treated as an override file. If this is intentional, consider documenting the naming convention; if not, a stricter check (e.g., suffix.override.json) would be more predictable.
68-77: Consider logging when the secrets directory is absent.When the directory doesn't exist, the method silently returns. A debug/trace-level log here would help operators diagnose misconfigured mounts without adding noise in normal operation.
martinothamar
force-pushed
the
chore/runtime-config-imp
branch
from
ea4d578 to
9a6d1e1
Compare
|
/publish |
PR release:
|
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/Altinn.App.Api/Extensions/WebHostBuilderExtensions.cs (1)
115-142:PhysicalFileProviderdisposal — ownership is transferred to the configuration system.Static analysis flagged lines 116 and 138 for missing
DisposeonPhysicalFileProvider. In practice, the provider's lifecycle is managed by theIConfigurationRootbuilt from this builder, which disposes its sources/providers when the host shuts down. This is the same pattern used inAddMaskinportenSettingsFile(seesrc/Altinn.App.Core/Features/Maskinporten/Extensions/WebHostBuilderExtensions.csline 23). The lazy??=allocation ensures the provider is only created when at least one file is loaded.If you want to silence the warning without altering behavior, you could suppress it locally, but this is a standard ASP.NET Core configuration pattern.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Altinn.App.Api/Extensions/WebHostBuilderExtensions.cs` around lines 115 - 142, Static analysis warns that the created PhysicalFileProvider instances (created by the expression "secretsFileProvider ??= new PhysicalFileProvider(secretsDirectory)" used in the calls to configBuilder.AddJsonFile inside WebHostBuilderExtensions) are not disposed; because ownership is intentionally transferred to the configuration system, silence the analyzer locally by wrapping the allocation lines with the appropriate pragma to disable the disposal warning (e.g. disable CA2000) around the statements that contain "secretsFileProvider ??= new PhysicalFileProvider(secretsDirectory)" (or alternatively apply a SuppressMessage attribute scoped to the method) so the behavior stays the same and the warning is suppressed.
🧹 Nitpick comments (2)
src/Altinn.App.Api/Extensions/ServiceCollectionExtensions.cs (1)
238-238: Consider extracting the repeated exporter-selection condition into a local variable.The condition
useOpenTelemetryCollector is not true && !string.IsNullOrWhiteSpace(appInsightsConnectionString)is duplicated across traces, metrics, and logs blocks (lines 262, 285, 307).♻️ Suggested refactor
var useOpenTelemetryCollector = config.GetValue<bool?>("AppSettings:UseOpenTelemetryCollector"); + var useAzureMonitorExporter = useOpenTelemetryCollector is not true + && !string.IsNullOrWhiteSpace(appInsightsConnectionString);Then replace each occurrence:
- if (useOpenTelemetryCollector is not true && !string.IsNullOrWhiteSpace(appInsightsConnectionString)) + if (useAzureMonitorExporter)Also applies to: 262-272, 285-295, 307-317
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Altinn.App.Api/Extensions/ServiceCollectionExtensions.cs` at line 238, Extract the repeated condition into a clearly named local boolean (e.g., useAppInsightsExporter) computed from the existing useOpenTelemetryCollector and appInsightsConnectionString variables (e.g., useAppInsightsExporter = useOpenTelemetryCollector is not true && !string.IsNullOrWhiteSpace(appInsightsConnectionString)); then replace the three duplicated checks in the OpenTelemetry traces, metrics and logs configuration blocks (the blocks referencing traces, metrics and logs in ServiceCollectionExtensions/ServiceCollectionExtensions.cs) with this single local variable to avoid duplication and improve readability.src/Altinn.App.Api/Extensions/WebHostBuilderExtensions.cs (1)
101-143: Consider partitioning files upfront to reduce iteration and duplication.The two loops iterate the full
jsonFilesarray with inverted conditions and both repeat theexistingJsonFilePaths.Containscheck. You could partition once and iterate each group, reducing code duplication. This is a minor nit given the array is small in practice.♻️ Possible simplification
- foreach (string jsonFile in jsonFiles) - { - string jsonFilePath = Path.GetFullPath(jsonFile); - if (existingJsonFilePaths.Contains(jsonFilePath)) - { - continue; - } - - string jsonFileName = Path.GetFileName(jsonFile); - if (jsonFileName.Contains(overrideFileNameFragment, StringComparison.OrdinalIgnoreCase)) - { - continue; - } - - configBuilder.AddJsonFile( - provider: secretsFileProvider ??= new PhysicalFileProvider(secretsDirectory), - path: jsonFileName, - optional: true, - reloadOnChange: true - ); - } - - foreach (string jsonFile in jsonFiles) - { - string jsonFilePath = Path.GetFullPath(jsonFile); - if (existingJsonFilePaths.Contains(jsonFilePath)) - { - continue; - } - - string jsonFileName = Path.GetFileName(jsonFile); - if (!jsonFileName.Contains(overrideFileNameFragment, StringComparison.OrdinalIgnoreCase)) - { - continue; - } - - configBuilder.AddJsonFile( - provider: secretsFileProvider ??= new PhysicalFileProvider(secretsDirectory), - path: jsonFileName, - optional: true, - reloadOnChange: true - ); - } + var newFiles = jsonFiles + .Where(f => !existingJsonFilePaths.Contains(Path.GetFullPath(f))) + .Select(f => (FullPath: f, FileName: Path.GetFileName(f))) + .ToList(); + + // Load non-override files first, then overrides, so overrides take precedence + var ordered = newFiles + .Where(f => !f.FileName.Contains(overrideFileNameFragment, StringComparison.OrdinalIgnoreCase)) + .Concat(newFiles.Where(f => f.FileName.Contains(overrideFileNameFragment, StringComparison.OrdinalIgnoreCase))); + + foreach (var (_, jsonFileName) in ordered) + { + configBuilder.AddJsonFile( + provider: secretsFileProvider ??= new PhysicalFileProvider(secretsDirectory), + path: jsonFileName, + optional: true, + reloadOnChange: true + ); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Altinn.App.Api/Extensions/WebHostBuilderExtensions.cs` around lines 101 - 143, The two foreach blocks over jsonFiles duplicate the Contains check and AddJsonFile call; refactor by partitioning jsonFiles once into two groups (those whose Path.GetFileName(...) contains overrideFileNameFragment and those that do not) while filtering out existingJsonFilePaths via Path.GetFullPath, then iterate each group and call configBuilder.AddJsonFile with the same provider logic (secretsFileProvider ??= new PhysicalFileProvider(secretsDirectory)), preserving optional: true and reloadOnChange: true; this reduces duplicated checks and makes the intent around overrideFileNameFragment, existingJsonFilePaths, secretsFileProvider, secretsDirectory, and configBuilder.AddJsonFile clearer.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/Altinn.App.Api/Extensions/WebHostBuilderExtensions.cs`:
- Around line 115-142: Static analysis warns that the created
PhysicalFileProvider instances (created by the expression "secretsFileProvider
??= new PhysicalFileProvider(secretsDirectory)" used in the calls to
configBuilder.AddJsonFile inside WebHostBuilderExtensions) are not disposed;
because ownership is intentionally transferred to the configuration system,
silence the analyzer locally by wrapping the allocation lines with the
appropriate pragma to disable the disposal warning (e.g. disable CA2000) around
the statements that contain "secretsFileProvider ??= new
PhysicalFileProvider(secretsDirectory)" (or alternatively apply a
SuppressMessage attribute scoped to the method) so the behavior stays the same
and the warning is suppressed.
---
Nitpick comments:
In `@src/Altinn.App.Api/Extensions/ServiceCollectionExtensions.cs`:
- Line 238: Extract the repeated condition into a clearly named local boolean
(e.g., useAppInsightsExporter) computed from the existing
useOpenTelemetryCollector and appInsightsConnectionString variables (e.g.,
useAppInsightsExporter = useOpenTelemetryCollector is not true &&
!string.IsNullOrWhiteSpace(appInsightsConnectionString)); then replace the three
duplicated checks in the OpenTelemetry traces, metrics and logs configuration
blocks (the blocks referencing traces, metrics and logs in
ServiceCollectionExtensions/ServiceCollectionExtensions.cs) with this single
local variable to avoid duplication and improve readability.
In `@src/Altinn.App.Api/Extensions/WebHostBuilderExtensions.cs`:
- Around line 101-143: The two foreach blocks over jsonFiles duplicate the
Contains check and AddJsonFile call; refactor by partitioning jsonFiles once
into two groups (those whose Path.GetFileName(...) contains
overrideFileNameFragment and those that do not) while filtering out
existingJsonFilePaths via Path.GetFullPath, then iterate each group and call
configBuilder.AddJsonFile with the same provider logic (secretsFileProvider ??=
new PhysicalFileProvider(secretsDirectory)), preserving optional: true and
reloadOnChange: true; this reduces duplicated checks and makes the intent around
overrideFileNameFragment, existingJsonFilePaths, secretsFileProvider,
secretsDirectory, and configBuilder.AddJsonFile clearer.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/Altinn.App.Api/Extensions/ServiceCollectionExtensions.cssrc/Altinn.App.Api/Extensions/WebHostBuilderExtensions.cssrc/Altinn.App.Core/Configuration/AppSettings.cstest/Altinn.App.Api.Tests/Extensions/WebHostBuilderExtensionsTests.cstest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
🚧 Files skipped from review as they are similar to previous changes (2)
- test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
- test/Altinn.App.Api.Tests/Extensions/WebHostBuilderExtensionsTests.cs
|
3 participants
Description
/mnt/app-secrets/JSON files automatically added, so we can extend configuration over time without relying on app-lib changes for every little thingRelated Issue(s)
Verification
Documentation
Summary by CodeRabbit
New Features
Improvements
Tests