Updating pool and database cmdlets to include optional LicenseType param #6311

johnpaulkee · 2018-05-25T21:30:01Z

Description

License type should be available as an option for customers when creating, updating SQL databases or SQL elastic pools. During restore, the license type should also be an option for SQL databases

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
  - The accepted review
- the cmdlet markdown files were generated using the platyPS module

jaredmoo

Minor style comments. Looks good!

jaredmoo · 2018-05-25T21:36:01Z

src/ResourceManager/Sql/Commands.Sql/Database Backup/Cmdlet/RestoreAzureRMSqlDatabase.cs

@@ -325,7 +332,8 @@ protected override AzureSqlDatabaseModel GetEntity()
                ElasticPoolName = ElasticPoolName,
                RequestedServiceObjectiveName = ServiceObjectiveName,
                Edition = Edition,
-                CreateMode = createMode
+                CreateMode = createMode,
+                LicenseType = LicenseType ?? null


?? null seems redundant

jaredmoo · 2018-05-25T21:36:26Z

src/ResourceManager/Sql/Commands.Sql/Database/Cmdlet/NewAzureSqlDatabase.cs

@@ -207,7 +214,8 @@ protected override AzureSqlDatabaseCreateOrUpdateModel ApplyUserInputToModel(Azu
                Tags = TagsConversionHelper.CreateTagDictionary(Tags, validate: true),
                ElasticPoolName = ElasticPoolName,
                ReadScale = ReadScale,
-                ZoneRedundant = MyInvocation.BoundParameters.ContainsKey("ZoneRedundant") ? (bool?)ZoneRedundant.ToBool() : null
+                ZoneRedundant = MyInvocation.BoundParameters.ContainsKey("ZoneRedundant") ? (bool?)ZoneRedundant.ToBool() : null,
+                LicenseType = LicenseType ?? null // note: default license type is LicenseIncluded


jaredmoo · 2018-05-25T21:38:03Z

src/ResourceManager/Sql/Commands.Sql/Elastic Pools/Cmdlet/NewAzureSqlElasticPool.cs

+        /// Gets or sets the license type for the Azure Sql database
+        /// </summary>
+        [Parameter(Mandatory = false, HelpMessage = "The license type for the Azure Sql database")]
+        [PSArgumentCompleter("LicenseIncluded", "BasePrice")]


https://github.com/Azure/azure-sdk-for-net/blob/psSdkJson6/src/SDKs/SqlManagement/Management.Sql/Generated/Models/DatabaseLicenseType.cs

jaredmoo · 2018-05-25T21:38:29Z

src/ResourceManager/Sql/Commands.Sql/Elastic Pools/Cmdlet/NewAzureSqlElasticPool.cs

@@ -201,7 +208,8 @@ protected override IEnumerable<AzureSqlElasticPoolModel> ApplyUserInputToModel(I
                Tags = TagsConversionHelper.CreateTagDictionary(Tags, validate: true),
                Location = location,
                ZoneRedundant = MyInvocation.BoundParameters.ContainsKey("ZoneRedundant") ? (bool?)ZoneRedundant.ToBool() : null,
-                MaxSizeBytes = MyInvocation.BoundParameters.ContainsKey("StorageMB") ? (long?)(StorageMB * Megabytes) : null
+                MaxSizeBytes = MyInvocation.BoundParameters.ContainsKey("StorageMB") ? (long?)(StorageMB * Megabytes) : null,
+                LicenseType = LicenseType ?? null


remove ?? null again

jaredmoo · 2018-05-25T21:38:49Z

src/ResourceManager/Sql/Commands.Sql/Elastic Pools/Cmdlet/SetAzureSqlElasticPool.cs

+        /// Gets or sets the license type for the Azure Sql database
+        /// </summary>
+        [Parameter(Mandatory = false, HelpMessage = "The license type for the Azure Sql database")]
+        [PSArgumentCompleter("LicenseIncluded", "BasePrice")]


https://github.com/Azure/azure-sdk-for-net/blob/psSdkJson6/src/SDKs/SqlManagement/Management.Sql/Generated/Models/ElasticPoolLicenseType.cs

jaredmoo · 2018-05-25T21:39:21Z

src/ResourceManager/Sql/Commands.Sql/Replication/Cmdlet/NewAzureSqlDatabaseCopy.cs

+        /// Gets or sets the license type for the Azure Sql database
+        /// </summary>
+        [Parameter(Mandatory = false, HelpMessage = "The license type for the Azure Sql database")]
+        [PSArgumentCompleter("LicenseIncluded", "BasePrice")]


use constants

… check

payiAzure · 2018-05-25T21:51:34Z

src/ResourceManager/Sql/Commands.Sql.Test/ScenarioTests/DatabaseReplicationTests.ps1

+
+	try
+	{
+		# Create a local database copy from a vcore database with base price license type - Default


For the comment. default is License Included not base price, right?

payiAzure · 2018-05-25T22:01:57Z

src/ResourceManager/Sql/Commands.Sql/Database/Model/AzureSqlDatabaseModel.cs

@@ -252,6 +257,8 @@ public AzureSqlDatabaseModel(string resourceGroup, string serverName, Management
            Family = database.Sku == null ? null : database.Sku.Family;

            SkuName = database.Sku == null ? null : database.Sku.Name;
+
+            LicenseType = database.LicenseType == null ? null : database.LicenseType;


don't need null as other places?

good catch. will update.

maddieclayton · 2018-05-25T23:46:19Z

@johnpaulkee Check out the ValidateMarkdownHelp.csv file here: https://azuresdkci.westus2.cloudapp.azure.com/job/powershell/4798/

johnpaulkee · 2018-05-26T01:17:37Z

Thanks @maddieclayton I've updated the markdown files.
Will check to see if the build succeeds this time.

johnpaulkee · 2018-05-26T05:26:15Z

Looks like Jenkins build succeeded.
https://azuresdkci.westus2.cloudapp.azure.com/job/powershell/4813/

Unsure why the Travis build is failing though. @maddieclayton https://travis-ci.org/Azure/azure-powershell/builds/383972854#L2438 can you take a look?

johnpaulkee · 2018-05-29T00:37:57Z

Travis build succeeded on retry it seems.
@maddieclayton can you check if it's good for merge?

maddieclayton · 2018-05-29T18:28:34Z

src/ResourceManager/Sql/Commands.Sql.Test/ScenarioTests/DatabaseCrudTests.ps1

+function Test-CreateVcoreDatabaseWithLicenseType
+{
+	# Setup
+	$location = "westcentralus"


This should use "Get-Location" https://github.com/Azure/azure-powershell/wiki/Azure-Powershell-Developer-Guide#using-common-code

Will update to use Get-Location

maddieclayton · 2018-05-29T18:30:08Z

src/ResourceManager/Sql/Commands.Sql.Test/ScenarioTests/DatabaseReplicationTests.ps1

+#>
+function Test-CreateVcoreDatabaseCopy
+{
+	Test-CreateVcoreCopyInternal "12.0" "Southeast Asia"


Same comment about location not being hardcoded - use Get-Location. This applies everywhere.

Will update to use Get-Location

maddieclayton · 2018-05-29T18:40:27Z

src/ResourceManager/Sql/Commands.Sql/Database/Cmdlet/NewAzureSqlDatabase.cs

@@ -207,7 +217,8 @@ protected override AzureSqlDatabaseCreateOrUpdateModel ApplyUserInputToModel(Azu
                Tags = TagsConversionHelper.CreateTagDictionary(Tags, validate: true),
                ElasticPoolName = ElasticPoolName,
                ReadScale = ReadScale,
-                ZoneRedundant = MyInvocation.BoundParameters.ContainsKey("ZoneRedundant") ? (bool?)ZoneRedundant.ToBool() : null
+                ZoneRedundant = MyInvocation.BoundParameters.ContainsKey("ZoneRedundant") ? (bool?)ZoneRedundant.ToBool() : null,
+                LicenseType = LicenseType // note: default license type is LicenseIncluded


Where are you setting the default?

Sorry if comment is confusing. Default is set in the SQL RP for License Type.
I'll update comment to be clearer.

maddieclayton · 2018-05-29T18:44:25Z

src/ResourceManager/Sql/Commands.Sql/help/New-AzureRmSqlElasticPool.md

@@ -214,7 +214,6 @@ The acceptable values for this parameter are:
 Type: String
 Parameter Sets: DtuBasedPool
 Aliases:
-Accepted values: None, Premium, Basic, Standard, DataWarehouse, Stretch, Free, PremiumRS


Why were these removed? Same for below.

Unsure.
The edition should be using these valid editions:

[PSArgumentCompleter("None", Management.Sql.Models.DatabaseEdition.Basic, Management.Sql.Models.DatabaseEdition.Standard, Management.Sql.Models.DatabaseEdition.Premium, Management.Sql.Models.DatabaseEdition.DataWarehouse, Management.Sql.Models.DatabaseEdition.Free, Management.Sql.Models.DatabaseEdition.Stretch, "GeneralPurpose", "BusinessCritical")]

Looks like this should updated anyway since GeneralPurpose and BusinessCritical needs to be added and PremiumRS needs to be deleted.

Will update these markdown files manually.

@maddieclayton for markdown generation, do PSArgumentCompleter values get automatically generated or do they get excluded?

Unlike ValidateSet or using an enum, it looks like PSArgumentCompleter doesn't have list of suggested strings get generated.

I'll add manually.

Updated to the correct accepted values @maddieclayton

Yes that's correct, PSArgumentCompleter is not caught by platyPS (it was created by us while ValidateSet is a built in powershell attribute)

maddieclayton · 2018-05-29T18:44:51Z

src/ResourceManager/Sql/Commands.Sql/help/Restore-AzureRmSqlDatabase.md

@@ -205,7 +210,6 @@ The acceptable values for this parameter are:
 Type: String
 Parameter Sets: FromPointInTimeBackup, FromDeletedDatabaseBackup, FromGeoBackup, FromLongTermRetentionBackup
 Aliases:
-Accepted values: None, Premium, Basic, Standard, DataWarehouse, Stretch, Free, PremiumRS


Same here (and in following files)

… new-azurermsqlelasticpool, and restore-azurermsqldatabase

maddieclayton · 2018-05-30T00:24:12Z

on demand job here: https://azuresdkci.westus2.cloudapp.azure.com/view/PowerShell/job/powershell-demand/898/

johnpaulkee · 2018-05-30T04:51:39Z

Thanks @maddieclayton for merging!

jaredmoo · 2018-05-30T15:17:53Z

Thanks @johnpaulkee and @maddieclayton for getting this into 6.2.0 !

John Paul Kee added 9 commits May 24, 2018 12:04

Initial addition of license type optional param

d6b1017

Initial updates to the markdown files

c0f8c0d

Initial edits to implementation

f6976b9

Fixing rename param set doc for set-azurermsqldatabase doc

f4cfd73

Fixing database crud tests

7be4fb0

Removing test

760dc43

Updating recorded scenario tests

c75fda2

Updating Sql ChangeLog.md

39b89f2

Fixing tabs

d4eaaf4

johnpaulkee requested review from jaredmoo and payiAzure May 25, 2018 21:30

azuresdkci added the needs-review label May 25, 2018

jaredmoo previously approved these changes May 25, 2018

View reviewed changes

jaredmoo requested a review from akromm-zz May 25, 2018 21:40

Updating cmdlets based on feedback. use constants, remove unnecessary…

954061d

… check

johnpaulkee dismissed jaredmoo’s stale review via 954061d May 25, 2018 21:50

payiAzure reviewed May 25, 2018

View reviewed changes

Removing default comment

fa5cfe6

payiAzure reviewed May 25, 2018

View reviewed changes

Removing unneccessary check in AzureSqlDatabaseModel

9c1e1a5

maddieclayton added needs-revision and removed needs-review labels May 25, 2018

maddieclayton assigned johnpaulkee May 25, 2018

Fixing markdown issues

18ba3cb

Fixing references to recorded tests

73b4f69

maddieclayton unassigned johnpaulkee May 29, 2018

maddieclayton self-assigned this May 29, 2018

maddieclayton added needs-review and removed needs-revision labels May 29, 2018

jaredmoo previously approved these changes May 29, 2018

View reviewed changes

maddieclayton suggested changes May 29, 2018

View reviewed changes

maddieclayton added needs-revision and removed needs-review labels May 29, 2018

maddieclayton assigned johnpaulkee and unassigned maddieclayton May 29, 2018

Updates based on feedback. Get-Location and markdown help files updated

f14a7a1

johnpaulkee dismissed jaredmoo’s stale review via f14a7a1 May 29, 2018 23:36

johnpaulkee added 2 commits May 29, 2018 16:42

Adding accepted values to other param sets in new-azurermsqldatabase,…

ab8d7ca

… new-azurermsqlelasticpool, and restore-azurermsqldatabase

Fix positioning of Accepted values in markdown help files

41a92a5

maddieclayton added needs-review and removed needs-revision labels May 30, 2018

maddieclayton assigned maddieclayton and unassigned johnpaulkee May 30, 2018

maddieclayton approved these changes May 30, 2018

View reviewed changes

maddieclayton added Waiting for On Demand and removed needs-review labels May 30, 2018

maddieclayton changed the base branch from preview to release-6.2.0 May 30, 2018 00:24

maddieclayton merged commit 2e56ab0 into Azure:release-6.2.0 May 30, 2018

Updating pool and database cmdlets to include optional LicenseType param #6311

Updating pool and database cmdlets to include optional LicenseType param #6311

Uh oh!

Conversation

johnpaulkee commented May 25, 2018

Description

Checklist

Uh oh!

jaredmoo 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!

Choose a reason for hiding this comment

Uh oh!

maddieclayton commented May 25, 2018

Uh oh!

johnpaulkee commented May 26, 2018

Uh oh!

johnpaulkee commented May 26, 2018

Uh oh!

johnpaulkee commented May 29, 2018

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!

johnpaulkee May 29, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johnpaulkee May 29, 2018 • 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!

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!

maddieclayton commented May 30, 2018

Uh oh!

johnpaulkee commented May 30, 2018

Uh oh!

jaredmoo commented May 30, 2018

Uh oh!

Uh oh!

johnpaulkee May 29, 2018 •

edited

Loading

johnpaulkee May 29, 2018 •

edited

Loading