ADL Updates to use latest GA packages. #3261

begoldsm · 2016-12-07T19:58:46Z

Description

This checklist is used to make sure that common guidelines for a pull request are followed. You can find a more complete discussion of PowerShell cmdlet best practices here.

I have read the contribution guidelines.
If changes were made to any cmdlet, the XML help was regenerated using the platyPSHelp module.
If any large changes are made to a service, they are reflected in the respective change log.

General Guidelines

Title of the pull request is clear and informative.
There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For more information on cleaning up the commits in your PR, see this page.
The pull request does not introduce breaking changes (unless a major version change occurs in the assembly and module).

Testing Guidelines

Pull request includes test coverage for the included changes.
PowerShell scripts used in tests should do any necessary setup as part of the test or suite setup, and should not use hard-coded values for locations or existing resources.

Cmdlet Signature Guidelines

New cmdlets that make changes or have side effects should implement ShouldProcess and have SupportShouldProcess=true specified in the cmdlet attribute. You can find more information on ShouldProcess here.
Cmdlet specifies OutputType attribute if any output is produced - if the cmdlet produces no output, it should implement a PassThrough parameter.

Cmdlet Parameter Guidelines

Parameter types should not expose types from the management library - complex parameter types should be defined in the module.
Complex parameter types are discouraged - a parameter type should be simple types as often as possible. If complex types are used, they should be shallow and easily creatable from a constructor or another cmdlet.
Cmdlet parameter sets should be mutually exclusive - each parameter set must have at least one mandatory parameter not in other parameter sets.

This also includes: 1. Protecting against breaking SDK changes 2. Re-run of tests 3. Fixing bugs in cmdlets 3a. Remove the ability to try and set default data lake store account. This has never been supported for customers. 3b. Remove the ability to try and set unsupported fields in Set-Accounts (which has never been supported) 3c. Remove "return" value for NewSecret, since it has never returned anything. 3d. Remove ability to try and set encryption in an existing account (has never been supported) 4. Add support for new properties in ADLA accounts (parallelism and retention policies)

cormacpayne · 2016-12-07T20:25:53Z

...taLakeAnalytics/Commands.DataLakeAnalytics/Commands/AddAzureRmDataLakeAnalyticsDataSource.cs

-                "Optionally indicates that this should now be the default storage account for the DataLakeAnalytics account."
-            )]
-        [ValidateNotNullOrEmpty]
-        public SwitchParameter Default { get; set; }


@begoldsm why are you removing this parameter? This will result in a breaking change

Because it was a bug. If any customer tried to pass this parameter the cmdlet would fail (always) since this has never been supported. It was a test gap miss that was corrected recently during a test debt payment.

cormacpayne · 2016-12-07T20:28:38Z

.../DataLakeAnalytics/Commands.DataLakeAnalytics/Commands/NewAzureRmDataLakeAnalyticsAccount.cs

@@ -54,6 +54,24 @@ public class NewAzureDataLakeAnalyticsAccount : DataLakeAnalyticsCmdletBase
        [ValidateNotNull]
        public Hashtable Tags { get; set; }

+        [Parameter(ValueFromPipelineByPropertyName = true, Position = 5, Mandatory = false,


@begoldsm more than four positional parameters for a cmdlet is too many. Please get ready of the position for the parameters you are adding to this cmdlet

cormacpayne · 2016-12-07T20:29:18Z

...akeAnalytics/Commands.DataLakeAnalytics/Commands/NewAzureRmDataLakeAnalyticsCatalogSecret.cs

@@ -72,8 +72,8 @@ public override void ExecuteCmdlet()

            var toUse = Uri ?? new Uri(string.Format("https://{0}:{1}", DatabaseHost, Port));

-            WriteObject(DataLakeAnalyticsClient.CreateSecret(Account, DatabaseName, Secret.UserName,
-                Secret.GetNetworkCredential().Password, toUse.AbsoluteUri));
+            DataLakeAnalyticsClient.CreateSecret(Account, DatabaseName, Secret.UserName,


@begoldsm this cmdlet won't return anything anymore?

It never returned anything. The object would be null/empty since the service has never returned anything. This was detected during the time spent adding more robust test support across our service.

cormacpayne · 2016-12-07T20:30:43Z

.../DataLakeAnalytics/Commands.DataLakeAnalytics/Commands/SetAzureRmDataLakeAnalyticsAccount.cs

@@ -30,35 +30,38 @@ public class SetAzureDataLakeAnalyticsAccount : DataLakeAnalyticsCmdletBase

        [Parameter(ValueFromPipelineByPropertyName = true, Position = 1, Mandatory = false,
            HelpMessage =
-                "Name of the new Data Lake account to set as the default storage for this DataLakeAnalytics account.")]
-        [ValidateNotNullOrEmpty]
-        public string DefaultDataLakeStore { get; set; }


@begoldsm why is this parameter being remove? This is a breaking change

Was never supported. Same comment as above. We were lucky no one tried to use this option, since it would have always resulted in an exception from the server saying "changes to default store are not allowed"

cormacpayne · 2016-12-07T20:31:03Z

.../DataLakeAnalytics/Commands.DataLakeAnalytics/Commands/SetAzureRmDataLakeAnalyticsAccount.cs

                "A string,string dictionary of tags associated with this account that should replace the current set of tags"
            )]
        [ValidateNotNull]
        public Hashtable Tags { get; set; }

+        [Parameter(ValueFromPipelineByPropertyName = true, Position = 2, Mandatory = false,


@begoldsm same comment about too many positional parameters

cormacpayne · 2016-12-07T20:32:11Z

...taLakeAnalytics/Commands.DataLakeAnalytics/Commands/SetAzureRmDataLakeAnalyticsDataSource.cs

-            ParameterSetName = DataLakeParameterSetName,
-            HelpMessage = "The name of the Data Lake Store to add to the account.")]
-        [ValidateNotNullOrEmpty]
-        public string DataLakeStore { get; set; }


@begoldsm same breaking change comment

Same comment as before. Since you can't set anything for a DataLakeStore data source (as of today) I have optimized this command for only Azure blob. We have 100% confidence that no customers have been trying to use it this way, since the command would always fail.

cormacpayne · 2016-12-07T20:33:57Z

...taLakeAnalytics/Commands.DataLakeAnalytics/Commands/SetAzureRmDataLakeAnalyticsDataSource.cs

        [ValidateNotNullOrEmpty]
        [Alias("AzureBlob")]
        public string Blob { get; set; }

        [Parameter(ValueFromPipelineByPropertyName = true, Position = 2, Mandatory = true,
-            ParameterSetName = BlobParameterSetName,


@begoldsm removing the parameter sets from this cmdlet causes breaking changes. For this parameter, if a user has a script that was using the parameter set DataLakeParameterSet, it will no longer work because AccessKey is now a required parameter in every call of this cmdlet

Back to my previous comments. Any customer who did that had a broken script anyway, since the command would throw an exception deterministically saying "changing default data lake store is not supported"

cormacpayne · 2016-12-07T20:34:28Z

...taLakeAnalytics/Commands.DataLakeAnalytics/Commands/SetAzureRmDataLakeAnalyticsDataSource.cs

-                "Optionally indicates that this should now be the default storage account for the DataLakeAnalytics account."
-            )]
-        [ValidateNotNullOrEmpty]
-        public SwitchParameter Default { get; set; }


@begoldsm same breaking change comment

Same response :)

cormacpayne · 2016-12-07T20:37:43Z

...ourceManager/DataLakeStore/Commands.DataLakeStore/Commands/SetAzureRmDataLakeStoreAccount.cs

-        [Parameter(ValueFromPipelineByPropertyName = true, Position = 6, Mandatory = false,
-            HelpMessage = "If the encryption type is User assigned, this is the key version of the key the user wishes to use")]
-        [ValidateNotNull]
-        public string KeyVersion { get; set; }


@begoldsm why are you removing four parameters in this cmdlet?

Same issue as the others. When adding tests, I discovered that this wasn't actually supported and will always fail. Removing it protects both us and our customers from assuming they can do things that are not supported. Unfortunately we should have caught this previously, but we did not have the bandwidth (or a good way) to test it until this cycle, where I have found the bugs before customers and am proactively fixing them.

And fix the conversion to utilize UTC.

markcowl · 2016-12-08T07:29:11Z

@begoldsm @cormacpayne as discussed api changes that would previously have resulted in errors are not breaking changes for our purposes, so this should be ok.

on demand run here: http://azuresdkci.cloudapp.net/view/1-AzurePowerShell/job/powershell-demand/1336/

begoldsm added 6 commits December 5, 2016 11:50

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

9ca9464

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

e8186fb

Updates to include set-expiry.

1ed6096

Update help.xml

e80f80d

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

d254419

azurecla added the cla-not-required label Dec 7, 2016

cormacpayne suggested changes Dec 7, 2016

View reviewed changes

begoldsm added 3 commits December 7, 2016 15:15

Address parameter position PR comment.

09b1f32

re-record filesystem tests.

dbbcf92

fix tests to be UTC instead of local

d0d1e49

And fix the conversion to utilize UTC.

cormacpayne approved these changes Dec 8, 2016

View reviewed changes

cormacpayne merged commit 772113c into Azure:dev Dec 8, 2016

ADL Updates to use latest GA packages. #3261

ADL Updates to use latest GA packages. #3261

Uh oh!

Conversation

begoldsm commented Dec 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

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!

markcowl commented Dec 8, 2016

Uh oh!

Uh oh!

begoldsm commented Dec 7, 2016 •

edited

Loading