Adding new Powershell cmdlets to support set/get/remove of Azure AD administrator on MI #10057

SanjaMalesevic · 2019-09-18T13:55:20Z

Description

Design review: https://github.com/Azure/azure-powershell-cmdlet-review-pr/issues/363

Checklist

I have read the Submitting Changes section of CONTRIBUTING.md
The title of the PR is clear and informative
The appropriate ChangeLog.md file(s) has been updated:
- For any service, the ChangeLog.md file can be found at src/{{SERVICE}}/{{SERVICE}}/ChangeLog.md
- A snippet outlining the change(s) made in the PR should be written under the ## Upcoming Release header -- no new version header should be added
The PR does not introduce breaking changes
If applicable, the changes made in the PR have proper test coverage
For public API changes to cmdlets:
- a cmdlet design review was approved for the changes in this repository (Microsoft internal only)
- the markdown help files have been regenerated using the commands listed here

…dministrator on MI

adxsdkps · 2019-09-18T13:55:23Z

Can one of the admins verify this patch?

The other tests which use Graph API are disabled because of existing problem with Graph authentication. I succeed to execute this test locally, but in error logs I saw this error: Unable to find a matching HTTP request for URL 'POST /72f988bf-86f1-41af-91ab-2d7cd011db47/getObjectsByObjectIds?' Here is the request from recorded json file 'POST /0c1edf5d-e5c5-4aca-ab69-ef194134f44b/getObjectsByObjectIds?api-version=1.6'. As you could see, the tenantId is different.

Syncing from original

Add ps commands for aad admin

…lesevic/azure-powershell into addPSCommandsForAadAdmin

isra-fel

@SanjaMalesevic Please see my inline comments, thanks.

...ctiveDirectoryAdministrator/Cmdlet/AzureSqlInstanceActiveDirectoryAdministratorCmdletBase.cs

...nceActiveDirectoryAdministrator/Cmdlet/RemoveAzureSqlInstanceActiveDirectoryAdministrator.cs

...ActiveDirectoryAdministrator/Services/AzureSqlInstanceActiveDirectoryAdministratorAdapter.cs

isra-fel · 2019-09-26T05:27:13Z

TODO for me: resolve markdown conflict once #10074 is merged

markcowl

A couple of test chnages, otherwise LGTM

markcowl · 2019-10-01T06:30:17Z

src/Sql/Sql.Test/ScenarioTests/ManagedInstanceActiveDirectoryAdministratorTests.ps1

+{
+	# Setup
+	$rg = Create-ResourceGroupForTest
+	$vnetName = "cl_initial"


You should generate random names for these - this allows each test execution to be indpenendent.

I check all other tests in SQL module, and they don't use random name for vnet. I suppose that the vnet's should stay deterministic so in the record mode the next test can create MI quickly. Also, I noticed that the vnet is never dropped.
So, if you agreee I would leave this as it is now.

There is an entire facility for creatingand recording random names used by essentially every other test project. As you have this now, anyone who runs two of your tests in the same subscription will fail. Please fix this.

markcowl · 2019-10-01T06:31:02Z

src/Sql/Sql.Test/ScenarioTests/ManagedInstanceActiveDirectoryAdministratorTests.ps1

+function Test-ManagedInstanceActiveDirectoryAdministrator
+{
+	# Setup
+	$rg = Create-ResourceGroupForTest


You should follow the pattern of cleaning up created resources as part of the test, using a try/finally after creating the resource group, where the ResourceGroup is removed in the finally is generally the pattern to follow.

markcowl · 2019-10-01T06:33:06Z

src/Sql/Sql.Test/ScenarioTests/SqlTestsBase.cs

+            if (HttpMockServer.Mode == HttpRecorderMode.Record)
+            {
+                tenantId = TestEnvironmentFactory.GetTestEnvironment().Tenant;
+            }


You want to store the tenant ID in HttpMockServer.Variables in this case, I think

src/Sql/Sql.Test/ScenarioTests/ManagedInstanceActiveDirectoryAdministratorTests.ps1

All comments addressed

After #10074 was merged, #10057 should have updated `ChangeLog.md` but it didn't, so I'm doing it right now.

Adding new Powershell cmdlets to support set/get/remove of Azure AD a…

cc77be4

…dministrator on MI

azuresdkci added the needs-review label Sep 18, 2019

SanjaMalesevic added 4 commits September 18, 2019 16:06

Merge branch 'master' into addPSCommandsForAadAdmin

a938639

Fix syntax error in this file. It has occurred during merge.

e910ebc

Delete recorded test

f711aff

erich-wang assigned isra-fel Sep 19, 2019

SanjaMalesevic and others added 4 commits September 19, 2019 10:35

Merge pull request #1 from Azure/master

8de5d3f

Syncing from original

Merge pull request #2 from SanjaMalesevic/addPSCommandsForAadAdmin

135c95e

Add ps commands for aad admin

Add recorded file

84c7242

Remove file from local history

7785de3

isra-fel added Cmdlet Review Required ⚠️ and removed needs-review labels Sep 20, 2019

SanjaMalesevic added 5 commits September 23, 2019 11:16

Add InputObject Parameter Set

6963685

Merge branch 'addPSCommandsForAadAdmin' of https://github.com/SanjaMa…

b726588

…lesevic/azure-powershell into addPSCommandsForAadAdmin

Add ResourceId Parameter Set

9a82ef7

Set default parameter set for get, set and remove

86a8be5

Rename parameter Name into InstanceName

809c89f

isra-fel added needs-review and removed Cmdlet Review Required ⚠️ labels Sep 25, 2019

isra-fel previously requested changes Sep 25, 2019

View reviewed changes

isra-fel added needs-revision and removed needs-review labels Sep 25, 2019

isra-fel assigned SanjaMalesevic and unassigned isra-fel Sep 25, 2019

Code review fixes

b4a6a65

isra-fel added needs-review and removed needs-revision labels Sep 26, 2019

isra-fel assigned isra-fel and unassigned SanjaMalesevic Sep 26, 2019

SanjaMalesevic added 4 commits September 27, 2019 10:48

Enable test for AAD admin on MI

29f37e3

Read TenantId from json during Playback mode

b597bf9

Renaming session record test folder

2e16333

Update tenant id in DefaultContext using value from json

4d68fed

markcowl unassigned isra-fel Oct 1, 2019

markcowl requested changes Oct 1, 2019

View reviewed changes

markcowl added needs-revision and removed needs-review labels Oct 1, 2019

markcowl assigned SanjaMalesevic Oct 1, 2019

Code review fixes 2

00c8452

markcowl requested changes Oct 3, 2019

View reviewed changes

src/Sql/Sql.Test/ScenarioTests/ManagedInstanceActiveDirectoryAdministratorTests.ps1 Show resolved Hide resolved

Adding a comment into a test

1ce0a90

markcowl approved these changes Oct 4, 2019

View reviewed changes

markcowl merged commit 6eed678 into Azure:master Oct 4, 2019

isra-fel added a commit that referenced this pull request Oct 8, 2019

Fix merging problem caused by #10057 & #10074

a50e8f7

After #10074 was merged, #10057 should have updated `ChangeLog.md` but it didn't, so I'm doing it right now.

isra-fel mentioned this pull request Oct 8, 2019

Fix merging problem in ChangeLog.md #10212

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Adding new Powershell cmdlets to support set/get/remove of Azure AD administrator on MI #10057

Adding new Powershell cmdlets to support set/get/remove of Azure AD administrator on MI #10057

Uh oh!

SanjaMalesevic commented Sep 18, 2019 •

edited by isra-fel

Loading

Uh oh!

adxsdkps commented Sep 18, 2019

Uh oh!

isra-fel left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

isra-fel commented Sep 26, 2019

Uh oh!

markcowl left a comment

Uh oh!

markcowl Oct 1, 2019

Uh oh!

SanjaMalesevic Oct 1, 2019

Uh oh!

markcowl Oct 3, 2019

Uh oh!

markcowl Oct 1, 2019

Uh oh!

SanjaMalesevic Oct 1, 2019

Uh oh!

markcowl Oct 1, 2019

Uh oh!

SanjaMalesevic Oct 1, 2019

Uh oh!

Uh oh!

Uh oh!

Adding new Powershell cmdlets to support set/get/remove of Azure AD administrator on MI #10057

Adding new Powershell cmdlets to support set/get/remove of Azure AD administrator on MI #10057

Uh oh!

Conversation

SanjaMalesevic commented Sep 18, 2019 • edited by isra-fel Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Uh oh!

adxsdkps commented Sep 18, 2019

Uh oh!

isra-fel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

isra-fel commented Sep 26, 2019

Uh oh!

markcowl left a comment

Choose a reason for hiding this comment

Uh oh!

markcowl Oct 1, 2019

Choose a reason for hiding this comment

Uh oh!

SanjaMalesevic Oct 1, 2019

Choose a reason for hiding this comment

Uh oh!

markcowl Oct 3, 2019

Choose a reason for hiding this comment

Uh oh!

markcowl Oct 1, 2019

Choose a reason for hiding this comment

Uh oh!

SanjaMalesevic Oct 1, 2019

Choose a reason for hiding this comment

Uh oh!

markcowl Oct 1, 2019

Choose a reason for hiding this comment

Uh oh!

SanjaMalesevic Oct 1, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

SanjaMalesevic commented Sep 18, 2019 •

edited by isra-fel

Loading