[AppConfig] Support Import/Export of feature flags #11467
Conversation
* FeatureManagement CLI Part 1 FeatureManagement initial code setup and implementation of "show feature" and "set feature" commands * Adding "delete" and "list" feature commands * Adding "set" and "show" feature commands * Add "enable", "disable", "lock", "unlock" commands
* FeatureManagement CLI Part 1 FeatureManagement initial code setup and implementation of "show feature" and "set feature" commands * Adding "delete" and "list" feature commands * Resolving Comments * Add "enable", "disable", "lock", "unlock" commands * Resolving comments * Resolving comments * setup for filter commands * Adding filter commands * Adding Unit Tests * Fix styling issues * test recordings * Update release history * resolving comments * resolving comments * Minor change * Add FeatureFlagValue Model * Validate filter parameters * Fix formatting of _params.py * Change variable names * Log warning when delete filter index mismatch * Resolving comments
| set_kv.last_modified = featureflag.last_modified | ||
|
|
||
| except: | ||
| logger.debug("Exception while converting feature flag to key value:\n%s\n", featureflag) |
There was a problem hiding this comment.
One question here is if the exception raised will throw clear error message.
If not, what about make logger.debug message here be part of exception message?
| config_data = json.load(config_file) | ||
| if 'FeatureManagement' in config_data: | ||
| del config_data['FeatureManagement'] | ||
| elif format_ == 'yaml': |
There was a problem hiding this comment.
Should you log warning here as you said in https://github.com/Azure/azure-cli/pull/11467/files#r356356673?
|
No other comment from CLI side. Please resolve left comments and make CI pass. We will freeze code tomorrow for the next release. If you want to make it merge before code freeze, please make your PR ready ASAP. |
| return key_values | ||
|
|
||
|
|
||
| def __read_features_from_config_store(cmd, name=None, connection_string=None, key=None, label=None): |
There was a problem hiding this comment.
__read_features_from_config_store [](start = 4, length = 33)
this helper method looks unnecessary. caller can call list_feature directly. Also it is odd to let this internal helper method to depend on feature.py. #Resolved
There was a problem hiding this comment.
|
|
||
| feature_entry = {feature.key: feature_state} | ||
|
|
||
| if format_ == 'json': |
There was a problem hiding this comment.
no need to check format here. Already done before #Resolved
There was a problem hiding this comment.
| key_values = [] | ||
| default_conditions = {'client_filters': []} | ||
| default_value = FeatureFlagValue(id_="") | ||
|
|
There was a problem hiding this comment.
can you create a new FeatureFlagValue instance inside of the for loop in line 613? Otherwise, we have to be careful that every iteration, we will clean the last round default. The default_conditions does make sense as it is an actual default value. #Resolved
| # We support alternate format for specifying always OFF features | ||
| # "FeatureU": {"EnabledFor": []} | ||
| default_value.enabled = False | ||
| default_value.conditions = default_conditions |
There was a problem hiding this comment.
when every iteration you create a FeatureFlagValue, you can assign the default condition to it. Then in here, you don't need the if check again. #Resolved
| feature_json = {'state': feature.state} | ||
| # description | ||
| feature_json['description'] = feature.description | ||
| # conditions |
There was a problem hiding this comment.
why we compare description in here but not in __serialize_features_from_kv_list_to_comparable_json_object #Resolved
There was a problem hiding this comment.
| # conditions | ||
| feature_json['conditions'] = feature.conditions | ||
| # key | ||
| res[feature.key] = feature_json |
There was a problem hiding this comment.
can we reuse __serialize_feature_list_to_comparable_json_object? #Resolved
There was a problem hiding this comment.
| # to simplify output, add one shared key in src and dest configuration | ||
| new_json['@base'] = '' | ||
| old_json['@base'] = '' | ||
| differ = JsonDiffer(syntax='symmetric') |
There was a problem hiding this comment.
symmetric [](start = 32, length = 9)
why we use symmetric for feature flag but explicit for key values? #Resolved
There was a problem hiding this comment.
explicit syntax doesn't work when comparing feature filters, probably due to whitespacing issue with the json formatting of filters.
In reply to: 356899926 [](ancestors = 356899926)
| if format_ == 'json': | ||
| for k, v in features_dict.items(): | ||
| key = FEATURE_FLAG_PREFIX + str(k) | ||
| default_value = FeatureFlagValue(id_=str(k)) |
There was a problem hiding this comment.
default_value [](start = 16, length = 13)
nit: rename it to something meaningful, like feature = FeatureFlagValue(id_=str(k))
4 participants
This checklist is used to make sure that common guidelines for a pull request are followed.
The PR has modified HISTORY.rst describing any customer-facing, functional changes. Note that this does not include changes only to help content. (see Modifying change log).
I adhere to the Command Guidelines.