Apply azure container instance SDK 2017-10-01 #5020

yolocs · 2017-11-22T21:54:32Z

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.

cormacpayne · 2017-11-27T17:17:08Z

@azuresdkci test this please

cormacpayne · 2017-11-27T17:17:21Z

@yolo3301 would you mind resolving the merge conflicts?

cormacpayne

@yolo3301 a few minor comments. We should have a quick cmdlet design review via email discussing the cmdlet changes that you are making in this PR. The cmdlet review template can be found here.

cormacpayne · 2017-11-27T17:17:55Z

src/ResourceManager/ContainerInstance/AzureRM.ContainerInstance.psd1

@@ -12,7 +12,7 @@
 # RootModule = ''

 # Version number of this module.
-ModuleVersion = '0.1.0'
+ModuleVersion = '0.1.1'


@yolo3301 you can revert any change made to versions in this PR. We will handle the versioning before publishing the module

cormacpayne · 2017-11-27T17:26:06Z

src/ResourceManager/ContainerInstance/ChangeLog.md

@@ -19,6 +19,12 @@
 -->
 ## Current Release

+## Version 0.1.1


@yolo3301 please remove the ## Version 0.1.1 header and move the below content to be under ## Current Release

cormacpayne · 2017-11-27T17:26:19Z

...ResourceManager/ContainerInstance/Commands.ContainerInstance.Test/Properties/AssemblyInfo.cs

@@ -32,5 +32,5 @@
 // You can specify all the values or you can default the Build and Revision Numbers
 // by using the '*' as shown below:
 // [assembly: AssemblyVersion("1.0.*")]
-[assembly: AssemblyVersion("0.1.0")]
-[assembly: AssemblyFileVersion("0.1.0")]
+[assembly: AssemblyVersion("0.1.1")]


@yolo3301 please revert the changes made in this file

cormacpayne · 2017-11-27T17:27:48Z

...nager/ContainerInstance/Commands.ContainerInstance/Commands/NewAzureContainerGroupCommand.cs

        [ValidateNotNullOrEmpty]
-        public int? Port { get; set; }
+        public int[] Ports { get; set; }


@yolo3301 why was this change made?

We support multiple ports. In Azure CLI we have already made a similar change.

@yolo3301 from PowerShell's Strongly Encouraged Development Guidelines (the snippet can be found here), parameter names should be singular unless there is validation for the parameter preventing the user from passing only one object. If a user can pass just one port, then this parameter should be public int[] Port

Good to know. Updated.

cormacpayne · 2017-11-27T17:28:20Z

src/ResourceManager/ContainerInstance/Commands.ContainerInstance/Properties/AssemblyInfo.cs

@@ -32,5 +32,5 @@
 // You can specify all the values or you can default the Build and Revision Numbers
 // by using the '*' as shown below:
 // [assembly: AssemblyVersion("1.0.*")]
-[assembly: AssemblyVersion("0.1.0")]


@yolo3301 please revert the changes made in this file

cormacpayne · 2017-11-27T17:29:15Z

...sourceManager/ContainerInstance/Commands.ContainerInstance/help/New-AzureRmContainerGroup.md

 ## PARAMETERS

+### -AzureFileVolumeAccountKey
+The storage account key of the Azure File share to mount.```yaml


@yolo3301 please make sure that the trailing ```yaml string is moved to a line by itself. This applies to all markdown files that have been edited in this file. This is caused from a bug in the platyPS module.

cormacpayne · 2017-11-27T17:29:33Z

tools/AzureRM/AzureRM.psd1

@@ -65,7 +65,7 @@ RequiredModules = @(@{ModuleName = 'AzureRM.Profile'; RequiredVersion = '4.0.0';
               @{ModuleName = 'AzureRM.CognitiveServices'; RequiredVersion = '0.9.0'; }, 
               @{ModuleName = 'AzureRM.Compute'; RequiredVersion = '4.0.1'; }, 
               @{ModuleName = 'AzureRM.Consumption'; RequiredVersion = '0.3.0'; }, 
-               @{ModuleName = 'AzureRM.ContainerInstance'; RequiredVersion = '0.1.0'; }, 
+               @{ModuleName = 'AzureRM.ContainerInstance'; RequiredVersion = '0.1.1'; },


@yolo3301 please revert the change made in this file

yolocs · 2017-11-27T20:06:39Z

@cormacpayne Created a simple review page

maddieclayton · 2017-11-27T22:35:57Z

Page for cmdlet review can now be found here: https://github.com/Azure/azure-powershell-pr/wiki/Apply-azure-container-instance-SDK-2017-10-01

cormacpayne · 2017-11-29T16:43:47Z

@azuresdkci test this please

maddieclayton

Some initial comments

maddieclayton · 2017-11-30T00:07:44Z

...r/ContainerInstance/Commands.ContainerInstance.Test/ScenarioTests/ContainerInstanceTests.ps1

@@ -23,17 +23,22 @@ function Test-AzureRmContainerGroup
    $location = Get-ProviderLocation "Microsoft.ContainerInstance/ContainerGroups"
    $image = "nginx"
    $osType = "Linux"


Please add a test for the new parameter set

Will the session recording expose storage account key used for volume?

Yes - Can you create a test storage account and delete it at the end of the test? This should prevent any security issues caused by exposing the account key.

maddieclayton · 2017-11-30T00:19:24Z

...nager/ContainerInstance/Commands.ContainerInstance/Commands/NewAzureContainerGroupCommand.cs

@@ -46,6 +47,12 @@ public class NewAzureContainerGroupCommand : ContainerInstanceCmdletBase
            ParameterSetName = CreateContainerGroupWithRegistryParamSet,
            ValueFromPipelineByPropertyName = true,
            HelpMessage = "The resource group name.")]
+        [Parameter(


If a parameter is in all sets, and Mandatory and in the same position, you don't need to have a separate attribute for each parameter set. Please erase all parameter attributes and put in:

[Parameter( Mandatory = true, Position = 0, ValueFromPipelineByPropertyName = true, HelpMessage = "The resource group name.")]

Apply this all other parameters also. Especially if you plan to add more parameter sets in the future, there is no need to create a new attribute each time.

maddieclayton · 2017-11-30T00:21:34Z

...nager/ContainerInstance/Commands.ContainerInstance/Commands/NewAzureContainerGroupCommand.cs

@@ -149,6 +177,34 @@ public class NewAzureContainerGroupCommand : ContainerInstanceCmdletBase
        [ValidateNotNullOrEmpty]
        public PSCredential RegistryCredential { get; set; }

+        [Parameter(


Quick recommendation: move these parameters above all of the non-mandatory parameters so that the users will see them more quickly when scanning help.

But these are only mandatory for this parameter set. Should I still do the same?

Yes, they are not in the other parameter sets, so it won't affect those other parameter sets.

maddieclayton · 2017-11-30T00:25:34Z

...nager/ContainerInstance/Commands.ContainerInstance/Commands/NewAzureContainerGroupCommand.cs

+            ParameterSetName = CreateContainerGroupWithAzureFileVolumeParamSet,
+            HelpMessage = "The storage account key of the Azure File share to mount.")]
+        [ValidateNotNullOrEmpty]
+        public PSCredential AzureFileVolumeAccountKey { get; set; }


Is account key the correct name for this parameter? I wouldn't usually consider a PSCredential to be a "key". Please correct me if I am wrong.

That's what I would consider a PSCredential to be - it requires both a username and password, so "key" seems like the wrong name.

maddieclayton · 2017-11-30T00:33:04Z

src/ResourceManager/ContainerInstance/Commands.ContainerInstance/Models/PSContainer.cs

@@ -62,7 +62,12 @@ public class PSContainer
        /// <summary>
        /// Gets or sets the events.
        /// </summary>
-        public IList<PSContainerEvent> Events { get; set; }


Is the change from PSContainerEvent to PSEvent an unrelated change? Just want to make sure I understand the scope of the changes.

Previously an event belongs to a container. Now an event can also belong to the container group. Simply keep up with sdk.

maddieclayton · 2017-11-30T00:35:33Z

src/ResourceManager/ContainerInstance/Commands.ContainerInstance/Models/PSContainerGroup.cs

+        /// <summary>
+        /// Gets or sets the state of the container group.
+        /// </summary>
+        public string State { get; set; }


Same here. Is this just to keep up to date with the current SDK?

maddieclayton · 2017-11-30T00:37:13Z

...sourceManager/ContainerInstance/Commands.ContainerInstance/help/New-AzureRmContainerGroup.md

 ```

 This commands creates a container group using a custom image from a custom container image registry.

+### Example 6: Creates a container group that mounts Azure File volume


Awesome examples! Thanks for this.

maddieclayton

Two more small things.

maddieclayton · 2017-12-01T00:06:22Z

...ger/ContainerInstance/Commands.ContainerInstance.Test/Commands.ContainerInstance.Test.csproj

-      <CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
-    </None>
-    <None Include="SessionRecords\Microsoft.Azure.Commands.ContainerInstance.Test.ScenarioTests.ContainerInstanceTests\TestContainerInstanceLog.json">
+    <None Include="SessionRecords\Microsoft.Azure.Commands.ContainerInstance.Test.ScenarioTests.ContainerInstanceTests\*">


Can you add these tests separately? We generally like to stay away from using *

Sure. Just curious why?

maddieclayton · 2017-12-01T00:15:40Z

...nager/ContainerInstance/Commands.ContainerInstance/Commands/NewAzureContainerGroupCommand.cs

+            Position = 2,
+            HelpMessage = "The container image.")]
+        [ValidateNotNullOrEmpty]
+        public string Image { get; set; }


Image previously was not a named parameter - was this addition intentional? If so, that is okay, I just want to ensure that the change was intended.

maddieclayton · 2017-12-02T00:57:25Z

@azuresdkci test this please

maddieclayton · 2017-12-02T00:58:04Z

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

azuresdkci added the needs-review label Nov 22, 2017

azuresdkci assigned cormacpayne Nov 22, 2017

cormacpayne suggested changes Nov 27, 2017

View reviewed changes

cormacpayne added Cmdlet Review Required ⚠️ needs-revision and removed needs-review labels Nov 27, 2017

cormacpayne assigned yolocs and unassigned cormacpayne Nov 27, 2017

yolocs added 4 commits November 27, 2017 11:52

applied new sdk and updated test cases

d775d65

updated help

a90e15b

update version

7392183

revert version and update help

e8ef048

maddieclayton assigned cormacpayne and unassigned yolocs Nov 27, 2017

maddieclayton added needs-review and removed needs-revision labels Nov 27, 2017

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

cormacpayne assigned yolocs and unassigned cormacpayne Nov 28, 2017

use singular for port

026c016

maddieclayton suggested changes Nov 30, 2017

View reviewed changes

maddieclayton added Cmdlet Review Complete and removed Cmdlet Review Required ⚠️ labels Nov 30, 2017

adjust cmdlet parameters and update test

11c2ed8

Merge branch 'preview' into aci_api_20171001

2e0ca40

maddieclayton reviewed Dec 1, 2017

View reviewed changes

maddieclayton assigned cormacpayne and unassigned yolocs Dec 1, 2017

maddieclayton added needs-review and removed needs-revision labels Dec 1, 2017

maddieclayton assigned markcowl and unassigned cormacpayne Dec 1, 2017

add records separately

f9cc56d

cormacpayne approved these changes Dec 2, 2017

View reviewed changes

maddieclayton approved these changes Dec 2, 2017

View reviewed changes

maddieclayton merged commit 21dc0cc into Azure:preview Dec 2, 2017

Apply azure container instance SDK 2017-10-01 #5020

Apply azure container instance SDK 2017-10-01 #5020

Uh oh!

Conversation

yolocs commented Nov 22, 2017 • edited by azuresdkci Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

cormacpayne commented Nov 27, 2017

Uh oh!

cormacpayne commented Nov 27, 2017

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!

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!

yolocs commented Nov 27, 2017

Uh oh!

maddieclayton commented Nov 27, 2017

Uh oh!

cormacpayne commented Nov 29, 2017

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!

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

yolocs commented Nov 22, 2017 •

edited by azuresdkci

Loading

maddieclayton commented Dec 2, 2017 •

edited

Loading