-
Notifications
You must be signed in to change notification settings - Fork 5.1k
[App Configuration] Removed enabled/conditions from feature flag required properties #54195
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: main
Are you sure you want to change the base?
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.
Pull request overview
This PR makes the enabled and conditions properties optional for feature flags, aligning with the Feature Flag v2.0.0 schema where only id is required. This enhances flexibility by allowing minimal feature flag definitions.
Key Changes:
- Removed
enabledandconditionsfrom the required properties validation array - Added comprehensive test coverage for feature flags with optional properties (only
id, withoutconditions, withoutenabled) - Tests verify that missing optional properties default correctly (
enableddefaults tofalse,ClientFiltersis empty)
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| sdk/appconfiguration/Azure.Data.AppConfiguration/src/Models/FeatureFlagConfigurationSetting.cs | Updated required properties array to only include "id", making "enabled" and "conditions" optional for validation |
| sdk/appconfiguration/Azure.Data.AppConfiguration/tests/FeatureFlagConfigurationSettingTests.cs | Added 6 new tests covering parsing and modification scenarios for feature flags with optional properties |
| [Test] | ||
| public void FeatureFlagWithoutConditionsIsValid() | ||
| { | ||
| var featureFlagValue = "{\"id\":\"my feature\",\"enabled\":true}"; | ||
| var featureFlag = new FeatureFlagConfigurationSetting(); | ||
| featureFlag.Value = featureFlagValue; | ||
|
|
||
| Assert.AreEqual("my feature", featureFlag.FeatureId); | ||
| Assert.AreEqual(true, featureFlag.IsEnabled); | ||
| Assert.AreEqual(0, featureFlag.ClientFilters.Count); | ||
| } |
Copilot
AI
Nov 27, 2025
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.
Missing test coverage: There should be a test verifying that a feature flag with only id can roundtrip without modification. Currently, the CanRountripValue test doesn't include test cases for minimal feature flags (e.g., {"id":"my feature"}).
Consider adding test cases to CanRountripValue or creating a new test that verifies:
var featureFlagValue = "{\"id\":\"my feature\"}";
var featureFlag = new FeatureFlagConfigurationSetting();
featureFlag.Value = featureFlagValue;
using var expected = JsonDocument.Parse(featureFlagValue);
using var actual = JsonDocument.Parse(featureFlag.Value);
Assert.IsTrue(_jsonComparer.Equals(expected.RootElement, actual.RootElement));This would ensure that reading and serializing without modification preserves the original minimal JSON structure.
Making
enabledandconditionsoptional according to the Feature Flag schema, where onlyidis required.