-
Notifications
You must be signed in to change notification settings - Fork 2k
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
App Config Spring - Remove Feature Flags as Config #44558
base: SpringCloudAzure6.0-Preview
Are you sure you want to change the base?
App Config Spring - Remove Feature Flags as Config #44558
Conversation
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.
PR Overview
This PR ensures that feature flags are no longer loaded as configuration, but are instead processed as dedicated feature flags when loading a snapshot.
- Refactored TestUtils to add a new overload for creating feature flag configuration settings.
- Modified AppConfigurationApplicationSettingPropertySource to return early in handleFeatureFlag.
- Updated tests to filter out feature flags from configuration setting key extraction and adjusted the FEATURE_ITEM creation with an eTag.
Reviewed Changes
File | Description |
---|---|
sdk/spring/spring-cloud-azure-appconfiguration-config/src/test/java/com/azure/spring/cloud/appconfiguration/config/implementation/TestUtils.java | Added a new overloaded method for creating feature flag configuration settings and adjusted inner loop processing. |
sdk/spring/spring-cloud-azure-appconfiguration-config/src/main/java/com/azure/spring/cloud/appconfiguration/config/implementation/AppConfigurationApplicationSettingPropertySource.java | Updated handleFeatureFlag to bypass processing of feature flags as configuration. |
sdk/spring/spring-cloud-azure-appconfiguration-config/src/test/java/com/azure/spring/cloud/appconfiguration/config/implementation/AppConfigurationApplicationSettingPropertySourceTest.java | Modified tests to exclude feature flag settings from configuration key extraction. |
sdk/spring/spring-cloud-azure-appconfiguration-config/src/test/java/com/azure/spring/cloud/appconfiguration/config/implementation/AppConfigurationApplicationSettingPropertySourceSnapshotTest.java | Updated FEATURE_ITEM creation to include an eTag and ensure proper instantiation. |
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (1)
sdk/spring/spring-cloud-azure-appconfiguration-config/src/test/java/com/azure/spring/cloud/appconfiguration/config/implementation/TestUtils.java:81
- The inner loop redundantly converts the entire nodeParams object on each iteration. Consider moving the conversion outside of the loop so that it is executed only once, then iterate over the resulting map to add parameters.
for (int j = 0; j < nodeParams.size(); j++) {
@@ -127,7 +127,8 @@ private void handleKeyVaultReference(String key, SecretReferenceConfigurationSet | |||
|
|||
void handleFeatureFlag(String key, FeatureFlagConfigurationSetting setting, List<String> trimStrings) | |||
throws InvalidConfigurationPropertyValueException { | |||
handleJson(setting, trimStrings); | |||
// Feature Flags aren't loaded as configuration, but are loaded as feature flags when loading a snapshot. | |||
return; |
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.
Is potentially a breaking change. Let's only release this in a major deploy.
Description
Feature Flags shouldn't be loaded as configuration. If they are loaded via snapshot they are loaded as actual feature flags.