-
Notifications
You must be signed in to change notification settings - Fork 56
Added support for AppConfig Feature Flag profiles #229
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
Conversation
reviewing this now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Adds support for AppConfig Feature Flag profiles by wrapping AWS AppConfig JSON under a consumer-defined parent node.
- Introduces
WrapperNodeName
on the configuration source and surfaces it in the extension methods - Implements
AddWrapperNodeAsync
inAppConfigProcessor
and applies it in the service-based retrieval path - Adds a unit test for the new wrapper logic and updates the README sample to demonstrate usage
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
src/Amazon.Extensions.Configuration.SystemsManager/AppConfig/AppConfigConfigurationSource.cs | Added WrapperNodeName property |
src/Amazon.Extensions.Configuration.SystemsManager/AppConfig/AppConfigExtensions.cs | Extended ConfigureSource signature to include wrapperNodeName |
src/Amazon.Extensions.Configuration.SystemsManager/AppConfig/AppConfigProcessor.cs | Implemented AddWrapperNodeAsync and applied it in GetDataFromServiceAsync |
test/Amazon.Extensions.Configuration.SystemsManager.Tests/AppConfigProcessorTests.cs | Added unit test for AddWrapperNodeAsync via reflection |
README.md | Updated sample code to show how to configure and consume the wrapper |
Comments suppressed due to low confidence (3)
README.md:224
- Add
using System.Text.Json.Serialization;
at the top of this sample so the[JsonPropertyName]
attribute is recognized.
[JsonPropertyName("enabled")]
README.md:254
- [nitpick] Per C# conventions, private fields should use camelCase (e.g.,
_featureFlags
) instead of PascalCase.
private readonly IOptionsMonitor<Dictionary<string, AppConfigFeatureFlag>> _FeatureFlags;
src/Amazon.Extensions.Configuration.SystemsManager/AppConfig/AppConfigExtensions.cs:235
- The new
wrapperNodeName
parameter should be added to the XML documentation comments for this method to keep the API docs up to date.
private static AppConfigConfigurationSource ConfigureSource(
var wrappedConfigBytes = System.Text.Encoding.UTF8.GetBytes(wrappedConfig); | ||
await wrappedStream.WriteAsync(wrappedConfigBytes, 0, wrappedConfigBytes.Length).ConfigureAwait(false); | ||
wrappedStream.Position = 0; | ||
} | ||
|
||
private async Task<IDictionary<string,string>> GetDataFromLambdaExtensionAsync() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The WrapperNodeName
wrapping is not applied in the GetDataFromLambdaExtensionAsync
path, so feature-flag JSON returned by the Lambda extension won’t be nested. Consider invoking AddWrapperNodeAsync
(as in the service path) before parsing when WrapperNodeName
is set.
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i was thinking about this comment. i couldn't think of a reason to not add the same logic here.
[Question] Can/should we add the new logic you added here too (or is there a specific reason you didn't)
var wrappedConfigBytes = System.Text.Encoding.UTF8.GetBytes(wrappedConfig); | ||
await wrappedStream.WriteAsync(wrappedConfigBytes, 0, wrappedConfigBytes.Length).ConfigureAwait(false); | ||
wrappedStream.Position = 0; | ||
} | ||
|
||
private async Task<IDictionary<string,string>> GetDataFromLambdaExtensionAsync() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i was thinking about this comment. i couldn't think of a reason to not add the same logic here.
[Question] Can/should we add the new logic you added here too (or is there a specific reason you didn't)
@@ -214,6 +214,51 @@ namespace CustomParameterProcessorExample | |||
} | |||
``` | |||
|
|||
### Simple AppConfig FeatureFlag Sample |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you also add a change file for this change https://github.com/aws/aws-dotnet-extensions-configuration/blob/main/CONTRIBUTING.md#adding-a-change-file-to-your-contribution-branch
thanks for the pr! i left a couple of small comments/questions @sandbrock-nd |
// | ||
|
||
// Map feature flag configuration | ||
builder.Services.Configure<Dictionary<string, AppConfigFeatureFlag>>(builder.Configuration.GetSection("FeatureFlags")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of injecting Dictionary<string, AppConfigFeatureFlag>
into the DI container, why not create an AppConfigFeatureFlagConfiguration
object that contains the dictionary you are trying to access? I'd prefer users to access the config using AppConfigFeatureFlagConfiguration
instead of Dictionary<string, AppConfigFeatureFlag>
Added support for AppConfig Feature Flag profiles by allowing the consumer to nest the resulting JSON in a named parent node.
Description
This aims to allow consumers to inject feature flags into their application. The user cannot control the schema for AppConfig Feature Flag profiles because they are controlled by AWS. The JSON format is like this:
This is not friendly with the .NET builder.Configuration system because there is no named wrapper node around the list of feature flags. The result is that each flag is nested directly under the root of the configuration hierarchy.
This update allows the consumer to pass in the name of a wrapper node to nest the AppConfig JSON inside of by using the extension method for AppConfig registration:
And then read the values from that configuration profile, by deserializing the JSON into a dictionary of feature flag class instances:
Motivation and Context
The end goal is to allow the consumer to inject feature flags into their controllers and minimal API class constructors, such as in the following:
This addresses the following feature request:
#228
Testing
Initial testing was done by creating a unit test. Follow-up testing was done by integrating the library into an application that consumed both configuration profiles and feature flag profiles. The tests validated both profiles loaded correctly and were injected into the services DI container.
Screenshots (if appropriate)
Types of changes
Checklist
License