[SQL] Support setting of AAD administrator on Managed Instance #10446

SanjaMalesevic · 2019-09-06T07:55:51Z

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.

…Malesevic/azure-cli into setAADAdminOnMiCliCommands

yonzhan · 2019-09-16T08:12:46Z

@jaredmoo please review and let us know when it is ready to merge.

jaredmoo · 2019-09-20T21:45:46Z

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

+        c.argument('managed_instance_name',
+                   arg_type=managed_instance_param_type)
+
+        c.argument('login',


Please define arg types (at top of file) that can be used for both sql server and MI arguments, instead of copying

jaredmoo · 2019-09-20T21:46:24Z

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

+
+    with self.argument_context('sql mi ad-admin create') as c:
+        # Create args that will be used to build up the ManagedInstanceAdministrator object
+        create_args_for_complex_type(


I think you can put this in sql mi ad-admin section so that it applies to both create and update (to avoid repetition)

I've tried to do this, but when executing get or delete command get the following error: ValueError: command authoring error: extra argument 'login' cannot be registered to a group-level scope 'sql mi ad-admin'. It must be registered to a specific command.
So, I need to keep both "sql mi ad-admin create" and "sql mi ad-admin update" sections.

jaredmoo · 2019-09-20T21:46:56Z

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

-            server_key_name=key_name
-        )
+        server_key_type=server_key_type,
+        server_key_name=key_name


Why is this changed?

Build wasn't successful because of failed test test_sql_tdebyok (azure.cli.command_modules.sql.tests.latest.test_sql_commands.SqlTransparentDataEncryptionScenarioTest). You can find more on this link https://github.com/Azure/azure-cli/runs/214404678.
After I've fixed this, all checks have passed

Azure/azure-sdk-for-python@58857ae

@zikalino , can you clarify why this breaking change happened?

jaredmoo · 2019-09-20T21:47:46Z

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

+
+    profile = Profile()
+    sub = profile.get_subscription()
+    kwargs['tenant_id'] = sub['tenantId']


Please extract function which gets tenant id so that the logic isn't duplicated between server and mi

jaredmoo · 2019-10-01T15:49:17Z

src/azure-cli/HISTORY.rst

+
+**SQL**
+
+* New Cmdlets for Management.Sql that supports setting AAD administrator on Managed instance


Instead of "Management.Sql", please use the command names i.e. "sql mi ad-admin"

jaredmoo · 2019-10-01T15:50:02Z

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

+    """
+helps['sql mi ad-admin create'] = """
+    type: command
+    short-summary: Create a new managed instance Active Directory administrator.


jaredmoo · 2019-10-01T15:50:23Z

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

+    """
+helps['sql mi ad-admin update'] = """
+    type: command
+    short-summary: Update an existing managed instance Active Directory administrator.


jaredmoo · 2019-10-01T15:52:01Z

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



+def _get_tenant_id():
+    from azure.cli.core._profile import Profile


Please add '''description''' to function

jaredmoo · 2019-10-01T15:54:46Z

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

+    return client.create_or_update(
+        resource_group_name=resource_group_name,
+        managed_instance_name=managed_instance_name,
+        administrator_name="ActiveDirectory",


define constant, e.g. ACTIVE_DIRECTORY = "ActiveDirectory" so that string isn't repeated

Also unclear why this is needed compared to SQL server admin

Skip this for now, because it will be removed after we change api specification and update the python client

jaredmoo · 2019-10-01T16:10:59Z

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

-            server_key_name=key_name
-        )
+        server_key_type=server_key_type,
+        server_key_name=key_name


Azure/azure-sdk-for-python@58857ae

@zikalino , can you clarify why this breaking change happened?

jaredmoo

Several comments but overall looks good

yonzhan · 2019-10-02T15:16:55Z

@jiasli please take a look and comment.

yonzhan · 2019-10-08T02:04:57Z

please Rebase against the dev branch. Which should get Travis CI successfully run.

fengzhou-msft · 2019-10-10T07:42:49Z

@SanjaMalesevic Can you rebase against dev and fix the conflicts? We're going to build for the next release on Friday Shanghai time.

yonzhan · 2019-10-11T06:46:06Z

This PR was merged and will be incorporated in our next release.

SanjaMalesevic added 4 commits August 30, 2019 18:24

Support managing of Azure AD administrator on Managed Instance

6c6441a

Add AAD admin test

70b2e03

Add release notes

ee93a67

Add release notes and upgrade version in requirements

d8cc932

SanjaMalesevic requested a review from jaredmoo September 6, 2019 07:55

SanjaMalesevic and others added 10 commits September 6, 2019 09:58

Revert version in setup.py

e2da8c5

Delete testcert.cer

c2afe5b

Delete testingAadAdmin.playlist

2463fba

Delete testkey.pvk

5828b13

Merge branch 'dev' into setAADAdminOnMiCliCommands

c05ae62

Update setup.py

1762a83

Update HISTORY.rst

7965e1f

Test fixes

856bbd7

Merge branch 'setAADAdminOnMiCliCommands' of https://github.com/Sanja…

6184aff

…Malesevic/azure-cli into setAADAdminOnMiCliCommands

Test fixes 2

6e76c9a

jaredmoo suggested changes Sep 20, 2019

View reviewed changes

SanjaMalesevic and others added 6 commits September 24, 2019 13:16

Merge branch 'dev' into setAADAdminOnMiCliCommands

229aaa7

Update History.rst

6219076

Code review fixes

b31e456

Resolving merge conflicts

27f51f1

Fix help file

43d6b2d

Update recorded file for test SqlInstanceFailoverGroupMgmtScenarioTest

3d6ceed

jaredmoo reviewed Oct 1, 2019

View reviewed changes

jaredmoo previously approved these changes Oct 1, 2019

View reviewed changes

Merge branch 'dev' into setAADAdminOnMiCliCommands

649c1ce

SanjaMalesevic dismissed jaredmoo’s stale review via 649c1ce October 1, 2019 16:24

SanjaMalesevic added 2 commits October 1, 2019 18:25

Update _help.py based on comments

cd1d265

Update file base on CR comment

489daad

jaredmoo previously approved these changes Oct 1, 2019

View reviewed changes

yonzhan requested a review from jiasli October 2, 2019 10:51

yonzhan assigned jiasli Oct 2, 2019

yonzhan added this to the Sprint 75 milestone Oct 2, 2019

Merge branch 'dev' into setAADAdminOnMiCliCommands

ec3b7cb

SanjaMalesevic dismissed jaredmoo’s stale review via ec3b7cb October 10, 2019 08:33

SanjaMalesevic added 2 commits October 10, 2019 13:19

Test fixes

20ee134

Record the test - api version is changed

e236f28

SanjaMalesevic requested a review from jaredmoo October 10, 2019 16:21

jaredmoo approved these changes Oct 10, 2019

View reviewed changes

haroldrandom merged commit 0970d1c into Azure:dev Oct 11, 2019

SanjaMalesevic mentioned this pull request Oct 24, 2019

AdministratorName parameter should take value which is defined in enum Azure/azure-rest-api-specs#7377

Merged

SanjaMalesevic deleted the setAADAdminOnMiCliCommands branch November 13, 2019 10:35


		SQL

		* New Cmdlets for Management.Sql that supports setting AAD administrator on Managed instance



		def _get_tenant_id():
		from azure.cli.core._profile import Profile

[SQL] Support setting of AAD administrator on Managed Instance #10446

[SQL] Support setting of AAD administrator on Managed Instance #10446

Uh oh!

Conversation

SanjaMalesevic commented Sep 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yonzhan commented Sep 16, 2019

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!

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!

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!

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!

jaredmoo left a comment

Choose a reason for hiding this comment

Uh oh!

yonzhan commented Oct 2, 2019

Uh oh!

yonzhan commented Oct 8, 2019

Uh oh!

fengzhou-msft commented Oct 10, 2019

Uh oh!

yonzhan commented Oct 11, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

SanjaMalesevic commented Sep 6, 2019 •

edited

Loading