Adding ability to scale for Redis Cache, adding tests by alfantp · Pull Request #2821 · Azure/azure-cli

alfantp · 2017-04-10T23:19:29Z

This checklist is used to make sure that common guidelines for a pull request are followed.

General Guidelines

The PR has modified HISTORY.rst with an appropriate description of the change (see Modifying change log).

Command Guidelines

Each command and parameter has a meaningful description.
Each new command has a test.

msftclas · 2017-04-10T23:19:33Z

@alfantp,
Thanks for your contribution.
To ensure that the project team has proper rights to use your work, please complete the Contribution License Agreement at https://cla.microsoft.com.

It will cover your contributions to all Microsoft-managed open source projects.
Thanks,
Microsoft Pull Request Bot

codecov-io · 2017-04-11T18:24:58Z

Codecov Report

Merging #2821 into master will increase coverage by 0.27%.
The diff coverage is 56.66%.

@@            Coverage Diff             @@
##           master    #2821      +/-   ##
==========================================
+ Coverage   62.89%   63.17%   +0.27%     
==========================================
  Files         480      466      -14     
  Lines       26012    26139     +127     
  Branches     3946     3997      +51     
==========================================
+ Hits        16361    16514     +153     
+ Misses       8627     8544      -83     
- Partials     1024     1081      +57

Impacted Files	Coverage Δ
...-redis/azure/cli/command_modules/redis/commands.py	`100% <100%> (ø)`	⬆️
...i-redis/azure/cli/command_modules/redis/_params.py	`77.77% <100%> (-0.61%)`	⬇️
...cli-redis/azure/cli/command_modules/redis/_help.py	`100% <100%> (ø)`	⬆️
...li-redis/azure/cli/command_modules/redis/custom.py	`40% <40.9%> (+6.66%)`	⬆️
src/azure-cli-core/azure/cli/core/_debug.py	`66.66% <0%> (-20.84%)`	⬇️
...ure-cli-sql/azure/cli/command_modules/sql/_util.py	`84.21% <0%> (-8.65%)`	⬇️
...-cli-find/azure/cli/command_modules/find/custom.py	`88% <0%> (-5.45%)`	⬇️
...h/azure/cli/command_modules/batch/_command_type.py	`90.04% <0%> (-4.36%)`	⬇️
...-cli-iot/azure/cli/command_modules/iot/commands.py	`96.07% <0%> (-3.93%)`	⬇️
...torage/azure/cli/command_modules/storage/custom.py	`83.47% <0%> (-3.54%)`	⬇️
... and 69 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update de6fe79...73dec9d. Read the comment docs.

tjprescott

The addition of the SKU parameters is fine, but then "redis_configuration" should be removed and this should be a "generic update" command (with --set, --add, --remove). Also, it seems the command should be named "redis update" instead of "update-settings".

tjprescott · 2017-04-11T22:48:18Z

src/command_modules/azure-cli-redis/azure/cli/command_modules/redis/custom.py

-def cli_redis_update_settings(client, resource_group_name, name, redis_configuration):
+def cli_redis_update_settings(client, resource_group_name, name, redis_configuration=None, # pylint:disable=too-many-arguments
+                              sku_name=None, sku_family=None, sku_capacity=None):
+    if sku_name != None or sku_family != None or sku_capacity != None:


Recommendation:

sku_group = [sku_name, sku_family, sku_capacity] if any(sku_group) and not all(sku_group): raise CLIError('incorrect usage: --sku-name NAME --sku-family FAMILY --sku-capactity CAPACITY')

The "usage string" style error is favored.

Thanks for the recommendation. Will update it. About renaming the command name, wouldnt changing the name break existing customers who use this for automations? Or since the product is in its early stages it's okay to do that? I'll be very willing to change the command if you can confirm its okay

It would. Also, looking into it, apparently the "redis-configuration" parameter only lets you change a very specific subset of things that aren't clearly enumerated.

In that case, my recommendation is:

create a new command called "redis update" that uses the "cli_generic_update" pattern that other resource types use.

Move the SKU related parameters as the convenience layer on that command, and leave "redis update-settings" as is.

In the future, we can deprecate "update-settings" (as the generic update would accomplish everything it does and more).

tjprescott · 2017-04-11T22:49:41Z

src/command_modules/azure-cli-redis/azure/cli/command_modules/redis/custom.py

+        existing.redis_configuration.update(redis_configuration)
+
+    if sku_name != None and sku_family != None and sku_capacity != None:
+        existing.sku = Sku(sku_name, sku_family, sku_capacity)


Why are we exposing a "configuration" object AND convenience parameters? This looks like it should be a generic update command and "redis_configuration" should be removed.

There are two ways to update a cache. One, you can update the Sku/VMSize to scale. Second, you can update the settings/properties within cache like say max memory threshold or backup interval etc. So IMO, it's fine to have both. Also, this is how the command is in powershell.

…Adding 'getting deprecated' message for update-settings command

tjprescott

Looks much better. A few suggestions. Main ask is to replace the hard-coded enum with more flexible parsing and let the service handle that validation. Feel free to reach out to me with questions.

tjprescott · 2017-04-14T18:10:41Z

src/command_modules/azure-cli-redis/HISTORY.rst

 ===============

+* Adding update command which also adds the ability to scale for redis cache
+* Adding 'getting deprecated' message for update-settings command


Rec: Deprecates the 'update-settings' command.

tjprescott · 2017-04-14T18:12:46Z

src/command_modules/azure-cli-redis/azure/cli/command_modules/redis/_help.py

    type: command
    short-summary: Update the settings of a redis cache.
+    long-summary: |
+        WARNING: This command is being deprecated. Please use 'update' command


Recommend you add (DEPRECATED) to the short summary so it shows up in az redis -h. I like that the long summary directs the user to the right command.

tjprescott · 2017-04-14T18:28:52Z

src/command_modules/azure-cli-redis/azure/cli/command_modules/redis/_params.py

        dictval = shell_safe_json_parse(value)
        self.update(dictval)

+class VmSize(Enum): # pylint: disable=too-few-public-methods


This is definitely one way of doing this but then it has the problem that you have to manually add things to this enum if the service supports something new.

I would recommend something like this:

family = size[0] capacity = int(size[1:])

And then in the help for the param, you give examples of the format. "C1, P2, etc." In this way, if the service starts to support additional combinations, then the CLI will automatically support them instead of artificially restricting them.

tjprescott · 2017-04-14T18:35:07Z

src/command_modules/azure-cli-redis/azure/cli/command_modules/redis/_params.py

+register_cli_argument('redis', 'vm_size', **enum_choice_list(VmSize))
+register_cli_argument('redis', 'enable_non_ssl_port', action='store_true')
+register_cli_argument('redis', 'shard_count', type=int)
+register_cli_argument('redis', 'subnet_id') # TODO: Create generic id completer similar to name


Please create an issue for this follow-up work.

tjprescott · 2017-04-14T18:48:34Z

src/command_modules/azure-cli-redis/azure/cli/command_modules/redis/custom.py

-    :param sku_name: What type of redis cache to deploy. Valid values: (Basic, Standard, Premium).
-    :param sku_family: Which family to use. Valid values: (C, P).
-    :param sku_capacity: What size of redis cache to deploy. Valid values for C family (0, 1, 2, 3, 4, 5, 6), for P family (1, 2, 3, 4)
+    :param sku: What type of redis cache to deploy. Valid values: (Basic, Standard, Premium).


If you are using the **enum_choice_list, it will automatically list (and validate) the options, so you don't need this. If you run this command with -h you'll see the list essentially repeated twice.

tjprescott · 2017-04-14T18:49:09Z

src/command_modules/azure-cli-redis/azure/cli/command_modules/redis/custom.py

-    :param sku_family: Which family to use. Valid values: (C, P).
-    :param sku_capacity: What size of redis cache to deploy. Valid values for C family (0, 1, 2, 3, 4, 5, 6), for P family (1, 2, 3, 4)
+    :param sku: What type of redis cache to deploy. Valid values: (Basic, Standard, Premium).
+    :param vm_size: What size of redis cache to deploy. Valid values for C family (C0, C1, C2, C3, C4, C5, C6), for P family (P1, P2, P3, P4)


And here, if you do like I suggest and parse the size, you will still need this so this is good.

tjprescott · 2017-04-14T18:50:23Z

src/command_modules/azure-cli-redis/azure/cli/command_modules/redis/custom.py

    params = RedisCreateOrUpdateParameters(
        location,
-        Sku(sku_name, sku_family, sku_capacity),
+        Sku(sku, vm_size[0], vm_size[1]),


I would recommend vm_size[1:] in case you ever support double digit capacity values.

troydai

Please run check_style --ci before push.

troydai · 2017-04-14T18:45:45Z

src/command_modules/azure-cli-redis/azure/cli/command_modules/redis/_params.py

+    P1 = 'P1'
+    P2 = 'P2'
+    P3 = 'P3'
+    P4 = 'P4'


Are these sizes defined in the SDK? Do we have to hard code the options in CLI? Could this list be impacted by other factors?

troydai · 2017-04-14T18:46:28Z

src/command_modules/azure-cli-redis/azure/cli/command_modules/redis/_params.py

+register_cli_argument('redis', 'vm_size', **enum_choice_list(VmSize))
+register_cli_argument('redis', 'enable_non_ssl_port', action='store_true')
+register_cli_argument('redis', 'shard_count', type=int)
+register_cli_argument('redis', 'subnet_id') # TODO: Create generic id completer similar to name


I'm sure pylint will reject this line. Two space between same line comments.

Actually, pylint doesn't care. It's PEP8 that hates this... ... ... for some reason.

troydai · 2017-04-14T21:31:31Z

src/command_modules/azure-cli-redis/azure/cli/command_modules/redis/commands.py

 cli_command(__name__, 'redis show', 'azure.mgmt.redis.operations.redis_operations#RedisOperations.get', cf_redis)
 cli_command(__name__, 'redis update-settings', 'azure.cli.command_modules.redis.custom#cli_redis_update_settings', cf_redis)

+cli_generic_update_command(__name__, 'redis update', 'azure.mgmt.redis.operations.redis_operations#RedisOperations.get', 'azure.mgmt.redis.operations.redis_operations#RedisOperations.create_or_update', cf_redis, custom_function_op='azure.cli.command_modules.redis.custom#cli_redis_update')


Please break this line.

troydai · 2017-04-14T21:32:03Z

src/command_modules/azure-cli-redis/tests/test_redis_params.py

        args = mock_echo_args('redis create',
-                              '--tenant-settings {\"hello\":1} -g wombat -n asldkj --sku-family c -l westus  --sku-capacity 1  --sku-name basic') # pylint: disable=line-too-long
+                              '--tenant-settings {\"hello\":1} -g wombat -n asldkj -l westus --sku basic --vm-size C1 ') # pylint: disable=line-too-long
        subset = set(dict(


this is short enough now to be put on the same line.

tjprescott · 2017-04-17T16:43:58Z

@troydai are you good with the changes? If so I'd like to merge this for the release.

alfantp added 2 commits April 10, 2017 15:42

adding ability to scale for redis cache

23c7bef

adding tests for redis cache, adding change in history.rst

0517121

msftclas added the cla-required label Apr 10, 2017

alfantp added 2 commits April 10, 2017 17:10

fixing test failures

e9e91e7

fixing build errors

3fea829

tjprescott suggested changes Apr 11, 2017

View reviewed changes

derekbekoe added cla-not-required and removed cla-required labels Apr 12, 2017

alfantp added 2 commits April 13, 2017 10:41

removing sku-capacity and sku-family and adding vm-size as variable. …

34d64e8

…Adding 'getting deprecated' message for update-settings command

fixing build

ff25e14

tjprescott suggested changes Apr 14, 2017

View reviewed changes

troydai suggested changes Apr 14, 2017

View reviewed changes

tjprescott mentioned this pull request Apr 14, 2017

Generic update doesn't support exception_handler kwarg #2864

Closed

2 tasks

incorporating CR comments

73dec9d

tjprescott approved these changes Apr 17, 2017

View reviewed changes

troydai approved these changes Apr 17, 2017

View reviewed changes

tjprescott merged commit 3f4409b into Azure:master Apr 17, 2017

haroldrandom added the cla-not-required label Oct 25, 2019

Conversation

alfantp commented Apr 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

General Guidelines

Command Guidelines

Uh oh!

msftclas commented Apr 10, 2017

Uh oh!

codecov-io commented Apr 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

tjprescott left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tjprescott Apr 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tjprescott left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tjprescott Apr 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

troydai left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tjprescott commented Apr 17, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

alfantp commented Apr 10, 2017 •

edited

Loading

codecov-io commented Apr 11, 2017 •

edited

Loading

tjprescott Apr 11, 2017 •

edited

Loading

tjprescott Apr 14, 2017 •

edited

Loading