Update Batch commandlets to support latest APIs #4762

matthchr · 2017-10-10T20:34:04Z

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 PassThru 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.

matthchr · 2017-10-12T23:00:49Z

@markcowl @cormacpayne The CI is failing because of this:
D:\workspace\powershell\src\Package\StaticAnalysis.exe D:\workspace\powershell\src\Package\Debug D:\workspace\powershell\src\Package true true failing, which looks to be failing because we made breaking changes.

But we made breaking changes on purpose... so I think we should ignore this failure?

cormacpayne · 2017-10-12T23:28:38Z

@matthchr Hey Matt, you can ignore that failure since this is a breaking change release and you are making the changes that customers have been previously notified of. You can go ahead and suppress these changes so they don't fail the build - to do so, you will open the BreakingChangeIssues.csv file from the Build Artifacts of the job (the csv is located in src/Package) and copy the contents of the file (you will need to do this in a text editor - make sure to ignore copying the header). Then you will open the BreakingChangeIssues.csv file in tools/StaticAnalysis/Exceptions and paste the contents you copied to the end (also using a text editor).

Let me know if you have any questions.

xingwu1 · 2017-10-16T18:03:07Z

src/ResourceManager/AzureBatch/Commands.Batch/BatchAccounts/NewBatchAccountCommand.cs

@@ -40,13 +42,30 @@ public class NewBatchAccountCommand : BatchCmdletBase
        [Parameter(Position = 3, Mandatory = false, ValueFromPipelineByPropertyName = true)]
        public string AutoStorageAccountId { get; set; }

+        [Parameter(Position = 4, Mandatory = false, ValueFromPipelineByPropertyName = true)]


If we don't set position, will it reduce the breaking change for future?

I am not sure -- I followed the existing pattern in this cmdlet. @cormacpayne @markcowl can you comment on this?

Think of this like unnamed parameters in a method signature - any positional parameter beyiond 3 tends to be not useful. We suggest that you not have positional parameter with position > 3

Got it. I will fix this

xingwu1 · 2017-10-16T20:00:03Z

src/ResourceManager/AzureBatch/Commands.Batch/Models.Generated/PSCloudPool.cs

@@ -233,11 +272,19 @@ public PSCloudServiceConfiguration CloudServiceConfiguration
            }
        }

-        public System.Int32? CurrentDedicated
+        public System.Int32? CurrentDedicatedComputeNodes


Add mapping to CurrentDedicated and obsolete it.

My intent is to just leave this as is (no obsolete) since this is a breaking change, unless otherwise suggested by one of the powershell maintainers.

As long as this is documented in the upcoming breaking changes document, this is ok.

xingwu1 · 2017-10-16T20:00:26Z

src/ResourceManager/AzureBatch/Commands.Batch/Models.Generated/PSCloudPool.cs

@@ -445,15 +501,27 @@ public PSPoolStatistics Statistics
            }
        }

-        public System.Int32? TargetDedicated
+        public System.Int32? TargetDedicatedComputeNodes


My intent is to just leave this as is (no obsolete) since this is a breaking change, unless otherwise suggested by one of the powershell maintainers.

This is ok, but it needs to be documented in the upcoming breaking changes document,a s noted.

Added documentation there

xingwu1 · 2017-10-16T20:02:00Z

src/ResourceManager/AzureBatch/Commands.Batch/Models.Generated/PSExitConditions.cs

@@ -151,28 +153,53 @@ public IList<PSExitCodeMapping> ExitCodes
            }
        }

-        public PSExitOptions SchedulingError
+        public PSExitOptions FileUploadError


mapping and obsolete?

My intent is to just leave this as is (no obsolete) since this is a breaking change, unless otherwise suggested by one of the powershell maintainers.

xingwu1 · 2017-10-16T20:04:00Z

src/ResourceManager/AzureBatch/Commands.Batch/Models.Generated/PSJobSchedulingError.cs

@@ -45,7 +45,7 @@ internal PSJobSchedulingError(Microsoft.Azure.Batch.JobSchedulingError omObject)
            this.omObject = omObject;
        }

-        public Microsoft.Azure.Batch.Common.SchedulingErrorCategory Category
+        public Microsoft.Azure.Batch.Common.ErrorCategory Category


Does it count "breaking change"?

I am not sure -- one of the Powershell team members can comment maybe?

If it has a superset of the properties of the previous type, then it is not a breaking change. Otherwise, it is a breakign change.

Yes, it has a superset.

xingwu1 · 2017-10-16T20:06:18Z

src/ResourceManager/AzureBatch/Commands.Batch/Models.Generated/PSNodeFile.cs

@@ -53,11 +53,11 @@ internal PSNodeFile(Microsoft.Azure.Batch.NodeFile omObject)
            }
        }

-        public string Name
+        public string Path


My intent is to just leave this as is (no obsolete) since this is a breaking change, unless otherwise suggested by one of the powershell maintainers.

This needs to be specifically documented (with an example) in the upcoming-breaking changes

xingwu1 · 2017-10-16T20:06:52Z

src/ResourceManager/AzureBatch/Commands.Batch/Models.Generated/PSPoolSpecification.cs

@@ -328,15 +367,27 @@ public PSStartTask StartTask
            }
        }

-        public System.Int32? TargetDedicated
+        public System.Int32? TargetDedicatedComputeNodes


My intent is to just leave this as is (no obsolete) since this is a breaking change, unless otherwise suggested by one of the powershell maintainers.

xingwu1 · 2017-10-16T20:08:42Z

src/ResourceManager/AzureBatch/Commands.Batch/Models.Generated/PSVirtualMachineConfiguration.cs

        private PSWindowsConfiguration windowsConfiguration;

-        public PSVirtualMachineConfiguration(PSImageReference imageReference, string nodeAgentSkuId, PSWindowsConfiguration windowsConfiguration = default(PSWindowsConfiguration))
+        public PSVirtualMachineConfiguration(PSImageReference imageReference, string nodeAgentSkuId)


since the original constructor isn't obsoleted, should we keep it?

In this case I don't think so since this is a breaking change release, we should feel free to make this sort of change (even though I missed obsoleting it previously).

xingwu1 · 2017-10-16T20:11:22Z

src/ResourceManager/AzureBatch/Commands.Batch/Models/ListNodeFileOptions.cs

        /// </summary>
-        public string NodeFileName { get; set; }
+        public string Path { get; set; }


should we keep the original property since it isn't obsoleted?

My intent is to just leave this as is (no obsolete) since this is a breaking change, unless otherwise suggested by one of the powershell maintainers.

The change in usage needs to be documented in upcoming-breaking-changes

This is actually only a type used internally in the cmdlets, it's not part of a cmdlet or return type. It's just used to store information about what request is being performed so we can pass it off to the C# SDK, so I don't think there's anything to really say in upcoming-breaking-changes. It was already covered in the Name -> Path change for the GetNodeFile cmdlet.

xingwu1 · 2017-10-16T20:11:48Z

src/ResourceManager/AzureBatch/Commands.Batch/Models/NewPoolParameters.cs

@@ -65,7 +65,12 @@ public NewPoolParameters(BatchAccountContext context, string poolId, IEnumerable
        /// <summary>
        /// The target number of compute nodes to allocate to the pool.
        /// </summary>
-        public int? TargetDedicated { get; set; }
+        public int? TargetDedicatedComputeNodes { get; set; }


My intent is to just leave this as is (no obsolete) since this is a breaking change, unless otherwise suggested by one of the powershell maintainers.

This is actually only a type used internally in the cmdlets, it's not part of a cmdlet or return type. It's just used to store information about what request is being performed so we can pass it off to the C# SDK, so I don't think there's anything to really say in upcoming-breaking-changes.

xingwu1 · 2017-10-16T20:12:22Z

src/ResourceManager/AzureBatch/Commands.Batch/Models/NodeFileOperationParameters.cs

        /// </summary>
-        public string NodeFileName { get; private set; }
+        public string Path { get; private set; }


My intent is to just leave this as is (no obsolete) since this is a breaking change, unless otherwise suggested by one of the powershell maintainers.

Same comment, must document in upcoming-breaking changes

This is actually only a type used internally in the cmdlets, it's not part of a cmdlet or return type. It's just used to store information about what request is being performed so we can pass it off to the C# SDK, so I don't think there's anything to really say in upcoming-breaking-changes. It was already covered in the Name -> Path change for the GetNodeFile/RemoveBatchNodeFile cmdlets, which was already documented.

cormacpayne · 2017-10-17T16:50:54Z

@azuresdkci test this please

matthchr · 2017-11-01T02:44:13Z

@markcowl - Done, commits should be much cleaner now

markcowl · 2017-11-01T18:43:52Z

src/ResourceManager/AzureBatch/documentation/upcoming-breaking-changes.md

-  no longer have a `SchedulingError` property, they instead have a `FailureInformation` property. `FailureInformation` is returned any time there is a task failure.
-  This includes all previous scheduling error cases, as well as nonzero task exit codes, and file upload failures from the new output files feature.
+# Upcoming Breaking Changes
+


The changes in the output objects are not covered here. Please include a section on how the corresponding output object changes (*Nodes -> *ComputeNodes) would be handled by scripts that use the old proeprty values

markcowl · 2017-11-01T18:45:49Z

src/ResourceManager/AzureBatch/Commands.Batch/Models.Generated/PSCloudPool.cs

@@ -445,15 +501,27 @@ public PSPoolStatistics Statistics
            }
        }

-        public System.Int32? TargetDedicated
+        public System.Int32? TargetDedicatedComputeNodes


This is ok, but it needs to be documented in the upcoming breaking changes document,a s noted.

markcowl · 2017-11-01T18:47:59Z

src/ResourceManager/AzureBatch/Commands.Batch/packages.config

-  <package id="WindowsAzure.Storage" version="6.2.0" targetFramework="net452" />
+  <package id="Microsoft.Azure.Management.Batch" version="4.2.0" targetFramework="net452" />
+  <package id="Microsoft.Azure.Management.Resources" version="2.20.0-preview" targetFramework="net45" />
+  <package id="Microsoft.Bcl" version="1.1.9" targetFramework="net45" />


The common packages are all being resrtored by the resource manager common package. We intentionally removed this to prevent different packages taking dependencies on different versions of common libraries. Please revart, other than the changes to batch sdk versions

Ah, will fix -- I think I used nuget to update the package dependencies and it probably added them back.

markcowl · 2017-11-01T18:50:19Z

src/ResourceManager/AzureBatch/documentation/upcoming-breaking-changes.md

+- Renamed `ResizeError` to `ResizeErrors` on `PSCloudPool`, and it is now a collection.
+- Renamed the `Name` property on `PSNodeFile` to `Path`.
+- Renamed `PSTaskSchedulingError` to `PSTaskFailureInformation`.
+- Renamed the `SchedulingError` property on `PSJobPreparationTaskExecutionInformation`, `PSJobReleaseTaskExecutionInformation`, `PSStartTaskInformation`, `PSSubtaskInformation`, and `PSTaskExecutionInformation` to `FailureInformation`.


I don't see any of the changes to secure parameter types here. Please ensure that these are documented

They're in the section below on per-cmdlet changes

markcowl · 2017-11-01T18:51:51Z

src/ResourceManager/AzureBatch/documentation/upcoming-breaking-changes.md

 - `PSMultiInstanceSettings` constructor no longer takes a required `numberOfInstances` parameter, instead it takes a required `coordinationCommandLine` parameter.

 The following cmdlets were affected this release:

 **New-AzureBatchCertificate**
- Parameter "Password" being replaced in favor of a Secure string


Please add explnantions of the change form 'runelevated' to 'UserIdentity'

Done -- I think I've covered this now both as a parameter to exisiting cmdlets as well as on the returned object from Get-AzureBatchTask

markcowl · 2017-11-01T18:55:58Z

src/ResourceManager/AzureBatch/Commands.Batch/Models.Generated/PSTaskFailureInformation.cs

@@ -27,17 +27,16 @@ namespace Microsoft.Azure.Commands.Batch.Models
    using System.Collections;
    using System.Collections.Generic;
    using Microsoft.Azure.Batch;
-
-
-    [Obsolete("SchedulingError will be removed in a future version and replaced with FailureInformation")]


Need an example of usage of the new type and how this has changed in the upcoming-breakign changes document

The type itself is used the same way (it's structured the same, just renamed). I will clarify in upcoming-breaking-changes though.

markcowl · 2017-11-01T18:57:01Z

src/ResourceManager/AzureBatch/Commands.Batch/Models/ListNodeFileOptions.cs

        /// </summary>
-        public string NodeFileName { get; set; }
+        public string Path { get; set; }


The change in usage needs to be documented in upcoming-breaking-changes

markcowl · 2017-11-01T18:57:24Z

src/ResourceManager/AzureBatch/Commands.Batch/Models/NodeFileOperationParameters.cs

        /// </summary>
-        public string NodeFileName { get; private set; }
+        public string Path { get; private set; }


Same comment, must document in upcoming-breaking changes

markcowl · 2017-11-01T18:57:54Z

src/ResourceManager/AzureBatch/Commands.Batch/Pools/StartBatchPoolResizeCommand.cs

        [ValidateNotNullOrEmpty]
-        public int TargetDedicated { get; set; }


needs toi be documented for the migration guide

markcowl · 2017-11-01T18:59:17Z

src/ResourceManager/AzureBatch/documentation/upcoming-breaking-changes.md

+- Renamed `PSTaskSchedulingError` to `PSTaskFailureInformation`.
+- Renamed the `SchedulingError` property on `PSJobPreparationTaskExecutionInformation`, `PSJobReleaseTaskExecutionInformation`, `PSStartTaskInformation`, `PSSubtaskInformation`, and `PSTaskExecutionInformation` to `FailureInformation`.
+  - `FailureInformation` is returned any time there is a task failure. This includes all previous scheduling error cases, as well as nonzero task exit codes, and file upload failures from the new output files feature.
+- Renamed the `SchedulingError` property on `PSExitConditions` to `PreProcessingError`.


Have noted some places where additional documentation appears to be necessary. Essentially any breaking change in parameter or output type properties need to be documented here with an example. This will be sued to create the migration guide.

matthchr · 2017-11-01T22:49:26Z

@markcowl - Updated according to your feedback. Breaking change doc clarified, moved to all per-cmdlet style.

Please let me know ASAP if there's anything else we need to do, or if we're good to merge.

cormacpayne · 2017-11-02T05:48:42Z

On-demand: https://azuresdkci.westus2.cloudapp.azure.com/job/powershell-demand/118/

markcowl · 2017-11-02T17:33:31Z

@matthchr the on-demand run is failing due to the plural parameter names - you will need to add suppressions to the SignatureIssues.xml file in toold\StaticAnalysis to match.

matthchr · 2017-11-02T17:54:54Z

@markcowl - There is no such file in tools\StaticAnalysis

Where should I add the suppressions, and how do I know what the format of the supressions is...?
edit: Mark helped me find the supressions log (it's .csv not .xml)

- Add support for Azure Batch SDK 8.0. - Includes one new cmdlet (Get-AzureBatchTaskCounts) and a number of cmdlet updates. - Re-record tests. - Regenerate help text. - Use secure string for password fields.

markcowl · 2017-11-02T20:48:05Z

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

markcowl · 2017-11-02T22:34:58Z

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

matthchr requested review from darylmsft and xingwu1 October 10, 2017 20:34

azuresdkci added the needs-review label Oct 10, 2017

azuresdkci assigned cormacpayne Oct 10, 2017

matthchr force-pushed the feature/batch-psh-vnext branch 3 times, most recently from f7179d9 to 7438ea7 Compare October 11, 2017 02:59

cormacpayne added needs-revision and removed needs-review labels Oct 13, 2017

cormacpayne assigned matthchr and unassigned cormacpayne Oct 13, 2017

matthchr force-pushed the feature/batch-psh-vnext branch from 31d8a45 to 0bebb1f Compare October 13, 2017 19:53

markcowl added the Cmdlet Review Required ⚠️ label Oct 16, 2017

xingwu1 reviewed Oct 16, 2017

View reviewed changes

xingwu1 previously approved these changes Oct 16, 2017

View reviewed changes

matthchr dismissed xingwu1’s stale review via 847c5d3 October 16, 2017 22:02

matthchr force-pushed the feature/batch-psh-vnext branch from 847c5d3 to 2b49aed Compare October 16, 2017 22:04

matthchr force-pushed the feature/batch-psh-vnext branch from 666e8a6 to e06645c Compare November 1, 2017 02:44

cormacpayne added needs-review and removed needs-revision labels Nov 1, 2017

cormacpayne assigned markcowl and unassigned matthchr Nov 1, 2017

markcowl requested changes Nov 1, 2017

View reviewed changes

markcowl added needs-revision and removed needs-review labels Nov 1, 2017

matthchr force-pushed the feature/batch-psh-vnext branch 2 times, most recently from 8f8ea9f to 5ea9226 Compare November 1, 2017 22:46

markcowl previously approved these changes Nov 2, 2017

View reviewed changes

markcowl added Waiting for On Demand and removed needs-revision labels Nov 2, 2017

markcowl dismissed their stale review via 54cc3a9 November 2, 2017 16:03

matthchr and others added 4 commits November 2, 2017 10:51

Regenerate docs

3e1edba

Add support for new Batch library features

68106da

Add Azure Batch AAD support

25ac615

Update Batch management plane with latest features

bbd4e65

Regenerate and support latest Azure Batch SDK

64f67d8

- Add support for Azure Batch SDK 8.0. - Includes one new cmdlet (Get-AzureBatchTaskCounts) and a number of cmdlet updates. - Re-record tests. - Regenerate help text. - Use secure string for password fields.

matthchr force-pushed the feature/batch-psh-vnext branch from 54cc3a9 to 64f67d8 Compare November 2, 2017 18:00

markcowl approved these changes Nov 2, 2017

View reviewed changes

markcowl changed the base branch from preview to release-5.0.0 November 2, 2017 23:24

markcowl merged commit 3477556 into Azure:release-5.0.0 Nov 3, 2017

		[ValidateNotNullOrEmpty]
		public int TargetDedicated { get; set; }

Update Batch commandlets to support latest APIs #4762

Update Batch commandlets to support latest APIs #4762

Uh oh!

Conversation

matthchr commented Oct 10, 2017 • edited by azuresdkci Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

matthchr commented Oct 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cormacpayne commented Oct 12, 2017

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!

matthchr Oct 26, 2017 • 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!

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!

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

matthchr commented Oct 10, 2017 •

edited by azuresdkci

Loading

matthchr commented Oct 12, 2017 •

edited

Loading

matthchr Oct 26, 2017 •

edited

Loading