[Synapse] Add new cmdlets for synapse sql aad admin and sql audit policy and add update cmdlet for synapse workspace firewall rule by aim-for-better · Pull Request #16241 · Azure/azure-cli

aim-for-better · 2020-12-11T05:53:09Z

Description

Add new cmdlets az synapse sql ad-admin show, create, update, delete
Add new cmdlet az synapse workspace firewall-rule update
Add new cmdlets az synapse sql audit-policy show, update

Testing Guide

History Notes

[Synapse] 1. Add new cmdlets az synapse sql ad-admin show, create, update, delete
2. Add new cmdlet az synapse workspace firewall-rule update
3. Add new cmdlets az synapse sql 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.

…ons (Azure#12771)

Support az synapse sql ad-admin create, update, delete, show Support az synapse workspace firewall-rule update Support az synapse sql audit-policy show, update

aim-for-better · 2020-12-11T06:18:41Z

Is the linter rule "MEDIUM severity: no_required_location_param" new added?

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

Juliehzl · 2020-12-15T13:52:20Z

src/azure-cli/azure/cli/command_modules/synapse/operations/sqlpoolblobauditingpolicy.py

+def _validate_audit_policy_arguments(state=None, storage_account=None, storage_endpoint=None,
+                                     storage_account_access_key=None, retention_days=None):
+    blob_storage_arguments_provided = any(
+        [storage_account, storage_endpoint, storage_account_access_key, retention_days])
+    if not state and not blob_storage_arguments_provided:
+        raise CLIError('Either state or blob storage arguments are missing')
+
+    if retention_days is not None and (not retention_days.isdigit() or int(retention_days) <= 0):
+        raise CLIError('retention-days must be a positive number greater than zero')


such validation would be recommended to add as validator and other commands/parameters could utilize the function.
see https://github.com/Azure/azure-cli/blob/dev/src/azure-cli/azure/cli/command_modules/storage/_params.py#L351

such validation would be recommended to add as validator and other commands/parameters could utilize the function.
see https://github.com/Azure/azure-cli/blob/dev/src/azure-cli/azure/cli/command_modules/storage/_params.py#L351

Hi @Juliehzl , thanks for your comment, I have removed the validation for retention_days. I agree with you, but per my understanding, the validator need to bind with one parameter which means one validator for one parameter, how should we validate multiple parameters in validator?

Hi @Juliehzl , for this comments, could you please give some suggestions? Thanks

I have moved this function to _validators.py and add validator in command level. Thanks for this comments, it is my first time to know that we can add validator in command level.

src/azure-cli/azure/cli/command_modules/synapse/operations/workspace.py

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

…group function in _client_factory.py

aim-for-better · 2020-12-17T10:30:04Z

/azp run

azure-pipelines · 2020-12-17T10:30:19Z

Azure Pipelines successfully started running 2 pipeline(s).

aim-for-better · 2020-12-18T04:58:05Z

Hi @jsntcy Could you please help review this PR? It is almost 5 work days. We hope we can catch S180. Thank you very much~

aim-for-better · 2020-12-18T05:05:20Z

/azp run

azure-pipelines · 2020-12-18T05:05:33Z

Azure Pipelines successfully started running 2 pipeline(s).

aim-for-better · 2020-12-21T05:39:33Z

/azp run

azure-pipelines · 2020-12-21T05:39:47Z

Azure Pipelines successfully started running 2 pipeline(s).

Juliehzl · 2020-12-21T11:36:15Z

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

+        g.show_command('show', 'get')
+        g.generic_update_command('update', custom_func_name='sqlserver_blob_auditing_policy_update')


only two commands for audit-policy? What about delete, create, list?

Here we just to keep consistent with az sql server. And I checked for az synapse sql audit-policy the dependent api is a long running, but for az synapse sql pool audit-policy the dependent api is not long running.
I have add wait command and add suppport_no_wait=True for this comments.

Juliehzl · 2020-12-21T11:40:20Z

src/azure-cli/azure/cli/command_modules/synapse/tests/latest/test_synapse_scenario.py

+        self.cmd('az synapse sql audit-policy update --state Enabled --storage-account {storage-account} '
+                 '--workspace-name {workspace} --resource-group {rg}')
+
+        self.cmd('az synapse sql audit-policy show --workspace-name {workspace} --resource-group {rg}',
+                 checks=[
+                     self.check('state', 'Enabled')
+                 ])


could we also check the update command?

could we get first, update and then get again to see the value change?

please add update for all parameters.

could we also check the update command?

could we get first, update and then get again to see the value change?

please add update for all parameters.

Thanks, I have updated the test for az synapse sql audit-policy and az synapse sql pool audit-policy

src/azure-cli/azure/cli/command_modules/synapse/tests/latest/test_synapse_scenario.py

yonzhan · 2020-12-21T12:14:42Z

Please address the comments raised by @Juliehzl before merging this PR.

… update tests

aim-for-better · 2020-12-22T09:00:49Z

Please address the comments raised by @Juliehzl before merging this PR.

Sure. Thanks

dkmiller and others added 22 commits April 27, 2020 16:13

{Docs} Remove stale reference in README to closed issue about extensi…

00b9235

…ons (Azure#12771)

Merge remote-tracking branch 'upstream/release'

ab16df0

Merge remote-tracking branch 'upstream/release'

03436bc

Merge remote-tracking branch 'upstream/release'

7d98b1f

Merge remote-tracking branch 'upstream/release'

06f6ec5

Merge remote-tracking branch 'upstream/release'

adab2d2

Merge remote-tracking branch 'upstream/release'

c7b9f98

Merge remote-tracking branch 'upstream/release'

18e8b8d

Merge remote-tracking branch 'upstream/release'

1eb5074

Merge remote-tracking branch 'upstream/release' into master

cc3a159

Merge remote-tracking branch 'upstream/release' into master

af8f98e

Merge remote-tracking branch 'upstream/release' into master

0f904b9

Merge remote-tracking branch 'upstream/release' into master

d17a993

Merge branch 'master' of github.com:Azure/azure-cli into dev

331b406

Merge branch 'dev' of github.com:Azure/azure-cli into dev

c98c195

Merge branch 'dev' of github.com:Azure/azure-cli into dev

ee648a0

Merge branch 'dev' of github.com:Azure/azure-cli into dev

d2faa9f

Merge branch 'dev' of github.com:Azure/azure-cli into dev

64d0de4

Merge branch 'dev' of github.com:Azure/azure-cli into dev

7b250f5

Merge branch 'dev' of github.com:Azure/azure-cli into dev

03c8785

Merge branch 'dev' of github.com:Azure/azure-cli into dev

2f814b1

Batch 2:

5c00b61

Support az synapse sql ad-admin create, update, delete, show Support az synapse workspace firewall-rule update Support az synapse sql audit-policy show, update

aim-for-better requested review from idear1203 and sunsw1994 as code owners December 11, 2020 05:53

aim-for-better added this to the S180 milestone Dec 11, 2020

aim-for-better changed the title ~~[Synapse] Support SQL DW cmdlet batch2~~ [Synapse] Add new cmdlets for synapse sql aad admin and sql audit policy and add update cmdlet for synapse workspace firewall rule Dec 11, 2020

Fix linter and style error

4c49538

aim-for-better force-pushed the SynapseSQLDWBatch2 branch from 9e2977b to 492737a Compare December 11, 2020 06:53

Make location as optional in create workspace

fbf2482

Juliehzl reviewed Dec 15, 2020

View reviewed changes

src/azure-cli/azure/cli/command_modules/synapse/commands.py Outdated Show resolved Hide resolved

Juliehzl reviewed Dec 15, 2020

View reviewed changes

src/azure-cli/azure/cli/command_modules/synapse/operations/workspace.py Outdated Show resolved Hide resolved

aim-for-better force-pushed the SynapseSQLDWBatch2 branch from 899623b to 7ccb153 Compare December 16, 2020 07:21

Fix the first iteration comments

3f55253

aim-for-better force-pushed the SynapseSQLDWBatch2 branch from 7ccb153 to 3f55253 Compare December 16, 2020 07:27

aim-for-better requested a review from Juliehzl December 16, 2020 07:30

Juliehzl reviewed Dec 17, 2020

View reviewed changes

src/azure-cli/azure/cli/command_modules/synapse/commands.py Outdated Show resolved Hide resolved

Add confirmation for ad-admin delete cmdlet and remove cf_resource_…

6bcdfcd

…group function in _client_factory.py

aim-for-better requested a review from Juliehzl December 17, 2020 09:40

Fix ad-admin test error

8b77b75

Juliehzl reviewed Dec 21, 2020

View reviewed changes

src/azure-cli/azure/cli/command_modules/synapse/tests/latest/test_synapse_scenario.py Show resolved Hide resolved

Juliehzl reviewed Dec 21, 2020

View reviewed changes

src/azure-cli/azure/cli/command_modules/synapse/tests/latest/test_synapse_scenario.py Show resolved Hide resolved

Addressed comments: add wait command for synapse sql audit-policy and…

8ec6270

… update tests

aim-for-better requested a review from Juliehzl December 22, 2020 08:42

Juliehzl approved these changes Dec 22, 2020

View reviewed changes

jsntcy approved these changes Dec 22, 2020

View reviewed changes

sunsw1994 approved these changes Dec 23, 2020

View reviewed changes

jsntcy merged commit 94dcc6a into Azure:dev Dec 23, 2020

		g.show_command('show', 'get')
		g.generic_update_command('update', custom_func_name='sqlserver_blob_auditing_policy_update')

Conversation

aim-for-better commented Dec 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aim-for-better commented Dec 11, 2020

Uh oh!

Uh oh!

Juliehzl Dec 15, 2020 • 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!

aim-for-better Dec 16, 2020

Choose a reason for hiding this comment

Uh oh!

aim-for-better Dec 21, 2020

Choose a reason for hiding this comment

Uh oh!

aim-for-better Dec 22, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

aim-for-better commented Dec 17, 2020

Uh oh!

azure-pipelines bot commented Dec 17, 2020

Uh oh!

aim-for-better commented Dec 18, 2020

Uh oh!

aim-for-better commented Dec 18, 2020

Uh oh!

azure-pipelines bot commented Dec 18, 2020

Uh oh!

aim-for-better commented Dec 21, 2020

Uh oh!

azure-pipelines bot commented Dec 21, 2020

Uh oh!

Juliehzl Dec 21, 2020

Choose a reason for hiding this comment

Uh oh!

aim-for-better Dec 22, 2020 • 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!

Juliehzl Dec 21, 2020

Choose a reason for hiding this comment

Uh oh!

aim-for-better Dec 22, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

yonzhan commented Dec 21, 2020

Uh oh!

aim-for-better commented Dec 22, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

aim-for-better commented Dec 11, 2020 •

edited

Loading

Juliehzl Dec 15, 2020 •

edited

Loading

aim-for-better Dec 22, 2020 •

edited

Loading