✨VaultProvider added before build #14
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the Vault configuration initialization to load secrets immediately during the configuration build phase, rather than deferring initialization until after the application is built. This change makes Vault secrets available earlier in the startup process, enabling their use in subsequent configuration steps such as Entity Framework database connection setup.
Changes:
- VaultService constructor now accepts an optional logger parameter (defaults to NullLogger)
- Vault secrets are loaded immediately during configuration.Build() instead of requiring a separate app.UseVault() call
- Removed deferred initialization architecture including UseVault() method, SetVaultService(), and InitializeVaultProviders()
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Vault/Services/VaultService.cs | Made logger parameter optional with NullLogger fallback |
| src/Vault/Extensions/VaultExtension.cs | Removed UseVault() method; VaultService now created immediately in AddVault() |
| src/Vault/Configuration/VaultConfigurationSource.cs | Added VaultService and Logger properties; Build() now loads secrets immediately |
| src/Vault/Configuration/VaultConfigurationProvider.cs | Removed deferred initialization constructor and SetVaultService() method |
| src/Vault/Configuration/VaultConfigurationExtensions.cs | Updated AddVaultConfiguration to require VaultService parameter; removed initialization helpers |
| test/Vault.Tests/Services/VaultServiceTests.cs | Updated test to verify NullLogger usage instead of null argument exception |
| test/Vault.Tests/Extensions/VaultExtensionTests.cs | Removed test for activated Vault registration; updated comment for clarity |
| test/Vault.Tests/Configuration/VaultConfigurationSourceTests.cs | Updated tests to verify immediate loading behavior with mocked VaultService |
| @@ -1,5 +1,4 @@ | |||
| using Microsoft.AspNetCore.Builder; | |||
| using Microsoft.Extensions.Configuration; | |||
| using Microsoft.Extensions.Configuration; | |||
There was a problem hiding this comment.
The removed import of Microsoft.AspNetCore.Builder indicates that UseVault() method was removed, but the documentation files (README.md, docfx/docs/features.md, docfx/docs/getting-started.md) still extensively reference app.UseVault() as a required step. This creates a breaking change and will cause confusion for users following the documentation. The documentation needs to be updated to reflect the new initialization flow where VaultService is created immediately during AddVault() and secrets are loaded during configuration build.
| IVaultService? vaultService = serviceProvider.GetService<IVaultService>(); | ||
| Assert.Null(vaultService); | ||
| } | ||
|
|
There was a problem hiding this comment.
The test 'AddVault_WithValidConfiguration_RegistersVaultOptions' was removed but it provided important coverage for verifying that VaultOptions is correctly registered when Vault is activated. Consider adding a similar test that verifies VaultOptions registration with an activated Vault configuration, including the VaultService registration as well.
| /// <summary> | ||
| /// Build the configuration provider. | ||
| /// Secrets are loaded immediately to make them available for subsequent | ||
| /// configuration (e.g., connection strings for Entity Framework). | ||
| /// </summary> | ||
| /// <exception cref="InvalidOperationException">Thrown when VaultService is not set.</exception> | ||
| public IConfigurationProvider Build(IConfigurationBuilder builder) | ||
| { | ||
| return new VaultConfigurationProvider(this); | ||
| if (VaultService == null) | ||
| { | ||
| throw new InvalidOperationException( | ||
| "VaultService must be set. Use AddVault() or AddVaultConfiguration() with a VaultService instance."); | ||
| } | ||
|
|
||
| // Create provider with VaultService and load immediately | ||
| var provider = new VaultConfigurationProvider(this, VaultService, Logger); | ||
| provider.Load(); | ||
|
|
||
| return provider; | ||
| } |
There was a problem hiding this comment.
The Build method calls provider.Load() synchronously on line 60, which internally calls LoadAsync().GetAwaiter().GetResult(). This blocks the thread during configuration build and could cause deadlocks in some contexts (e.g., if called from a UI thread or in certain ASP.NET Core synchronization contexts). While this is intentional for immediate loading, consider documenting this blocking behavior in the XML documentation comment to warn users about potential performance implications during application startup.
No description provided.