Skip to content

Conversation

@avanigupta
Copy link
Owner


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.

arg_type=get_enum_type(['key', 'label', 'locked' ,'last_modified', 'state', 'description', 'conditions'])
arg_type=get_enum_type(['key', 'label', 'locked', 'last_modified', 'state', 'description', 'conditions'])
)
filter_parameters_arg_type = CLIArgumentType(
Copy link

@shenmuxiaosen shenmuxiaosen Sep 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

filter_parameters_arg_type [](start = 4, length = 26)

can we reuse the tags_type natively supported by CLI core? #Resolved

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, because they only support argument name "tags". Also, we need to slightly modify the logic to convert duplicate params into array instead of overwriting (see line 120 in _validators.py)


In reply to: 328766626 [](ancestors = 328766626)

text:
az appconfig feature filter add -n MyAppConfiguration --feature color --label MyLabel --filterName MyFilter --filterParameters Name=Value Name2=Value2
- name: Add a filter with name 'MyFilter' using connection string.
text:
Copy link

@shenmuxiaosen shenmuxiaosen Sep 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add an example for adding with index #Resolved

c.argument('feature', help='Name of the feature from which you want to delete the filter.')
c.argument('label', help="If no label specified, delete from the feature flag with null label by default.")
c.argument('filterName', help='Name of the filter to be deleted.')
c.argument('index', help='Zero-based index of the filter to be deleted in case ther are multiple instances with same filter name.')
Copy link

@shenmuxiaosen shenmuxiaosen Sep 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ther [](start = 87, length = 4)

nit:there #Resolved

"Invalid value.\n" + error_msg)

except UnsupportedValuesException as exception:
raise UnsupportedValuesException(str(exception))
Copy link

@shenmuxiaosen shenmuxiaosen Sep 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why we need to catch and throw the same exception #Resolved

Copy link
Owner Author

Choose a reason for hiding this comment

The 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)

value = map_valuestr_to_valuedict(retrieved_kv)
value = map_valuestr_to_valuedict(
getattr(retrieved_kv, 'value', ""))
except (UnsupportedValuesException, InvalidJsonException) as exception:
Copy link

@shenmuxiaosen shenmuxiaosen Sep 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need to use getattr and give a default value. Since retrieved_kv is a kv object, you can just use retrieved_kv.key. Also in map_valuestr_to_valuedict() you already handled the case if value is null or empty. #Resolved

try:
value = map_valuestr_to_valuedict(
getattr(retrieved_kv, 'value', ""))
except (UnsupportedValuesException, InvalidJsonException) as exception:
Copy link

@shenmuxiaosen shenmuxiaosen Sep 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

retrieved_kv.value #Resolved

str(exception))

value['enabled'] = True
confirmation_message = "Are you sure you want to enable this feature '{}' ?".format(
Copy link

@shenmuxiaosen shenmuxiaosen Sep 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[](start = 85, length = 1)

nit:remove the white space #Resolved

keyvalue=updated_key_value, show_conditions=False)

except ValueError as exception:
raise CLIError(str(exception))
Copy link

@shenmuxiaosen shenmuxiaosen Sep 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need to catch here. It will be caught later in line 492 #Resolved

feature_filters.clear()

updated_key_value = __update_existing_key_value(
azconfig_client,
Copy link

@shenmuxiaosen shenmuxiaosen Sep 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if the feature_filter is empty, why we need to update the keyvalue. Also maybe we should move the validation before asking for confirmation. After we retrieved the keyvalue and map to feature flag, we check filters. If no filter found, then no op. #Resolved

display_filters = []
if feature_filters:
display_filters = copy.deepcopy(feature_filters)
feature_filters.clear()
Copy link

@shenmuxiaosen shenmuxiaosen Sep 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can map the return value from _update_exisiting_key_value method. No need to allocate beforehand, as the update operation can fail. #Resolved

raise CLIError(str(exception))
raise CLIError(str(exception))
except Exception as exception:
raise CLIError(str(exception))
Copy link

@shenmuxiaosen shenmuxiaosen Sep 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just catch the general Exception here. Since no action difference for HttpException and Exception. #Resolved

c.argument('filterName', help='Name of the filter to be added.')
c.argument('filterParameters', arg_type=filter_parameters_arg_type)
c.argument('index', help='Zero-based index in the list of filters where you want to insert the new filter.')

Copy link

@shenmuxiaosen shenmuxiaosen Sep 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

type=int #Resolved

# If user has specified index, we use it as secondary check to display
# a unique filter
if 0 <= index < len(feature_filters):
if feature_filters[index].get(
Copy link

@shenmuxiaosen shenmuxiaosen Sep 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given a filter name, we should first loop over the client_filter client. If there is a match we compare current index with passed in index. something like:
for i in range(len(feature_filters)):
if name match:
check if index match. #Resolved

# user
for idx, ff in enumerate(feature_filters):
if ff['name'].lower() == filterName.lower():
match_index.append(idx)
Copy link

@shenmuxiaosen shenmuxiaosen Sep 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we are case insensitive for filter name? #Resolved

for idx, ff in enumerate(feature_filters):
if ff['name'].lower() == filterName.lower():
match_index.append(idx)

Copy link

@shenmuxiaosen shenmuxiaosen Sep 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can compare idx with the index passed-in here. If there is no match after looping over the feature_filters, just error out. #Resolved

str(exception))

return feature_flag_value['conditions']['client_filters']

Copy link

@shenmuxiaosen shenmuxiaosen Sep 27, 2019

Choose a reason for hiding this comment

The 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

feature_filters.insert(index, new_filter.__dict__)
else:
logger.debug("Adding new filter to the end of list.\n")
feature_filters.append(new_filter.__dict__)
Copy link

@shenmuxiaosen shenmuxiaosen Sep 27, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: we can also log for index out of range #Resolved


if len(match_index) == 1:
display_filter = copy.deepcopy(
feature_filters[match_index[0]])
Copy link

@shenmuxiaosen shenmuxiaosen Sep 27, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need somehow log warning if there is no duplicate but index and parameter name mismatch. #Resolved


# get all filters where name matches filterName provided by user
display_filters = [
ff for ff in feature_filters if ff['name'] == filterName]
Copy link

@shenmuxiaosen shenmuxiaosen Sep 27, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ff [](start = 12, length = 2)

nit: please don't name variables like "ff" especially in our case, it can short for feature flag and feature filter. So maybe just name it "filter" #Resolved

result = (comps[0], comps[1]) if len(comps) > 1 else (string, '')
else:
logger.warning("Ignoring filter parameter '%s' because parameter name is empty.", string)
return result
Copy link

@shenmuxiaosen shenmuxiaosen Oct 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if parameter key is null, we should stop and error out instead of continuing. #Resolved

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mirroring the behavior from portal, I thought we could just ignore the parameters with empty name and continue with the rest of changes just like portal allows users to do it.


In reply to: 332228746 [](ancestors = 332228746)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

considering we will log warning. it is fine to ignore it.


In reply to: 332276635 [](ancestors = 332276635,332228746)

# Ignore invalid arguments like '=value' or '='
if comps[0]:
result = (comps[0], comps[1]) if len(comps) > 1 else (string, '')
else:
Copy link

@shenmuxiaosen shenmuxiaosen Oct 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if len(comps) <= 1, throw error. For instance, if --parameter "abc", we can't assume "abc" is the parameter name. #Resolved

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we allow parameters to have null value, having "abc" argument feels more intuitive than asking users to specify "abc=". This is also aligned with how CLI handles tags in a similar way.


In reply to: 332229424 [](ancestors = 332229424)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if tags also behave like this, we should be consistent with tags.


In reply to: 332277050 [](ancestors = 332277050,332229424)

filter_parameters_dict = {}
for item in namespace.filterParameters:
param_tuple = validate_filter_parameter(item)
if param_tuple:
Copy link

@shenmuxiaosen shenmuxiaosen Oct 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

param_name, param_value = validate_filter_parameter(item), no need to check param_tuple. It will either return a valid (name, value) or throw error. #Resolved

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we are not throwing error, this needs to be checked here.


In reply to: 332230700 [](ancestors = 332230700)

c.argument('filterName', help='Name of the filter to be added.')
c.argument('filterParameters', arg_type=filter_parameters_arg_type)
c.argument('index', help='Zero-based index in the list of filters where you want to insert the new filter.')

Copy link

@shenmuxiaosen shenmuxiaosen Oct 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: can you add the default behavior if no index specified in help message? #Resolved

error_msg = f"Exception while parsing value for feature: {feature_name}\nValue: {valuestr}\n"
raise Exception(error_msg + str(exception))
feature_flag_dict = {}
default_conditions = {'client_filters': []}
Copy link

@shenmuxiaosen shenmuxiaosen Oct 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: no need to initialize it here. #Resolved

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just felt it looks more readable in line 249 if I give it a name. Thoughts?


In reply to: 332254096 [](ancestors = 332254096)

for client_filter in client_filters:
# If there is a filter, it should always have a name
# In case it doesn't, ignore this filter
name = client_filter.get('name')
Copy link

@shenmuxiaosen shenmuxiaosen Oct 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: log the case name is missing. #Resolved

except ValueError as exception:
error_msg = f"Invalid value. Unable to decode the following JSON value: \n{keyvalue.value}." + \
f"\nFull exception: \n{str(exception)}"
raise ValueError("Invalid value.\n" + error_msg)
Copy link

@shenmuxiaosen shenmuxiaosen Oct 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Invalid value.\n" [](start = 25, length = 18)

nit: duplicate error message. #Resolved

filters = conditions.get("client_filters", [])
filters = conditions["client_filters"]

if filters and state == FeatureState.ON:
Copy link

@shenmuxiaosen shenmuxiaosen Oct 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

filters [](start = 7, length = 7)

nit: if "client_filters" in filters and state == FeatureState.ON, then remove line 196 #Resolved

Copy link
Owner Author

@avanigupta avanigupta Oct 7, 2019

Choose a reason for hiding this comment

The 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)

Choose a reason for hiding this comment

The 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", [])
if filters and state == FeatureState.ON:


In reply to: 332275941 [](ancestors = 332275941,332256557)

Copy link
Owner Author

Choose a reason for hiding this comment

The 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)

c.argument('filterName', help='Name of the filter to be added.')
c.argument('filterParameters', arg_type=filter_parameters_arg_type)
c.argument('index', help='Zero-based index in the list of filters where you want to insert the new filter.')

Copy link

@shenmuxiaosen shenmuxiaosen Oct 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

type=int #Resolved

if 0 <= index <= len(feature_filters):
logger.debug("Adding new filter at index '%s'.\n", index)
feature_filters.insert(index, new_filter.__dict__)
else:
Copy link

@shenmuxiaosen shenmuxiaosen Oct 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

feature_filters is a list of FeatureFilter object, we should insert new_filter directly instead of a dict. #Resolved


updated_key_value = __update_existing_key_value(
azconfig_client, retrieved_kv=retrieved_kv, updated_value=json.dumps(
feature_flag_value.__dict__, default=lambda o: o.__dict__))
Copy link

@shenmuxiaosen shenmuxiaosen Oct 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: can you check the formatting here. Looks odd. #Resolved

# this means we have not deleted the filter yet
if len(match_index) == 1:
if index != -1:
logger.warning("Found filter '%s' at index '%s'. Ignoring provided index '%s'", filterName, match_index[0], index)
Copy link

@shenmuxiaosen shenmuxiaosen Oct 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we didn't validate the index input, it can be any negative value. Or it can be positive but out of range. #Resolved

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of adding a check for each of those cases, I thought we could just state that we are ignoring whatever user has provided because we found the filter by name.


In reply to: 332267723 [](ancestors = 332267723)

updated_key_value = __update_existing_key_value(
azconfig_client, retrieved_kv=retrieved_kv, updated_value=json.dumps(
feature_flag_value.__dict__, default=lambda o: o.__dict__))

Copy link

@shenmuxiaosen shenmuxiaosen Oct 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why we need to customize the output format here? default=lambda o: o.dict #Resolved

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FeatureFlagValue object contains FeatureFilter objects, which are not JSON serializable by default. So we need to dump the dict of all values inside FeatureFlagValue, instead of dumping the value directly.


In reply to: 332272154 [](ancestors = 332272154)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see.


In reply to: 332275434 [](ancestors = 332275434,332272154)

Copy link

@shenmuxiaosen shenmuxiaosen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@avanigupta avanigupta merged commit f2024c6 into dev Oct 8, 2019
@avanigupta avanigupta deleted the avanigupta/cli-feature-management branch October 8, 2019 23:52
avanigupta added a commit that referenced this pull request Nov 19, 2019
* FeatureManagement CLI Part 1 (#1)

* 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

* Final changes for feature management CLI (#2)

* 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

* Fix styling issues

* String formatting for python 2.7 compatibility

* Python 2.7 compatibility for unit tests

* fix id attribute for feature flag value

* Modify list feature default label behavior

* Check content type for features

* CLI team's suggested changes

* Update test recordings

* Changing commands to custom_commands

* changed empty label behavior for delete feature

* Change warning message

* Removing label from FeatureFlagValue

* Use shell_safe_json_parse for deserialiization
avanigupta added a commit that referenced this pull request Dec 17, 2019
* FeatureManagement CLI Part 1 (#1)

* 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

* Final changes for feature management CLI (#2)

* 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

* Fix styling issues

* String formatting for python 2.7 compatibility

* Python 2.7 compatibility for unit tests

* fix id attribute for feature flag value

* Modify list feature default label behavior

* Check content type for features

* CLI team's suggested changes

* Update test recordings

* Changing commands to custom_commands

* Stop reading dest file contents during export

* History file update

* Merge with latest code

* changed empty label behavior for delete feature

* Change warning message

* Removing label from FeatureFlagValue

* Use shell_safe_json_parse for deserialiization

* Updating export for feature flags

* Feature export scenario updates

* Import feature flags from json format

* Adding test

* Fix styling

* Fix Styling

* UPdate history file

* Add sample commands

* improving help messages

* Split print preview of features and kv

* Support alternate json format for ON/OFF features

* Adding more tests

* fix styling

* Use knack logger and enum arg type

* refactoring

* disabling ensure_ascii for json dumps

* Resolving comments

* Refactoring export command

* Resolvong CLI side comments

* Resolving comments

* Update test recordings

* Rename default_value variable

* fix history file

* change separator for history file
avanigupta pushed a commit that referenced this pull request Jun 18, 2021
…18098)

* IoT Hub track 2 updates and managed identity implementation (#2)

* Updates to use track 2 Hub GA SDK

* Added user-assigned identity functionality

* Added routing endpoint identity

* Add DeviceConnectionStateEvents as a routing source type

* RoutingSource test updates

* SDK version update to 2.0.0

Co-authored-by: Ryan Kelly <[email protected]>

* Test recording updates with 2.0.0 SDK

* Fixes for hyrid profile commands / tests and iot_security tests

* String fixes from code review

Co-authored-by: Xing Zhou <[email protected]>

* Updated CLIErrors for missing params to use RequiredArgumentMissingError

* Fix CLIError -> ArgumentUsageError

* PR feedback and test updates

* New format for managed identity parameters

Updated params and help

Test recording updates

* Fix wrong parameter in error description

* Removed identity update command

Added support for 
emove --user-assigned to remove all user-assigned identities

Co-authored-by: Ryan Kelly <[email protected]>
Co-authored-by: Xing Zhou <[email protected]>
avanigupta pushed a commit that referenced this pull request Aug 5, 2021
…file (Azure#18888)

* Add support for bicep files in TS

* Add tests

* re-record test

* Changed new test to a live test

* Changed new test to a live test #2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants