From f9cd777e026578d174941dac966a76ac43e09b43 Mon Sep 17 00:00:00 2001 From: Jared Moore Date: Thu, 9 Jan 2020 16:56:54 -0800 Subject: [PATCH 01/16] Restructure db/dw create/update params Just adding new structure, not moving any code yet --- .../azure/cli/command_modules/sql/_params.py | 62 ++++++++++++++----- 1 file changed, 47 insertions(+), 15 deletions(-) diff --git a/src/azure-cli/azure/cli/command_modules/sql/_params.py b/src/azure-cli/azure/cli/command_modules/sql/_params.py index 5d8778d1183..8eb8365275c 100644 --- a/src/azure-cli/azure/cli/command_modules/sql/_params.py +++ b/src/azure-cli/azure/cli/command_modules/sql/_params.py @@ -245,32 +245,61 @@ def get_location_type_with_default_from_resource_group(cli_ctx): # sql db # ############################################### +# Q. Where should db/dw params be configured? +# A. See this table. +# +# | Param applies to | Param should be configured at: | +# | | | +# | DB create | DB update | DW create | DW update | | +# | (or other | | (or other | | | +# | create mode) | | create mode) | | | +# |--------------|-----------|--------------|-----------|-----------------------------------------| +# | True | | | | argument_context('sql db ') | +# | | True | | | argument_context('sql db update') | +# | | | True | | argument_context('sql dw ') | +# | | | | True | argument_context('sql dw update') | +# | True | | True | | _configure_db_create_params() | +# | True | True | True | True | _configure_db_create_update_params() | +# | ----------------------------------------------------|-----------------------------------------| +# | *Any other combination* | _configure_db_create_update_params() | +# | | Then .ignore() when not applicable | +# +# Q. Why is it like this? +# A. The PUT database REST API has many parameters and many modes (`create_mode`) that control +# which parameters are valid. To make it easier for CLI users to get the param combinations +# correct, these create modes are separated into different commands (e.g.: create, copy, +# restore, etc). +# +# On top of that, some create modes and some params are not allowed if the database edition is +# DataWarehouse. For this reason, regular database commands are separated from datawarehouse +# commands (`db` vs `dw`.) +# +# As a result, the param combination matrix is a little complicated. When adding a new param, +# we want to make sure that the param is visible for the appropriate commands. We also want to +# avoid duplication. class Engine(Enum): # pylint: disable=too-few-public-methods """SQL RDBMS engine type.""" db = 'db' dw = 'dw' +def _configure_db_dw_create_update_params(arg_ctx): + """ + Configures params for db/dw create and update commands. + + Some of these params might not apply to all create modes (e.g. create, restore, copy, etc) + and might not apply to both engine types (DB and DW). That's ok because for create commands + _configure_db_create_params() can .ignore() the params that aren't applicable, and for update commands + the custom update function can just avoid declaring that parameter. + """ + pass -def _configure_db_create_params( +def _configure_db_dw_create_params( arg_ctx, engine, create_mode): """ - Configures params for db/dw create/update commands. - - The PUT database REST API has many parameters and many modes (`create_mode`) that control - which parameters are valid. To make it easier for CLI users to get the param combinations - correct, these create modes are separated into different commands (e.g.: create, copy, - restore, etc). - - On top of that, some create modes and some params are not allowed if the database edition is - DataWarehouse. For this reason, regular database commands are separated from datawarehouse - commands (`db` vs `dw`.) - - As a result, the param combination matrix is a little complicated. This function configures - which params are ignored for a PUT database command based on a command's SQL engine type and - create mode. + Configures params for db/dw create commands. engine: Engine enum value (e.g. `db`, `dw`) create_mode: Valid CreateMode enum value (e.g. `default`, `copy`, etc) @@ -314,6 +343,9 @@ def _configure_db_create_params( 'tier', ]) + # Now that all args are created, do the configuration that applies to both create and update commands. + _configure_db_dw_create_update_params(arg_ctx) + arg_ctx.argument('name', # Note: this is sku name, not database name options_list=['--service-objective'], arg_group=sku_arg_group, From e45ef6f4dd3040f1d65a927a3bfc8105b6762269 Mon Sep 17 00:00:00 2001 From: Jared Moore Date: Thu, 9 Jan 2020 17:01:02 -0800 Subject: [PATCH 02/16] Dedupe compute_model, auto_pause_delay, min_capacity --- .../azure/cli/command_modules/sql/_params.py | 28 ++++++------------- 1 file changed, 9 insertions(+), 19 deletions(-) diff --git a/src/azure-cli/azure/cli/command_modules/sql/_params.py b/src/azure-cli/azure/cli/command_modules/sql/_params.py index 8eb8365275c..952f1e3df70 100644 --- a/src/azure-cli/azure/cli/command_modules/sql/_params.py +++ b/src/azure-cli/azure/cli/command_modules/sql/_params.py @@ -292,7 +292,15 @@ def _configure_db_dw_create_update_params(arg_ctx): _configure_db_create_params() can .ignore() the params that aren't applicable, and for update commands the custom update function can just avoid declaring that parameter. """ - pass + + arg_ctx.argument('compute_model', + arg_type=compute_model_param_type) + + arg_ctx.argument('auto_pause_delay', + arg_type=auto_pause_delay_param_type) + + arg_ctx.argument('min_capacity', + arg_type=min_capacity_param_type) def _configure_db_dw_create_params( arg_ctx, @@ -357,15 +365,6 @@ def _configure_db_dw_create_params( arg_type=elastic_pool_id_param_type, help='The name or resource id of the elastic pool to create the database in.') - arg_ctx.argument('compute_model', - arg_type=compute_model_param_type) - - arg_ctx.argument('auto_pause_delay', - arg_type=auto_pause_delay_param_type) - - arg_ctx.argument('min_capacity', - arg_type=min_capacity_param_type) - arg_ctx.argument('read_scale', arg_type=read_scale_param_type) @@ -579,15 +578,6 @@ def load_arguments(self, _): c.argument('max_size_bytes', help='The new maximum size of the database expressed in bytes.') - c.argument('compute_model', - arg_type=compute_model_param_type) - - c.argument('auto_pause_delay', - arg_type=auto_pause_delay_param_type) - - c.argument('min_capacity', - arg_type=min_capacity_param_type) - with self.argument_context('sql db export') as c: # Create args that will be used to build up the ExportRequest object create_args_for_complex_type( From d6a2febc3bf2707ea2127e0bdf2af68780766293 Mon Sep 17 00:00:00 2001 From: Jared Moore Date: Thu, 9 Jan 2020 17:02:57 -0800 Subject: [PATCH 03/16] Dedupe read_scale and read_replica_count --- .../azure/cli/command_modules/sql/_params.py | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/src/azure-cli/azure/cli/command_modules/sql/_params.py b/src/azure-cli/azure/cli/command_modules/sql/_params.py index 952f1e3df70..4b44063a63f 100644 --- a/src/azure-cli/azure/cli/command_modules/sql/_params.py +++ b/src/azure-cli/azure/cli/command_modules/sql/_params.py @@ -302,6 +302,12 @@ def _configure_db_dw_create_update_params(arg_ctx): arg_ctx.argument('min_capacity', arg_type=min_capacity_param_type) + arg_ctx.argument('read_scale', + arg_type=read_scale_param_type) + + arg_ctx.argument('read_replica_count', + arg_type=read_replicas_param_type) + def _configure_db_dw_create_params( arg_ctx, engine, @@ -365,12 +371,6 @@ def _configure_db_dw_create_params( arg_type=elastic_pool_id_param_type, help='The name or resource id of the elastic pool to create the database in.') - arg_ctx.argument('read_scale', - arg_type=read_scale_param_type) - - arg_ctx.argument('read_replicas', - arg_type=read_replicas_param_type) - # Only applicable to default create mode. Also only applicable to db. if create_mode != CreateMode.default or engine != Engine.db: arg_ctx.ignore('sample_name') @@ -455,12 +455,6 @@ def load_arguments(self, _): c.argument('license_type', arg_type=get_enum_type(DatabaseLicenseType)) - c.argument('read_scale', - arg_type=read_scale_param_type) - - c.argument('read_replica_count', - arg_type=read_replicas_param_type) - c.argument('zone_redundant', arg_type=zone_redundant_param_type) From 2df659730f4a4cea04a6dc26ea08302f707d041d Mon Sep 17 00:00:00 2001 From: Jared Moore Date: Thu, 9 Jan 2020 18:20:06 -0800 Subject: [PATCH 04/16] Dedupe elastic_pool_id --- src/azure-cli/azure/cli/command_modules/sql/_params.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/azure-cli/azure/cli/command_modules/sql/_params.py b/src/azure-cli/azure/cli/command_modules/sql/_params.py index 4b44063a63f..6ab4c7b3299 100644 --- a/src/azure-cli/azure/cli/command_modules/sql/_params.py +++ b/src/azure-cli/azure/cli/command_modules/sql/_params.py @@ -293,6 +293,9 @@ def _configure_db_dw_create_update_params(arg_ctx): the custom update function can just avoid declaring that parameter. """ + arg_ctx.argument('elastic_pool_id', + arg_type=elastic_pool_id_param_type) + arg_ctx.argument('compute_model', arg_type=compute_model_param_type) @@ -368,7 +371,6 @@ def _configure_db_dw_create_params( (db_service_objective_examples if engine == Engine.db else dw_service_objective_examples)) arg_ctx.argument('elastic_pool_id', - arg_type=elastic_pool_id_param_type, help='The name or resource id of the elastic pool to create the database in.') # Only applicable to default create mode. Also only applicable to db. @@ -567,7 +569,6 @@ def load_arguments(self, _): ' the pool.') c.argument('elastic_pool_id', - arg_type=elastic_pool_id_param_type, help='The name or resource id of the elastic pool to move the database into.') c.argument('max_size_bytes', help='The new maximum size of the database expressed in bytes.') From 4295d58e7ea4ab933f1fdd4bd8d2e86285c7f56b Mon Sep 17 00:00:00 2001 From: Jared Moore Date: Thu, 9 Jan 2020 18:22:16 -0800 Subject: [PATCH 05/16] Dedupe max_size_bytes --- src/azure-cli/azure/cli/command_modules/sql/_params.py | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/src/azure-cli/azure/cli/command_modules/sql/_params.py b/src/azure-cli/azure/cli/command_modules/sql/_params.py index 6ab4c7b3299..6a977f40e6d 100644 --- a/src/azure-cli/azure/cli/command_modules/sql/_params.py +++ b/src/azure-cli/azure/cli/command_modules/sql/_params.py @@ -293,6 +293,9 @@ def _configure_db_dw_create_update_params(arg_ctx): the custom update function can just avoid declaring that parameter. """ + arg_ctx.argument('max_size_bytes', + arg_type=max_size_bytes_param_type) + arg_ctx.argument('elastic_pool_id', arg_type=elastic_pool_id_param_type) @@ -438,9 +441,6 @@ def load_arguments(self, _): # Allow --ids command line argument. id_part=child_name_1 is 2nd name in uri id_part='child_name_1') - c.argument('max_size_bytes', - arg_type=max_size_bytes_param_type) - creation_arg_group = 'Creation' c.argument('collation', @@ -796,9 +796,6 @@ def _configure_security_policy_storage_params(arg_ctx): # Allow --ids command line argument. id_part=child_name_1 is 2nd name in uri id_part='child_name_1') - c.argument('max_size_bytes', - arg_type=max_size_bytes_param_type) - c.argument('service_objective', help='The service objective of the data warehouse. For example: ' + dw_service_objective_examples) From e087b752963228c0fdc29eb0768a78b606c30bf2 Mon Sep 17 00:00:00 2001 From: Jared Moore Date: Thu, 9 Jan 2020 18:23:00 -0800 Subject: [PATCH 06/16] Indentation --- src/azure-cli/azure/cli/command_modules/sql/_params.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/azure-cli/azure/cli/command_modules/sql/_params.py b/src/azure-cli/azure/cli/command_modules/sql/_params.py index 6a977f40e6d..e81455b720f 100644 --- a/src/azure-cli/azure/cli/command_modules/sql/_params.py +++ b/src/azure-cli/azure/cli/command_modules/sql/_params.py @@ -294,7 +294,7 @@ def _configure_db_dw_create_update_params(arg_ctx): """ arg_ctx.argument('max_size_bytes', - arg_type=max_size_bytes_param_type) + arg_type=max_size_bytes_param_type) arg_ctx.argument('elastic_pool_id', arg_type=elastic_pool_id_param_type) @@ -309,10 +309,10 @@ def _configure_db_dw_create_update_params(arg_ctx): arg_type=min_capacity_param_type) arg_ctx.argument('read_scale', - arg_type=read_scale_param_type) + arg_type=read_scale_param_type) arg_ctx.argument('read_replica_count', - arg_type=read_replicas_param_type) + arg_type=read_replicas_param_type) def _configure_db_dw_create_params( arg_ctx, From 5b99c9ae509b02247c48d9026cf6e32c00b3cd91 Mon Sep 17 00:00:00 2001 From: Jared Moore Date: Thu, 9 Jan 2020 18:32:19 -0800 Subject: [PATCH 07/16] Function naming --- .../azure/cli/command_modules/sql/_params.py | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/azure-cli/azure/cli/command_modules/sql/_params.py b/src/azure-cli/azure/cli/command_modules/sql/_params.py index e81455b720f..ec6b13f0b83 100644 --- a/src/azure-cli/azure/cli/command_modules/sql/_params.py +++ b/src/azure-cli/azure/cli/command_modules/sql/_params.py @@ -258,10 +258,10 @@ def get_location_type_with_default_from_resource_group(cli_ctx): # | | True | | | argument_context('sql db update') | # | | | True | | argument_context('sql dw ') | # | | | | True | argument_context('sql dw update') | -# | True | | True | | _configure_db_create_params() | -# | True | True | True | True | _configure_db_create_update_params() | +# | True | | True | | _configure_db_dw_create_params() | +# | True | True | True | True | _configure_db_dw_create_update_params() | # | ----------------------------------------------------|-----------------------------------------| -# | *Any other combination* | _configure_db_create_update_params() | +# | *Any other combination* | _configure_db_dw_create_update_params() | # | | Then .ignore() when not applicable | # # Q. Why is it like this? @@ -289,7 +289,7 @@ def _configure_db_dw_create_update_params(arg_ctx): Some of these params might not apply to all create modes (e.g. create, restore, copy, etc) and might not apply to both engine types (DB and DW). That's ok because for create commands - _configure_db_create_params() can .ignore() the params that aren't applicable, and for update commands + _configure_db_dw_create_params() can .ignore() the params that aren't applicable, and for update commands the custom update function can just avoid declaring that parameter. """ @@ -476,10 +476,10 @@ def load_arguments(self, _): 'Allowed values include: Gen4, Gen5.') with self.argument_context('sql db create') as c: - _configure_db_create_params(c, Engine.db, CreateMode.default) + _configure_db_dw_create_params(c, Engine.db, CreateMode.default) with self.argument_context('sql db copy') as c: - _configure_db_create_params(c, Engine.db, CreateMode.copy) + _configure_db_dw_create_params(c, Engine.db, CreateMode.copy) c.argument('dest_name', help='Name of the database that will be created as the copy destination.') @@ -499,7 +499,7 @@ def load_arguments(self, _): help='The new name that the database will be renamed to.') with self.argument_context('sql db restore') as c: - _configure_db_create_params(c, Engine.db, CreateMode.point_in_time_restore) + _configure_db_dw_create_params(c, Engine.db, CreateMode.point_in_time_restore) c.argument('dest_name', help='Name of the database that will be created as the restore destination.') @@ -659,7 +659,7 @@ def load_arguments(self, _): # sql db replica ##### with self.argument_context('sql db replica create') as c: - _configure_db_create_params(c, Engine.db, CreateMode.secondary) + _configure_db_dw_create_params(c, Engine.db, CreateMode.secondary) c.argument('partner_resource_group_name', options_list=['--partner-resource-group'], @@ -804,7 +804,7 @@ def _configure_security_policy_storage_params(arg_ctx): help='The collation of the data warehouse.') with self.argument_context('sql dw create') as c: - _configure_db_create_params(c, Engine.dw, CreateMode.default) + _configure_db_dw_create_params(c, Engine.dw, CreateMode.default) with self.argument_context('sql dw show') as c: # Service tier advisors and transparent data encryption are not included in the first batch From f3187a31f09874c88d7f65a31ad93d82a5048a00 Mon Sep 17 00:00:00 2001 From: Jared Moore Date: Thu, 9 Jan 2020 18:33:36 -0800 Subject: [PATCH 08/16] Formatting --- src/azure-cli/azure/cli/command_modules/sql/_params.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/azure-cli/azure/cli/command_modules/sql/_params.py b/src/azure-cli/azure/cli/command_modules/sql/_params.py index ec6b13f0b83..8784f1fecb8 100644 --- a/src/azure-cli/azure/cli/command_modules/sql/_params.py +++ b/src/azure-cli/azure/cli/command_modules/sql/_params.py @@ -258,10 +258,9 @@ def get_location_type_with_default_from_resource_group(cli_ctx): # | | True | | | argument_context('sql db update') | # | | | True | | argument_context('sql dw ') | # | | | | True | argument_context('sql dw update') | -# | True | | True | | _configure_db_dw_create_params() | -# | True | True | True | True | _configure_db_dw_create_update_params() | +# | True | | True | | _configure_db_dw_create_params() | # | ----------------------------------------------------|-----------------------------------------| -# | *Any other combination* | _configure_db_dw_create_update_params() | +# | *Any other combination* | _configure_db_dw_create_update_params() | # | | Then .ignore() when not applicable | # # Q. Why is it like this? From 92d52c2ac62f9becb6fd3746ce05fbbb9db694b2 Mon Sep 17 00:00:00 2001 From: Jared Moore Date: Mon, 13 Jan 2020 12:07:55 -0800 Subject: [PATCH 09/16] More refactoring --- .../azure/cli/command_modules/sql/_params.py | 112 +++++++++--------- 1 file changed, 55 insertions(+), 57 deletions(-) diff --git a/src/azure-cli/azure/cli/command_modules/sql/_params.py b/src/azure-cli/azure/cli/command_modules/sql/_params.py index 8784f1fecb8..cf036661c3f 100644 --- a/src/azure-cli/azure/cli/command_modules/sql/_params.py +++ b/src/azure-cli/azure/cli/command_modules/sql/_params.py @@ -245,51 +245,20 @@ def get_location_type_with_default_from_resource_group(cli_ctx): # sql db # ############################################### -# Q. Where should db/dw params be configured? -# A. See this table. -# -# | Param applies to | Param should be configured at: | -# | | | -# | DB create | DB update | DW create | DW update | | -# | (or other | | (or other | | | -# | create mode) | | create mode) | | | -# |--------------|-----------|--------------|-----------|-----------------------------------------| -# | True | | | | argument_context('sql db ') | -# | | True | | | argument_context('sql db update') | -# | | | True | | argument_context('sql dw ') | -# | | | | True | argument_context('sql dw update') | -# | True | | True | | _configure_db_dw_create_params() | -# | ----------------------------------------------------|-----------------------------------------| -# | *Any other combination* | _configure_db_dw_create_update_params() | -# | | Then .ignore() when not applicable | -# -# Q. Why is it like this? -# A. The PUT database REST API has many parameters and many modes (`create_mode`) that control -# which parameters are valid. To make it easier for CLI users to get the param combinations -# correct, these create modes are separated into different commands (e.g.: create, copy, -# restore, etc). -# -# On top of that, some create modes and some params are not allowed if the database edition is -# DataWarehouse. For this reason, regular database commands are separated from datawarehouse -# commands (`db` vs `dw`.) -# -# As a result, the param combination matrix is a little complicated. When adding a new param, -# we want to make sure that the param is visible for the appropriate commands. We also want to -# avoid duplication. class Engine(Enum): # pylint: disable=too-few-public-methods """SQL RDBMS engine type.""" db = 'db' dw = 'dw' -def _configure_db_dw_create_update_params(arg_ctx): +def _configure_db_dw_params(arg_ctx): """ - Configures params for db/dw create and update commands. + Configures params that are based on `Database` resource and therefore apply to one or more DB/DW create/update commands. + The idea is that this does some basic configuration of each property. Each command can then potentially + build on top of this (e.g. to give a parameter more specific help text) and .ignore() parameters that aren't applicable. - Some of these params might not apply to all create modes (e.g. create, restore, copy, etc) - and might not apply to both engine types (DB and DW). That's ok because for create commands - _configure_db_dw_create_params() can .ignore() the params that aren't applicable, and for update commands - the custom update function can just avoid declaring that parameter. + Normally these param configurations would be implemented at the command group level, but these params are used across + 2 different param groups - `sql db` and `sql dw`. So extracting it out into this common function prevents duplication. """ arg_ctx.argument('max_size_bytes', @@ -313,6 +282,26 @@ def _configure_db_dw_create_update_params(arg_ctx): arg_ctx.argument('read_replica_count', arg_type=read_replicas_param_type) + creation_arg_group = 'Creation' + + arg_ctx.argument('collation', + arg_group=creation_arg_group) + + arg_ctx.argument('catalog_collation', + arg_group=creation_arg_group, + arg_type=get_enum_type(CatalogCollationType)) + + arg_ctx.argument('sample_name', + arg_group=creation_arg_group, + arg_type=get_enum_type(SampleName)) + + arg_ctx.argument('license_type', + arg_type=get_enum_type(DatabaseLicenseType)) + + arg_ctx.argument('zone_redundant', + arg_type=zone_redundant_param_type) + + def _configure_db_dw_create_params( arg_ctx, engine, @@ -320,10 +309,30 @@ def _configure_db_dw_create_params( """ Configures params for db/dw create commands. + The PUT database REST API has many parameters and many modes (`create_mode`) that control + which parameters are valid. To make it easier for CLI users to get the param combinations + correct, these create modes are separated into different commands (e.g.: create, copy, + restore, etc). + + On top of that, some create modes and some params are not allowed if the database edition is + DataWarehouse. For this reason, regular database commands are separated from datawarehouse + commands (`db` vs `dw`.) + + As a result, the param combination matrix is a little complicated. When adding a new param, + we want to make sure that the param is visible for the appropriate commands. We also want to + avoid duplication. Instead of spreading out & duplicating the param definitions across all + the different commands, it has been more effective to define this reusable function. + + The main task here is to create extra params based on the `Database` model, then .ignore() the params that + aren't applicable to the specified engine and create mode. There is also some minor tweaking of help text + to make the help text more specific to creation. + engine: Engine enum value (e.g. `db`, `dw`) create_mode: Valid CreateMode enum value (e.g. `default`, `copy`, etc) """ + #### Step 0: Validation #### + # DW does not support all create modes. Check that engine and create_mode are consistent. if engine == Engine.dw and create_mode not in [ CreateMode.default, @@ -331,6 +340,8 @@ def _configure_db_dw_create_params( CreateMode.restore]: raise ValueError('Engine {} does not support create mode {}'.format(engine, create_mode)) + #### Step 1: Create extra params #### + # Create args that will be used to build up the Database object create_args_for_complex_type( arg_ctx, 'parameters', Database, [ @@ -362,8 +373,7 @@ def _configure_db_dw_create_params( 'tier', ]) - # Now that all args are created, do the configuration that applies to both create and update commands. - _configure_db_dw_create_update_params(arg_ctx) + #### Step 2: Apply customizations specific to create (as opposed to update) #### arg_ctx.argument('name', # Note: this is sku name, not database name options_list=['--service-objective'], @@ -375,6 +385,8 @@ def _configure_db_dw_create_params( arg_ctx.argument('elastic_pool_id', help='The name or resource id of the elastic pool to create the database in.') + #### Step 3: Ignore params that are not applicable (based on engine & create mode) #### + # Only applicable to default create mode. Also only applicable to db. if create_mode != CreateMode.default or engine != Engine.db: arg_ctx.ignore('sample_name') @@ -440,25 +452,11 @@ def load_arguments(self, _): # Allow --ids command line argument. id_part=child_name_1 is 2nd name in uri id_part='child_name_1') - creation_arg_group = 'Creation' - - c.argument('collation', - arg_group=creation_arg_group) - - c.argument('catalog_collation', - arg_group=creation_arg_group, - arg_type=get_enum_type(CatalogCollationType)) - - c.argument('sample_name', - arg_group=creation_arg_group, - arg_type=get_enum_type(SampleName)) - - c.argument('license_type', - arg_type=get_enum_type(DatabaseLicenseType)) - - c.argument('zone_redundant', - arg_type=zone_redundant_param_type) + _configure_db_dw_params(c) + # SKU-related params are different from DB versus DW, so we want this configuration to apply here + # in 'sql db' group but not in 'sql dw' group. If we wanted to apply to both, we would put the + # configuration into _configure_db_dw_params(). c.argument('tier', arg_type=tier_param_type, help='The edition component of the sku. Allowed values include: Basic, Standard, ' From d8343b010ebe31971eb80a1683c2691d25e8dc10 Mon Sep 17 00:00:00 2001 From: Jared Moore Date: Mon, 13 Jan 2020 12:19:09 -0800 Subject: [PATCH 10/16] Hide --read-replica-count from sql dw --- src/azure-cli/azure/cli/command_modules/sql/_params.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/azure-cli/azure/cli/command_modules/sql/_params.py b/src/azure-cli/azure/cli/command_modules/sql/_params.py index cf036661c3f..9c10221278e 100644 --- a/src/azure-cli/azure/cli/command_modules/sql/_params.py +++ b/src/azure-cli/azure/cli/command_modules/sql/_params.py @@ -429,7 +429,7 @@ def _configure_db_dw_create_params( # ReadScale properties are not valid for DataWarehouse arg_ctx.ignore('read_scale') - arg_ctx.ignore('read_replicas') + arg_ctx.ignore('read_replica_count') # pylint: disable=too-many-statements @@ -443,6 +443,8 @@ def load_arguments(self, _): help='If specified, the failover operation will allow data loss.') with self.argument_context('sql db') as c: + _configure_db_dw_params(c) + c.argument('server_name', arg_type=server_param_type) @@ -452,8 +454,6 @@ def load_arguments(self, _): # Allow --ids command line argument. id_part=child_name_1 is 2nd name in uri id_part='child_name_1') - _configure_db_dw_params(c) - # SKU-related params are different from DB versus DW, so we want this configuration to apply here # in 'sql db' group but not in 'sql dw' group. If we wanted to apply to both, we would put the # configuration into _configure_db_dw_params(). @@ -784,6 +784,8 @@ def _configure_security_policy_storage_params(arg_ctx): # sql dw # ############################################### with self.argument_context('sql dw') as c: + _configure_db_dw_params(c) + c.argument('server_name', arg_type=server_param_type) From 981f8f9bdb405887eb8c6eaba45bca42ca3b87fb Mon Sep 17 00:00:00 2001 From: Jared Moore Date: Mon, 13 Jan 2020 12:33:29 -0800 Subject: [PATCH 11/16] Deprecate sql dw create --read-replicas --- .../azure/cli/command_modules/sql/_params.py | 32 ++++++++++++++++++- 1 file changed, 31 insertions(+), 1 deletion(-) diff --git a/src/azure-cli/azure/cli/command_modules/sql/_params.py b/src/azure-cli/azure/cli/command_modules/sql/_params.py index 9c10221278e..1a026b5def2 100644 --- a/src/azure-cli/azure/cli/command_modules/sql/_params.py +++ b/src/azure-cli/azure/cli/command_modules/sql/_params.py @@ -343,6 +343,35 @@ def _configure_db_dw_create_params( #### Step 1: Create extra params #### # Create args that will be used to build up the Database object + # + # IMPORTANT: It is very easy to add a new parameter and accidentally forget to .ignore() it in + # some commands that it is not applicable to. Therefore, when adding a new param, you should compare + # command help before & after your change. + # e.g.: + # + # # Get initial help text + # git checkout dev + # $file = 'help_original.txt' + # az sql db create -h >> $file + # az sql db copy -h >> $file + # az sql db restore -h >> $file + # az sql db replica create -h >> $file + # az sql db update -h >> $file + # az sql dw create -h >> $file + # az sql dw update -h >> $file + # + # # Get updated help text + # git checkout mybranch + # $file = 'help_updated.txt' + # az sql db create -h >> $file + # az sql db copy -h >> $file + # az sql db restore -h >> $file + # az sql db replica create -h >> $file + # az sql db update -h >> $file + # az sql dw create -h >> $file + # az sql dw update -h >> $file + # + # Then compare 'help_original.txt' <-> 'help_updated.txt' in your favourite text diff tool. create_args_for_complex_type( arg_ctx, 'parameters', Database, [ 'catalog_collation', @@ -429,7 +458,8 @@ def _configure_db_dw_create_params( # ReadScale properties are not valid for DataWarehouse arg_ctx.ignore('read_scale') - arg_ctx.ignore('read_replica_count') + arg_ctx.argument('read_replica_count', + deprecate_info=arg_ctx.deprecate(target='--read-replicas')) # pylint: disable=too-many-statements From a2d300cbce8f47e454cc7d3c5fa733ee77b71eb8 Mon Sep 17 00:00:00 2001 From: Jared Moore Date: Mon, 13 Jan 2020 12:51:17 -0800 Subject: [PATCH 12/16] Deprecate sql dw create --zone-redundant --- .../azure/cli/command_modules/sql/_params.py | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/src/azure-cli/azure/cli/command_modules/sql/_params.py b/src/azure-cli/azure/cli/command_modules/sql/_params.py index 1a026b5def2..aeb973d5058 100644 --- a/src/azure-cli/azure/cli/command_modules/sql/_params.py +++ b/src/azure-cli/azure/cli/command_modules/sql/_params.py @@ -458,8 +458,17 @@ def _configure_db_dw_create_params( # ReadScale properties are not valid for DataWarehouse arg_ctx.ignore('read_scale') + + # Below properties are not valid for DataWarehouse, but were accidentally + # included in previous releases and therefore are deprecated and will be removed + # in the future. arg_ctx.argument('read_replica_count', - deprecate_info=arg_ctx.deprecate(target='--read-replicas')) + options_list=['--read-replica-count'], + deprecate_info=arg_ctx.deprecate(target='--read-replica-count')) + + arg_ctx.argument('zone_redundant', + options_list=['--zone-redundant'], + deprecate_info=arg_ctx.deprecate(target='--zone-redundant')) # pylint: disable=too-many-statements From 132a286a13f670f53c43e4d94842cbb7e5879890 Mon Sep 17 00:00:00 2001 From: Jared Moore Date: Mon, 13 Jan 2020 13:02:23 -0800 Subject: [PATCH 13/16] Add history and hide the parameters --- src/azure-cli/HISTORY.rst | 4 ++++ src/azure-cli/azure/cli/command_modules/sql/_params.py | 4 ++-- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/src/azure-cli/HISTORY.rst b/src/azure-cli/HISTORY.rst index 676d03a15bb..d83892e1370 100644 --- a/src/azure-cli/HISTORY.rst +++ b/src/azure-cli/HISTORY.rst @@ -3,6 +3,10 @@ Release History =============== +**SQL** + +* `sql dw create`: deprecated `--zone-redundant` and `--read-replica-count` parameters. These parameters do not apply to DataWarehouse. + 2.0.80 ++++++ diff --git a/src/azure-cli/azure/cli/command_modules/sql/_params.py b/src/azure-cli/azure/cli/command_modules/sql/_params.py index aeb973d5058..9796d318a2a 100644 --- a/src/azure-cli/azure/cli/command_modules/sql/_params.py +++ b/src/azure-cli/azure/cli/command_modules/sql/_params.py @@ -464,11 +464,11 @@ def _configure_db_dw_create_params( # in the future. arg_ctx.argument('read_replica_count', options_list=['--read-replica-count'], - deprecate_info=arg_ctx.deprecate(target='--read-replica-count')) + deprecate_info=arg_ctx.deprecate(hide=True)) arg_ctx.argument('zone_redundant', options_list=['--zone-redundant'], - deprecate_info=arg_ctx.deprecate(target='--zone-redundant')) + deprecate_info=arg_ctx.deprecate(hide=True)) # pylint: disable=too-many-statements From 2e6104d7eb2fc14daaf6c26f37d521d085512b9c Mon Sep 17 00:00:00 2001 From: Jared Moore Date: Mon, 13 Jan 2020 13:05:01 -0800 Subject: [PATCH 14/16] Tweak comments --- src/azure-cli/azure/cli/command_modules/sql/_params.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/azure-cli/azure/cli/command_modules/sql/_params.py b/src/azure-cli/azure/cli/command_modules/sql/_params.py index 9796d318a2a..11bc950a04f 100644 --- a/src/azure-cli/azure/cli/command_modules/sql/_params.py +++ b/src/azure-cli/azure/cli/command_modules/sql/_params.py @@ -457,15 +457,15 @@ def _configure_db_dw_create_params( arg_ctx.ignore('compute_model') # ReadScale properties are not valid for DataWarehouse + # --read-replica-count was accidentally included in previous releases and + # therefore is hidden using `deprecate_info` instead of `ignore` arg_ctx.ignore('read_scale') - - # Below properties are not valid for DataWarehouse, but were accidentally - # included in previous releases and therefore are deprecated and will be removed - # in the future. arg_ctx.argument('read_replica_count', options_list=['--read-replica-count'], deprecate_info=arg_ctx.deprecate(hide=True)) + # Zone redundant was accidentally included in previous releases and + # therefore is hidden using `deprecate_info` instead of `ignore` arg_ctx.argument('zone_redundant', options_list=['--zone-redundant'], deprecate_info=arg_ctx.deprecate(hide=True)) From a926cb0499d47318ccfa4f21d7098978c2b8f2f6 Mon Sep 17 00:00:00 2001 From: Jared Moore Date: Thu, 16 Jan 2020 13:39:31 -0800 Subject: [PATCH 15/16] Style --- .../azure/cli/command_modules/sql/_params.py | 23 +++++++++++-------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/src/azure-cli/azure/cli/command_modules/sql/_params.py b/src/azure-cli/azure/cli/command_modules/sql/_params.py index 11bc950a04f..1314644798b 100644 --- a/src/azure-cli/azure/cli/command_modules/sql/_params.py +++ b/src/azure-cli/azure/cli/command_modules/sql/_params.py @@ -251,14 +251,17 @@ class Engine(Enum): # pylint: disable=too-few-public-methods db = 'db' dw = 'dw' + def _configure_db_dw_params(arg_ctx): """ - Configures params that are based on `Database` resource and therefore apply to one or more DB/DW create/update commands. - The idea is that this does some basic configuration of each property. Each command can then potentially - build on top of this (e.g. to give a parameter more specific help text) and .ignore() parameters that aren't applicable. - - Normally these param configurations would be implemented at the command group level, but these params are used across - 2 different param groups - `sql db` and `sql dw`. So extracting it out into this common function prevents duplication. + Configures params that are based on `Database` resource and therefore apply to one or more DB/DW create/update + commands. The idea is that this does some basic configuration of each property. Each command can then potentially + build on top of this (e.g. to give a parameter more specific help text) and .ignore() parameters that aren't + applicable. + + Normally these param configurations would be implemented at the command group level, but these params are used + across 2 different param groups - `sql db` and `sql dw`. So extracting it out into this common function prevents + duplication. """ arg_ctx.argument('max_size_bytes', @@ -331,7 +334,7 @@ def _configure_db_dw_create_params( create_mode: Valid CreateMode enum value (e.g. `default`, `copy`, etc) """ - #### Step 0: Validation #### + # *** Step 0: Validation *** # DW does not support all create modes. Check that engine and create_mode are consistent. if engine == Engine.dw and create_mode not in [ @@ -340,7 +343,7 @@ def _configure_db_dw_create_params( CreateMode.restore]: raise ValueError('Engine {} does not support create mode {}'.format(engine, create_mode)) - #### Step 1: Create extra params #### + # *** Step 1: Create extra params *** # Create args that will be used to build up the Database object # @@ -402,7 +405,7 @@ def _configure_db_dw_create_params( 'tier', ]) - #### Step 2: Apply customizations specific to create (as opposed to update) #### + # *** Step 2: Apply customizations specific to create (as opposed to update) *** arg_ctx.argument('name', # Note: this is sku name, not database name options_list=['--service-objective'], @@ -414,7 +417,7 @@ def _configure_db_dw_create_params( arg_ctx.argument('elastic_pool_id', help='The name or resource id of the elastic pool to create the database in.') - #### Step 3: Ignore params that are not applicable (based on engine & create mode) #### + # *** Step 3: Ignore params that are not applicable (based on engine & create mode) *** # Only applicable to default create mode. Also only applicable to db. if create_mode != CreateMode.default or engine != Engine.db: From 28d271f57a1755554a10c32b563c313235914df3 Mon Sep 17 00:00:00 2001 From: Jared Moore Date: Thu, 30 Jan 2020 14:38:10 -0800 Subject: [PATCH 16/16] Fixed style --- src/azure-cli/azure/cli/command_modules/sql/_params.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/azure-cli/azure/cli/command_modules/sql/_params.py b/src/azure-cli/azure/cli/command_modules/sql/_params.py index 2072adef6ef..f108d0ec525 100644 --- a/src/azure-cli/azure/cli/command_modules/sql/_params.py +++ b/src/azure-cli/azure/cli/command_modules/sql/_params.py @@ -298,9 +298,9 @@ def _configure_db_dw_params(arg_ctx): # WideWorldImportersStd and WideWorldImportersFull cannot be successfully created. # AdventureWorksLT is the only sample name that is actually supported. - c.argument('sample_name', - arg_group=creation_arg_group, - arg_type=get_enum_type([SampleName.adventure_works_lt])) + arg_ctx.argument('sample_name', + arg_group=creation_arg_group, + arg_type=get_enum_type([SampleName.adventure_works_lt])) arg_ctx.argument('license_type', arg_type=get_enum_type(DatabaseLicenseType))