[AppConfig] appconfig feature list/show and appconfig kv import/export: Support microsoft feature management schema
#30376
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 19 commits
November 13, 2024 14:14
…ineWanjau/azure-cli into cwanjau/supportFFV2Schema
…ineWanjau/azure-cli into cwanjau/supportFFV2Schema
️✔️AzureCLI-FullTest
|
️✔️AzureCLI-BreakingChangeTest
|
|
AppConfig |
added 5 commits
November 18, 2024 15:15
avanigupta
reviewed
Jan 8, 2025
Comment on lines
+926
to
+927
| elif feature_flag_version == FeatureFlagVersion.V1: | ||
| feature_flag_dict[FeatureFlagConstants.CONDITIONS] = default_conditions |
There was a problem hiding this comment.
this looks odd. which if statement is this elif corresponding to?
There was a problem hiding this comment.
It corresponds to the first if checking if conditions are None. If None and Feature Flag is version 1, we set the conditions to the default conditions.
avanigupta
reviewed
Jan 8, 2025
|
|
||
| # Backend returns conditions: {client_filters: None} for flags with no conditions. | ||
| # No need to write empty conditions to key-values. | ||
| if conditions.get(FeatureFlagConstants.CLIENT_FILTERS, None) is None and conditions.get(FeatureFlagConstants.REQUIREMENT_TYPE, None) is None: |
There was a problem hiding this comment.
you could simplify this:
if client_filters or requirement_type:
feature_flag_dict[FeatureFlagConstants.CONDITIONS] = conditions
elif feature_flag_version == FeatureFlagVersion.V1:
feature_flag_dict[FeatureFlagConstants.CONDITIONS] = default_conditions
There was a problem hiding this comment.
Makes sense. Updated
avanigupta
reviewed
Jan 8, 2025
| @@ -939,6 +989,25 @@ def is_feature_flag(kv): | |||
| return False | |||
|
|
|||
|
|
|||
| def convert_boolean_value(enabled, feature_name): | |||
There was a problem hiding this comment.
We can make this method generic for converting any boolean to string.
def convert_string_to_bool(value, feature_name)
There was a problem hiding this comment.
Sure, this has been updated
avanigupta
reviewed
Jan 8, 2025
| """ | ||
| enabled = None | ||
| telemetry_enabled = feature_telemetry_dict.get(FeatureFlagConstants.ENABLED, None) | ||
| if telemetry_enabled is not None: |
There was a problem hiding this comment.
Suggested change
| if telemetry_enabled is not None: | |
| if telemetry_enabled: |
There was a problem hiding this comment.
telemetry_enabled could be false and we don't want to remove it if its explictly defined as False
albertofori
reviewed
Jan 8, 2025
| FeatureFlagConstants.TELEMETRY, | ||
| FeatureFlagConstants.DISPLAY_NAME | ||
| } | ||
| if valid_fields.union(feature_flag_dict.keys()) == valid_fields: |
There was a problem hiding this comment.
Yes, nice catch! Updated
avanigupta
reviewed
Jan 8, 2025
| featurefilterdict = {} | ||
|
|
||
| for key, value in conditions_dict.items(): | ||
| if key == FeatureFlagConstants.CLIENT_FILTERS: | ||
| featurefilterdict[key] = [feature_filter.__dict__ for feature_filter in value] | ||
| if value is not None: |
There was a problem hiding this comment.
Suggested change
| if value is not None: | |
| if value: |
There was a problem hiding this comment.
I was wondering a case where we have conditions: {client_filters: [] } I was thinking we would want to retain this, if we just check it as suggested it might be removed.
albertofori
reviewed
Jan 8, 2025
Comment on lines
873
to
874
| description = feature_flag_dict.get(FeatureFlagConstants.DESCRIPTION, "") # assign empty string as default | ||
| feature_flag_dict[FeatureFlagConstants.DESCRIPTION] = description |
There was a problem hiding this comment.
Suggested change
| description = feature_flag_dict.get(FeatureFlagConstants.DESCRIPTION, "") # assign empty string as default | |
| feature_flag_dict[FeatureFlagConstants.DESCRIPTION] = description | |
| feature_flag_dict[FeatureFlagConstants.DESCRIPTION] = feature_flag_dict.get(FeatureFlagConstants.DESCRIPTION, "") # assign empty string as default |
albertofori
reviewed
Jan 8, 2025
Comment on lines
936
to
937
| variant_list = [] | ||
| if variants: |
There was a problem hiding this comment.
Suggested change
| variant_list = [] | |
| if variants: | |
| if variants: | |
| variant_list = [] |
| if variants: | ||
| for variant in variants: | ||
| feature_variant = FeatureVariant.convert_from_dict(variant) | ||
| if feature_variant: |
There was a problem hiding this comment.
Is there a condition under which convert_from_dict can retun None?
Should we rather check if variant is not None first before trying to convert?
There was a problem hiding this comment.
Yes, we should. This has been updated
zhoxing-ms
modified the milestones:
January 2025 (2025-01-14),
February 2025 (2025-02-11)
added 2 commits
January 8, 2025 11:07
albertofori
reviewed
Jan 9, 2025
Comment on lines
+215
to
+218
| for keywordset in FeatureManagementReservedKeywords.ALL: | ||
| if keywordset.feature_management == dotnet_schema_keyword: | ||
| feature_management_keywords = keywordset | ||
| break |
There was a problem hiding this comment.
There was a problem hiding this comment.
I don't think there is a need to do that since the keywords are already defined in the keywordset.
albertofori
approved these changes
Jan 13, 2025
avanigupta
approved these changes
Jan 15, 2025
added 2 commits
January 16, 2025 12:48
|
Hello @zhoxing-ms this PR is ready to be merged. |
ChristineWanjau
changed the title
appconfig feature list/show and appconfig kv import/export: Support new microsoft feature management schemaappconfig feature list/show and appconfig kv import/export: Support microsoft feature management schema
|
Since there are multiple changes in this PR, it is better to seperate the changelog into small ones and list in the History Notes section, e.g., |
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
yanzhudd
previously approved these changes
Jan 22, 2025
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
yanzhudd
approved these changes
Jan 22, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Related command
Description
This PR updates the import and export of features to support new feature management schema.
The currently supported schema is the Feature Managment Dotnet Schema.
This PR also adds the use of a new environment variable called [AZURE_APPCONFIG_FM_COMPATIBILE] when exporting to a file for backward compatibility for users. This ensures our systems can process data in both old and new formats, avoiding disruptions for users of the previous version and maintaining functionality during the transition.
Default compatibility mode to true.
If compatibility mode true:
Example,
{
"FeatureManagement":{
// old schema
}
"feature_management":{
"feature_flags": [
// new schema
]
}
}
If compatibility mode is false:
"feature_management":{
"feature_flags": [
// new schema
]
}
Link to design doc
Testing Guide
History Notes
[App Config]
az appconfig kv import: Support microsoft feature management schema[App Config]
az appconfig kv export: Support microsoft feature management schema[App Config]
az appconfig kv export: Introduce a new environment variable calledAZURE_APPCONFIG_FM_COMPATIBILEwhen exporting to a file for backward compatibility for users[App Config]
az appconfig feature show: Support microsoft feature management schema[App Config]
az appconfig feature list: Support microsoft feature management schemaThis checklist is used to make sure that common guidelines for a pull request are followed.
The PR title and description has followed the guideline in Submitting Pull Requests.
I adhere to the Command Guidelines.
I adhere to the Error Handling Guidelines.