[Synapse] Support synapse sql dw cmdlets and update az synapse workspace create cmdlet #16095
Conversation
aim-for-better
force-pushed
the
SupportSynapseSqlDW
branch
from
cea6089 to
9406732
Compare
aim-for-better
force-pushed
the
SupportSynapseSqlDW
branch
2 times, most recently
from
0fcd654 to
d001c3b
Compare
…workspace create cmdlet 2. Add new cmdlets az synapse sql pool classification create, show, update,list, delete 3. Add new cmdlets az synapse sql pool classification recommendation list, enable, disable 4. Add new cmdlets az synapse sql pool restore, list-deleted, show-connection-string 5. Add new cmdlets az synapse sql pool tde set, show 6. Add new cmdlets az synapse sql pool threat-policy show, update 7. Add new cmdlets az synapse sql pool audit-policy show, update
aim-for-better
force-pushed
the
SupportSynapseSqlDW
branch
from
d001c3b to
fa706e3
Compare
|
The "Integration Test against Profiles Python36", "Integration Test against Profiles Python38", "Test Extensions Loading Python36" and "Verify Sphinx, Document Generato" failed for the same error as bellow: |
| return client.update(resource_group_name, workspace_name, sql_pool_name, sql_pool_patch_info) | ||
|
|
||
|
|
||
| def restore_sql_pool(cmd, client, resource_group_name, workspace_name, sql_pool_name, dest_name, performance_level=None, |
There was a problem hiding this comment.
dest_name [](start = 86, length = 9)
destination_name may be better to me #Resolved
There was a problem hiding this comment.
Thanks I have changed it to destination_name and add --dest-name in options list.
In reply to: 533103965 [](ancestors = 533103965)
|
Synapse |
| } | ||
| } | ||
|
|
||
| f = formats[client_provider][auth_type] |
There was a problem hiding this comment.
f [](start = 4, length = 1)
Better to use a more meaningful variable name
There was a problem hiding this comment.
I agree with you but I am afraid that I can not modify this, because the sql db is using this parameter.
We hope that sql dw's customer can have the same or similar experience when they use synapse, so I think we should keep consistent with sql db.
If I modify the parameter name here, then I need to add a option list in parameter to expose the same parameter with sql db. I prefer just keep this. @idear1203 How do you like?
In reply to: 533113490 [](ancestors = 533113490)
| retention_days is not None) | ||
|
|
||
| if not state and not blob_storage_arguments_provided: | ||
| raise CLIError('Either state or blob storage arguments are missing') |
There was a problem hiding this comment.
Do you mean user must provide at least one parameter from below list? Can we use any() like:
if not any([storage_account, storage_endpoint, storage_account_access_key, retention_days, state]):
raise CLIError('Either state or blob storage arguments are missing')
``` #Resolved
There was a problem hiding this comment.
Thanks for reminder, I refactor the blob_storage_arguments_provided by using any.
In reply to: 533138845 [](ancestors = 533138845)
| storage_account_subscription_id = _find_storage_account_subscription_id(cmd.cli_ctx, storage_account) | ||
|
|
||
| if storage_endpoint is not None: | ||
| instance.storage_endpoint = storage_endpoint |
There was a problem hiding this comment.
Also need to add this?
# Validate storage endpoint arguments
if storage_endpoint and storage_account:
raise CLIError('--storage-endpoint and --storage-account cannot both be specified.')
``` #Resolved
There was a problem hiding this comment.
Sql db doesn't add this validation. I want to keep the same with sql db although it will not cause error without this validation.
In reply to: 533155346 [](ancestors = 533155346)
| # url parse package has different names in Python 2 and 3. 'six' package works cross-version. | ||
| from six.moves.urllib.parse import urlparse # pylint: disable=import-error | ||
|
|
||
| return urlparse(storage_endpoint).netloc.split('.')[0] |
There was a problem hiding this comment.
Duplicate definition：
sqlpoolsecurityalertpolicy.py +140
sqlpoolblobauditingpolicy.py +165
_get_storage_account_name, _get_storage_endpoint, _get_storage_key
#Resolved
There was a problem hiding this comment.
fixed, removed the definition in this file and reuse the definition in sqlpoolblobauditingpolicy.py file
In reply to: 533160003 [](ancestors = 533160003)
| table_name, | ||
| column_name, | ||
| "current") # ToDo: need to change to SensitivityLabelSource.current | ||
|
|
There was a problem hiding this comment.
Just out of curiosity, This ToDo label is meaningful? #Resolved
There was a problem hiding this comment.
removed, so sorry for this, this is because there is issue in the old sdk, I use this ToDo as reminder but forget to remove this when the latest sdk is available.
In reply to: 533162734 [](ancestors = 533162734)
| security_center_client = get_mgmt_service_client(cmd.cli_ctx, SecurityCenter, asc_location="centralus") | ||
|
|
||
| information_protection_policy = security_center_client.information_protection_policies.get( | ||
| scope='/providers/Microsoft.Management/managementGroups/{}'.format(_get_tenant_id()), |
There was a problem hiding this comment.
'/providers/Microsoft.Management/managementGroups/{}'.format(_get_tenant_id()), [](start = 14, length = 79)
define a _get_current_tenant_scope() may be better? Realization of _get_tenant_id also can be placed inside. #Resolved
There was a problem hiding this comment.
| schema_name, | ||
| table_name, | ||
| column_name, | ||
| SensitivityLabelSource.current) |
There was a problem hiding this comment.
SensitivityLabelSource.current) [](start = 12, length = 31)
Is different from above mention at ToDo label? #Resolved
There was a problem hiding this comment.
| raise CLIError('The provided label name was not found in the information protection policy.') | ||
| sensitivity_label.label_id = label_id | ||
| sensitivity_label.label_name = label_name | ||
| if information_type: |
There was a problem hiding this comment.
better to add a blank line #Resolved
There was a problem hiding this comment.
| from azure.mgmt.security import SecurityCenter | ||
| from msrestazure.azure_exceptions import CloudError | ||
|
|
||
| security_center_client = get_mgmt_service_client(cmd.cli_ctx, SecurityCenter, asc_location="centralus") |
There was a problem hiding this comment.
asc_location="centralus" [](start = 82, length = 24)
This hard-code location info is required? #Resolved
There was a problem hiding this comment.
copied from sql db, I suggest to keep this, I don't know why sql db hard code asc_location="centralus".
In reply to: 533177465 [](ancestors = 533177465)
| resource_group_name, workspace_name, sql_pool_name, schema_name, table_name, column_name, sensitivity_label) | ||
|
|
||
|
|
||
| def sqlpool_sensitivity_label_create( |
There was a problem hiding this comment.
sqlpool_sensitivity_label_create [](start = 4, length = 32)
same comments with above function. #Resolved
There was a problem hiding this comment.
| client_factory=cf_synapse_client_sqlpool_sensitivity_labels_factory) as g: | ||
| g.custom_show_command('show', 'sqlpool_sensitivity_label_show') | ||
| g.command('list', 'list_current') | ||
| g.custom_command('create', 'sqlpool_sensitivity_label_create') |
There was a problem hiding this comment.
That is the new added cmdlet?
@idear1203 Can we use set to combine create and update ?
There was a problem hiding this comment.
| c.argument('enable_managed_virtual_network', options_list=['--enable-managed-vnet', | ||
| '--enable-managed-virtual-network'], | ||
| arg_type=get_three_state_flag(), | ||
| help='The flag indicates whether enable managed virtual network.') |
There was a problem hiding this comment.
help='The flag indicates whether enable managed virtual network.') [](start = 19, length = 66)
Better to add the description of the default value #Resolved
There was a problem hiding this comment.
The arg tye is get_three_state_flag when we run az synapse workspace create -h, for this parameter the help message is as bellow:
The flag indicates whether enable managed virtual network. Allowed values: false, true.
I think it is clear enough.
In reply to: 533221222 [](ancestors = 533221222)
| help='Whether enabling azure monitor target or not.', | ||
| arg_type=get_three_state_flag()) | ||
|
|
||
| policy_arg_group = 'Policy' |
There was a problem hiding this comment.
policy_arg_group = 'Policy [](start = 8, length = 26)
Duplicate definition
+230 +274 #Resolved
There was a problem hiding this comment.
| help='Threat detection policy state', | ||
| arg_type=get_enum_type(SecurityAlertPolicyState)) | ||
|
|
||
| c.argument('retention_days', |
There was a problem hiding this comment.
Remove blank line between c.argument may be better.
Not an important issue, it depends on you.
#Resolved
There was a problem hiding this comment.
|
Hi @jsntcy, @Juliehzl, @haroldrandom |
| helps['sql db classification recommendation enable'] = """ | ||
| type: command | ||
| short-summary: Enable sensitivity recommendations for a given column(recommendations are enabled by default on all columns). | ||
| examples: | ||
| - name: Enable sensitivity recommendations for a given column. | ||
| text: |- | ||
| az synapse sql pool classification recommendation enable --name sqlpool --workspace-name testsynapseworkspace \\ | ||
| --resource-group rg --schema dbo --table mytable --column mycolumn | ||
| """ | ||
|
|
||
| helps['sql db classification recommendation disable'] = """ | ||
| type: command | ||
| short-summary: Disable sensitivity recommendations for a given column(recommendations are enabled by default on all columns). | ||
| examples: | ||
| - name: Disable sensitivity recommendations for a given column. | ||
| text: |- | ||
| az synapse sql pool classification recommendation disable --name sqlpool --workspace-name testsynapseworkspace \\ | ||
| --resource-group rg --schema dbo --table mytable --column mycolumn | ||
| """ |
There was a problem hiding this comment.
The help entry name should be synapse sql pool classification ... instead of sql db classification ....
This blocks CI for sql module.
9 participants
Description
Testing Guide
History Notes
[Synapse]
1. Add new parameter --enable-managed-virtual-netowrk for az synapse workspace create cmdlet
2. Add new cmdlets az synapse sql pool classification create, show, update,list, delete
3. Add new cmdlets az synapse sql pool classification recommendation list, enable, disable
4. Add new cmdlets az synapse sql pool restore, list-deleted, show-connection-string
5. Add new cmdlets az synapse sql pool tde set, show
6. Add new cmdlets az synapse sql pool threat-policy show, update
7. Add new cmdlets az synapse sql pool audit-policy show, update
This checklist is used to make sure that common guidelines for a pull request are followed.
The PR title and description has followed the guideline in Submitting Pull Requests.
I adhere to the Command Guidelines.
I adhere to the Error Handling Guidelines.