-
Notifications
You must be signed in to change notification settings - Fork 0
Final changes for feature management CLI #2
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
Changes from 13 commits
8dcfa36
6e07f35
abc0352
d521e67
b27d8f5
2acc8de
515fa60
c074906
677dfdd
b3077ca
f6e31bb
d66bbdd
85b212a
3701942
f4bb43e
e0efa12
85f372c
8dc309f
0cf0c57
3c80690
ecd0015
9263886
a3d7624
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,18 +5,16 @@ | |
|
|
||
| from enum import Enum | ||
| import json | ||
| from knack.util import CLIError | ||
| from knack.log import get_logger | ||
|
|
||
| # pylint: disable=too-few-public-methods | ||
| # pylint: disable=too-many-instance-attributes | ||
|
|
||
| logger = get_logger(__name__) | ||
| FEATURE_FLAG_PREFIX = ".appconfig.featureflag/" | ||
| DEFAULT_CONDITIONS = {'client_filters':[]} | ||
| DEFAULT_CONDITIONS = {'client_filters': []} | ||
|
|
||
| # Feature Flag Models # | ||
|
|
||
|
|
||
| class FeatureState(Enum): | ||
| OFF = 1 | ||
| ON = 2 | ||
|
|
@@ -34,7 +32,7 @@ class FeatureQueryFields(Enum): | |
| ALL = KEY | LABEL | LAST_MODIFIED | LOCKED | STATE | DESCRIPTION | CONDITIONS | ||
|
|
||
|
|
||
| class FeatureFlagDisplay(object): | ||
| class FeatureFlag(object): | ||
| ''' | ||
| Feature Flag schema as displayed to the user. | ||
|
|
||
|
|
@@ -56,14 +54,14 @@ class FeatureFlagDisplay(object): | |
| Dictionary that contains client_filters List (and server_filters List in future) | ||
| ''' | ||
|
|
||
| def __init__(self, | ||
| key, | ||
| label=None, | ||
| state=None, | ||
| description=None, | ||
| conditions=None, | ||
| locked=None, | ||
| last_modified=None): | ||
| def __init__(self, | ||
| key, | ||
| label=None, | ||
| state=None, | ||
| description=None, | ||
| conditions=None, | ||
| locked=None, | ||
| last_modified=None): | ||
| self.key = key | ||
| self.label = label | ||
| self.state = state.name.lower() | ||
|
|
@@ -73,7 +71,7 @@ def __init__(self, | |
| self.locked = locked | ||
|
|
||
| def __str__(self): | ||
| featureflagdisplay = { | ||
| featureflag = { | ||
| "Key": self.key, | ||
| "Label": self.label, | ||
| "State": self.state, | ||
|
|
@@ -83,22 +81,22 @@ def __str__(self): | |
| "Conditions": custom_serialize_conditions(self.conditions) | ||
| } | ||
|
|
||
| return json.dumps(featureflagdisplay, indent=2) | ||
| return json.dumps(featureflag, indent=2) | ||
|
|
||
|
|
||
| class FeatureFilter(object): | ||
| ''' | ||
| Feature filters class. | ||
|
|
||
| :ivar str Name: | ||
| Name of the filter | ||
| :ivar dict {str, str} parameters: | ||
| Name-Value pairs of parameters | ||
| ''' | ||
|
|
||
| def __init__(self, | ||
| name, | ||
| parameters=None): | ||
| def __init__(self, | ||
| name, | ||
| parameters=None): | ||
| self.name = name | ||
| self.parameters = parameters | ||
|
|
||
|
|
@@ -107,9 +105,7 @@ def __repr__(self): | |
| "name": self.name, | ||
| "parameters": self.parameters | ||
| } | ||
| return json.dumps(featurefilter,indent=2) | ||
|
|
||
|
|
||
| return json.dumps(featurefilter, indent=2) | ||
|
|
||
|
|
||
| # Feature Flag Exceptions # | ||
|
|
@@ -126,133 +122,161 @@ def __init__(self, message): | |
| super(UnsupportedValuesException, self).__init__(message) | ||
|
|
||
|
|
||
|
|
||
| # Feature Flag Helper Functions # | ||
|
|
||
| def custom_serialize_conditions(conditions_dict): | ||
| ''' | ||
| Helper Function to serialize Conditions | ||
|
|
||
| Input: conditions_dict | ||
| Dictionary of {str, List[FeatureFilter]} | ||
| Args: | ||
| conditions_dict - Dictionary of {str, List[FeatureFilter]} | ||
|
|
||
| Return: JSON serializable Dictionary | ||
| Return: | ||
| JSON serializable Dictionary | ||
| ''' | ||
| featurefilterdict = {} | ||
| if conditions_dict: | ||
| for key,value in conditions_dict.items(): | ||
| featurefilters = [] | ||
| for filter in value: | ||
| featurefilters.append(str(filter)) | ||
| for key, value in conditions_dict.items(): | ||
| featurefilters = [] | ||
| for ff in value: | ||
| featurefilters.append(str(ff)) | ||
| featurefilterdict[key] = featurefilters | ||
| return featurefilterdict | ||
|
|
||
|
|
||
| def map_keyvalue_to_featureflagdisplay(keyvalue, show_conditions=True): | ||
| def map_keyvalue_to_featureflag(keyvalue, show_conditions=True): | ||
| ''' | ||
| Helper Function to convert KeyValue object to FeatureFlagDisplay object | ||
|
|
||
| Input: keyvalue | ||
| KeyValue object to be converted | ||
| Helper Function to convert KeyValue object to FeatureFlag object | ||
|
|
||
| Input: show_conditions | ||
| Boolean for controlling whether we want to display "Conditions" or not | ||
| Args: | ||
| keyvalue - KeyValue object to be converted | ||
| show_conditions - Boolean for controlling whether we want to display "Conditions" or not | ||
|
|
||
| Return: FeatureFlagDisplay object | ||
| Return: | ||
| FeatureFlag object | ||
| ''' | ||
| key = getattr(keyvalue, 'key') | ||
| feature_name = key[len(FEATURE_FLAG_PREFIX):] | ||
| valuestr = getattr(keyvalue, 'value', "") | ||
|
|
||
| # we check that value retrieved is a valid json and only has the fields supported by backend. | ||
| # if it's invalid, we throw exception | ||
| # we check that value retrieved is a valid json and only has the fields supported by backend. | ||
| # if it's invalid, we throw exception | ||
| # For all other exceptions, we let the outer try/except handle it. | ||
| try: | ||
| feature_flag_value = map_valuestr_to_valuedict(keyvalue) | ||
| feature_flag_value = map_valuestr_to_valuedict(valuestr) | ||
| except (UnsupportedValuesException, InvalidJsonException) as exception: | ||
| raise ValueError(f"Invalid Value found for Key '{key}'. Aborting operation\n" + str(exception)) | ||
|
|
||
| raise ValueError( | ||
| f"Invalid value found for feature '{feature_name}'. Aborting operation\n" + | ||
| str(exception)) | ||
|
|
||
| state = FeatureState.OFF | ||
| if feature_flag_value.get('enabled', False): | ||
| state = FeatureState.ON | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we should use if feature_flag_value.get('enabled', False) here. Generally for an object, we can directly reference its attribute which can be null or other default value. However, for a dictionary, if we do dict[key], it will throw error if dict doens't contain the key. So for dict.get(key, default) is like try-get key, return default if key is not found. #Resolved
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In this case, the dictionary definitely contains this field because we validate that in map_valuestr_to_valuedict (line 164) In reply to: 329185221 [](ancestors = 329185221) |
||
|
|
||
| conditions = feature_flag_value.get('conditions', DEFAULT_CONDITIONS) | ||
|
|
||
| # if conditions["client_filters"] list is not empty, make state conditional | ||
| filters = conditions.get("client_filters", []) | ||
| if filters and state == FeatureState.ON: | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
nit: if "client_filters" in filters and state == FeatureState.ON, then remove line 196 #Resolved
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I need to check if the list of "client_filters" is empty or not. ( if "client_filters" in conditions ) will always be true. In reply to: 332256557 [](ancestors = 332256557) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yep, I was meaning "client_filters" in considitions. But we still need to check if the client_filters is empty of not. In line 196 just a little concern that we directly reference a hard coded key which can potentially throw exception. We can give it a default value. filters = conditions.get("client_filters", []) In reply to: 332275941 [](ancestors = 332275941,332256557)
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. map_keyvalue_to_featureflagvalue() will ensure that we have at least the default conditions at all times, which means it will always return empty list if "client_filters" list is empty. So we don't need to check again here. In reply to: 332285703 [](ancestors = 332285703,332275941,332256557) |
||
| state = FeatureState.CONDITIONAL | ||
| feature_flag_display = FeatureFlagDisplay(feature_name, | ||
| getattr(keyvalue, 'label', ""), | ||
| state, | ||
| feature_flag_value.get('description', ""), | ||
| conditions, | ||
| getattr(keyvalue, 'locked', False), | ||
| getattr(keyvalue, 'last_modified', "")) | ||
|
|
||
| feature_flag = FeatureFlag(feature_name, | ||
| getattr(keyvalue, 'label', ""), | ||
| state, | ||
| feature_flag_value.get('description', ""), | ||
| conditions, | ||
| getattr(keyvalue, 'locked', False), | ||
| getattr(keyvalue, 'last_modified', "")) | ||
|
|
||
| # By Default, we will try to show conditions unless the user has | ||
| # specifically filtered them using --fields arg. | ||
| # But in some operations like 'Delete feature', we don't want | ||
| # specifically filtered them using --fields arg. | ||
| # But in some operations like 'Delete feature', we don't want | ||
| # to display all the conditions as a result of delete operation | ||
| if not show_conditions: | ||
| del feature_flag_display.conditions | ||
| return feature_flag_display | ||
| del feature_flag.conditions | ||
| return feature_flag | ||
|
|
||
|
|
||
| def map_valuestr_to_valuedict(keyvalue): | ||
| def map_valuestr_to_valuedict(valuestr): | ||
| ''' | ||
| Helper Function to convert value string to a VALID value dictionary. | ||
| Throws Exception if value is invalid. | ||
|
|
||
| Input: keyvalue | ||
| KeyValue object to be converted | ||
|
|
||
| Return: Valid value dictionary | ||
| Args: | ||
| valuestr - value string from KeyValue object | ||
|
|
||
| Return: | ||
| Valid value dictionary | ||
|
|
||
| Raises: | ||
| UnsupportedValuesException: raised when feature flag value is missing required | ||
| fields or contains other invalid fields | ||
| InvalidJsonException: raised when JSON decode error is thrown because value | ||
| string cannot be deserialized to a valid JSON | ||
|
|
||
| Raises: | ||
| UnsupportedValuesException: raised when feature flag value is missing required fields or contains other invalid fields | ||
| InvalidJsonException: raised when JSON decode error is thrown because value string cannot be deserialized to a valid JSON | ||
| ''' | ||
|
|
||
| feature_flag_value = {} | ||
| key = getattr(keyvalue, 'key') | ||
| feature_name = key[len(FEATURE_FLAG_PREFIX):] | ||
|
|
||
| valuestr = getattr(keyvalue, 'value', "") | ||
| if valuestr: | ||
| try: | ||
| # Make sure value string is a valid json | ||
| feature_flag_value = json.loads(valuestr) | ||
|
|
||
| # Make sure value json has all the fields we support in the backend | ||
| valid_fields = {'id', 'description', 'enabled', 'label', 'conditions'} | ||
| valid_fields = { | ||
| 'id', | ||
| 'description', | ||
| 'enabled', | ||
| 'label', | ||
| 'conditions'} | ||
| if valid_fields != feature_flag_value.keys(): | ||
| error_msg = f"This feature flag cannot be processed because it is missing required values or it contains unsupported values.\n" | ||
| raise UnsupportedValuesException(f"Feature flag {feature_name} contains invalid value. " + error_msg) | ||
|
|
||
| error_msg = f"This feature flag cannot be processed because it is missing " + \ | ||
| "required values or it contains unsupported values.\n" | ||
| raise UnsupportedValuesException( | ||
| "Invalid value.\n" + error_msg) | ||
|
|
||
| except UnsupportedValuesException as exception: | ||
| raise UnsupportedValuesException(str(exception)) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why we need to catch and throw the same exception #Resolved
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because I'm catching specifically this type of exception in the calling function. If I don't do this, it will be thrown as basic Exception In reply to: 328805633 [](ancestors = 328805633) |
||
|
|
||
| except ValueError as exception: | ||
| error_msg = f"Unable to decode the following JSON value: \n{valuestr}. \nFull Exception: \n{str(exception)}" | ||
| raise InvalidJsonException(f"Feature flag {feature_name} contains invalid value. " + error_msg) | ||
| error_msg = f"Unable to decode the following JSON value: \n{valuestr}. \nFull exception: \n{str(exception)}" | ||
| raise InvalidJsonException("Invalid value.\n" + error_msg) | ||
|
|
||
| except Exception as exception: | ||
| error_msg = f"Exception while parsing value for feature: {feature_name}\nValue: {valuestr}\n" | ||
| error_msg = f"Exception while parsing value:\n{valuestr}\n" | ||
| raise Exception(error_msg + str(exception)) | ||
|
|
||
| return feature_flag_value | ||
|
|
||
|
|
||
| def map_json_to_featurefilter(json_object): | ||
| featurefilters = FeatureFilter(__get_value(json_object, 'name'), | ||
| __get_value(json_object, 'parameters')) | ||
| return featurefilters | ||
| def map_valuestr_to_featurefilter_list(valuestr): | ||
| ''' | ||
| Helper Function to extract Feature Filters from KeyValue object | ||
|
|
||
| Args: | ||
| valuestr - Value string from KeyValue Object | ||
|
|
||
| Return: | ||
| List containing FeatureFilter Objects | ||
| ''' | ||
|
|
||
| # we check that value retrieved is a valid json and only has the fields supported by backend. | ||
| # if it's invalid, we throw exception | ||
| # For all other exceptions, we let the outer try/except handle it. | ||
| try: | ||
| feature_flag_value = map_valuestr_to_valuedict(valuestr) | ||
| except (UnsupportedValuesException, InvalidJsonException) as exception: | ||
| raise ValueError( | ||
| f"Invalid value. Aborting operation\n" + | ||
| str(exception)) | ||
|
|
||
| conditions = feature_flag_value.get('conditions', DEFAULT_CONDITIONS) | ||
| filters = conditions.get("client_filters", []) | ||
|
|
||
| return filters | ||
|
|
||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should we map to like of FeatureFilter instead of list of dict and rely on this method to guarantee the dict has same attributes as the FeatureFilter object. #Resolved |
||
|
|
||
| def __get_value(item, argument): | ||
| try: | ||
| return item[argument] | ||
| except (KeyError, TypeError, IndexError): | ||
| return None | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
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.
nit:key.value #Resolved