Adding VirtualCluster related cmdlets and corresponding tests... #8932

milostod · 2019-04-03T16:58:54Z

...and help files.

Description

This change adds cmdlets related to VirtualCluster resource. The intention is to enable customers to get information about virtual cluster(s) under their subscription and resource group, as well as to remove specified virtual cluster, which was a major ask from our customers. Explicit creation of new virtual clusters is not supported, as virtual cluster resource is only implicitly created when customer creates first managed instance in the specified subnet. Similarly, updates on virtual clusters are not currently supported.
Test coverage and recordings has been added for all chenges, as well as markdown help files.
Cmdlet design review is completed here: https://github.com/Azure/azure-powershell-cmdlet-review-pr/issues/271

Checklist

I have read the Submitting Changes section of CONTRIBUTING.md
The title of the PR is clear and informative
The appropriate change log has been updated
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:
- the changes have gone through a cmdlet design review (Microsoft internal partners only)
- the cmdlet markdown files were generated using the platyPS module

…p files.

adxsdkps · 2019-04-03T17:01:27Z

Can one of the admins verify this patch?

milostod · 2019-04-03T17:18:55Z

src/Sql/Sql.Test/Sql.Test.csproj

@@ -11,7 +11,7 @@
  </PropertyGroup>

  <ItemGroup>
-    <PackageReference Include="Microsoft.Azure.Management.Sql" Version="1.28.0-preview" />
+    <PackageReference Include="Microsoft.Azure.Management.Sql" Version="1.29.0-preview" />


Note: New NuGet package has been published from the Azure SDK for .NET repository

milostod · 2019-04-03T17:27:27Z

Note: there are some auto-generated changes in Az.Sql.md related to sensitivity classification/recommendation, as well as in Set-AzSqlDatabaseThreatDetectionPolicy.md and Set-AzSqlServerThreatDetectionPolicy.md. My guess is that original changes in these areas were not accompanied with md update through platyPS, so I included those in my change.

cormacpayne · 2019-04-03T22:41:27Z

@azuresdkci add to whitelist

cormacpayne

@milostod a few minor comments, otherwise LGTM

cormacpayne · 2019-04-03T22:43:10Z

src/Sql/Sql/VirtualCluster/Cmdlet/GetAzureSqlVirtualCluster.cs

+            HelpMessage = "The name of the resource group.")]
+        [Parameter(ParameterSetName = GetByResourceGroupParameterSet,
+            Mandatory = false,
+            Position = 1,


@milostod please remove the Position property from optional parameters

cormacpayne · 2019-04-03T22:45:31Z

src/Sql/Sql/VirtualCluster/Cmdlet/RemoveAzureSqlVirtualCluster.cs

+        /// </summary>
+        [Parameter(ParameterSetName = RemoveByResourceIdParameterSet,
+            Mandatory = true,
+            Position = 0,


@milostod minor: you can remove this positional parameter since users will never be able to use it (if they provide a single string, PowerShell won't be able to differentiate between the Name/ResourceGroupName and ResourceId parameter sets, and since the Name/ResourceGroupName set is the default, it will choose that one and prompt the user to provide a value for -ResourceGroupName)

I see, thanks. I've made the change.

cormacpayne · 2019-04-03T22:46:52Z

src/Sql/Sql/VirtualCluster/Services/AzureSqlVirtualClusterAdapter.cs

+            // Extract the resource group name from the ID.
+            // ID is in the form:
+            // /subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/rgName/providers/Microsoft.Sql/virtualClusters/virtualClusterName
+            string[] segments = resp.Id.Split('/');


@milostod you can use the ResourceIdentifier class that's in our common ARM code to safely parse a provided resource id string

Nice, thanks for the comment.

milostod · 2019-04-05T11:00:35Z

@cormacpayne I've made changes we discussed. Windows and Linux builds failed in automated checks, although build works fine on my local machine. It seems I cannot request rebuild.

Syncing changes from original

milostod · 2019-04-10T09:56:59Z

@cormacpayne @maddieclayton, I've made changes and fixed merge conflicts, please review.

maddieclayton

Few very small comments, then this should be good to go.

maddieclayton · 2019-04-10T20:37:22Z

src/Sql/Sql.Test/ScenarioTests/VirtualClusterCrudScenarioTests.ps1

+function Test-GetVirtualCluster
+{
+	# Setup
+	$location = "eastus"


Please get location using Get-Location to ensure this will work on all clouds: https://github.com/Azure/azure-powershell/blob/master/tools/ScenarioTest.ResourceManager/Common.ps1#L583

Hi @maddieclayton , I used some pre-made resources to speed up the test because provisioning of a new virtual cluster takes several hours. I've made the changes to create everything in the test, however, each test takes almost 4 hours to execute. If this is not a problem, I'll commit the change. Please let me know.

@milostod It is fine to take 4 hours to record this test - because the SDK calls will be instantaneous in playback mode, it will not affect our CI, and having a test take 4 hours to record is much better than having to do manual set up that takes the same amount of time.

@maddieclayton I agree. I've made the changes, tests are now creating everything from scratch, including ResourceGroup, VNet and Subnet. Total running time was 7hrs:30min for two tests. I used Get-ProviderLocation since it uses Get-AzResourceProvider instead of Get-AzureRmResourceProvider command for which I had some trouble running locally.

maddieclayton · 2019-04-10T20:37:53Z

src/Sql/Sql.Test/ScenarioTests/VirtualClusterCrudScenarioTests.ps1

+	$location = "eastus"
+	$rg = Create-ResourceGroupForTest $location
+
+	$rgName = "RG_MIPlayground"


Please have all resource name be randomly generated using getAssetName: https://github.com/Azure/azure-powershell/blob/master/tools/ScenarioTest.ResourceManager/Common.ps1#L371

Done. rgName is created in Create-ResourceGroupForTest using getAssetName. I kept hardcoded names for vnet and subnet as it seems that calling getAssetName multiple times in short period produces "queue is empty" error. I see multiple tests are keeping hardcoded values for vnet and subnet, probably for the same reason.

maddieclayton · 2019-04-10T20:38:27Z

src/Sql/Sql/Az.Sql.psd1

@@ -271,7 +272,7 @@ PrivateData = @{
        # IconUri = ''

        # ReleaseNotes of this module
-        ReleaseNotes = '* Support Database Data Classification.'
+        ReleaseNotes = '* Add Virtual Cluster cmdlets'


Please revert this change, we will update during versioning.

Done, thanks.

…ments.

addressed

Adding VirtualCluster related cmdlets and corresponding tests and hel…

ee44471

…p files.

azuresdkci added the needs-review label Apr 3, 2019

Updating change log.

0dc8943

milostod commented Apr 3, 2019

View reviewed changes

cormacpayne previously requested changes Apr 3, 2019

View reviewed changes

cormacpayne added needs-revision and removed needs-review labels Apr 3, 2019

cormacpayne assigned milostod Apr 3, 2019

Addressing comments.

1f3b0d4

milostod requested a review from cormacpayne April 5, 2019 11:00

maddieclayton added needs-review Merge Conflicts needs-revision and removed needs-revision needs-review labels Apr 8, 2019

milostod and others added 2 commits April 9, 2019 19:43

Merge pull request #1 from Azure/master

fd18400

Syncing changes from original

Modifying Az.Sql.psd1 after the merge from orig.

d7df235

maddieclayton added needs-review and removed Merge Conflicts needs-revision labels Apr 10, 2019

maddieclayton assigned maddieclayton and unassigned milostod Apr 10, 2019

maddieclayton suggested changes Apr 10, 2019

View reviewed changes

maddieclayton added needs-revision and removed needs-review labels Apr 10, 2019

maddieclayton assigned milostod and unassigned maddieclayton Apr 10, 2019

maddieclayton added needs-review and removed needs-revision labels Apr 11, 2019

maddieclayton assigned maddieclayton and unassigned milostod Apr 11, 2019

maddieclayton added needs-revision and removed needs-review labels Apr 11, 2019

maddieclayton assigned milostod and unassigned maddieclayton Apr 11, 2019

Modifying tests not to require any prepared resources. Addressing com…

d592aa8

…ments.

maddieclayton added needs-review and removed needs-revision labels Apr 12, 2019

maddieclayton assigned maddieclayton and unassigned milostod Apr 12, 2019

maddieclayton approved these changes Apr 12, 2019

View reviewed changes

maddieclayton merged commit 25679d6 into Azure:master Apr 12, 2019

Adding VirtualCluster related cmdlets and corresponding tests... #8932

Adding VirtualCluster related cmdlets and corresponding tests... #8932

Uh oh!

Conversation

milostod commented Apr 3, 2019 • edited by SergeyRazmyslov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Uh oh!

adxsdkps commented Apr 3, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

milostod commented Apr 3, 2019

Uh oh!

cormacpayne commented Apr 3, 2019

Uh oh!

cormacpayne left a comment

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!

milostod commented Apr 5, 2019

Uh oh!

milostod commented Apr 10, 2019

Uh oh!

maddieclayton left a comment

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!

Uh oh!

milostod commented Apr 3, 2019 •

edited by SergeyRazmyslov

Loading