AzureRmKubernetes cmdlets for managed Kubernetes #5087

devigned · 2017-12-06T02:47:06Z

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.

twitchax · 2017-12-07T10:12:44Z

Restarted the travis build: no idea why it died.

twitchax · 2017-12-07T10:22:10Z

Looks like there are some errors with api version namespaces?

maddieclayton · 2017-12-19T18:40:54Z

@devigned I'll assign this back to you for the moment, please reassign it to me when you're ready for me to take a look. Thanks!

markcowl · 2017-12-20T01:05:49Z

src/ResourceManager/Kubernetes/AzureRM.Kubernetes.Netcore.psd1

+    PSData = @{
+
+        # Tags applied to this module. These help with module discovery in online galleries.
+        Tags = 'Azure','Kubernetes','ARM','Provider','AKS'


Needs the tags that indicates this runs on core (Linux, Mac) - this is the mechanism that the posh team is using to differentiate module compatible with PowerShell Core.

What tags would those be? As far as I can tell, AzureRM.Profile.Netcore.psd1 doesn't have any tags that jump out at me which scream netcore (Tags = 'Azure','ResourceManager','ARM','Profile','Authentication','Environment','Subscription').

markcowl · 2017-12-20T01:07:12Z

src/ResourceManager/Kubernetes/AzureRM.Kubernetes.psd1

+RequiredModules = @(@{ModuleName = 'AzureRM.Profile'; ModuleVersion = '4.0.0'; })
+
+# Assemblies that must be loaded prior to importing this module
+RequiredAssemblies = '.\YamlDotNet.dll'


All assemblies required by the cmdlet assembly should go here, normally this includes the associated management library.

The generated management library is included in the command module. This will need to be removed and replaced with a reference when the SDK is produced.

markcowl · 2017-12-20T01:07:29Z

src/ResourceManager/Kubernetes/AzureRM.Kubernetes.psd1

+    PSData = @{
+
+        # Tags applied to this module. These help with module discovery in online galleries.
+        Tags = 'Azure','ResourceManager','ARM','AKS', 'Kubernetes'


markcowl · 2017-12-20T01:09:01Z

src/ResourceManager/Kubernetes/Commands.Kubernetes.Test.Netcore/MergeKubeConfigTests.cs

+        }
+
+        [Fact]
+        public void MergedKubeConfigOverwriteClustersTest()


Use traits to mark these as check-in tests ([Trait(Category.AcceptanceType, Category.CheckIn)] these constants are in the common scenario test assembly

markcowl · 2017-12-20T01:10:41Z

...esourceManager/Kubernetes/Commands.Kubernetes.Test.Netcore/ScenarioTests/KubernetesTests.ps1

+    $location = Get-ProviderLocation "Microsoft.ContainerService/managedClusters"
+
+    New-AzureRmResourceGroup -Name $resourceGroupName -Location $location
+


I am assuming this will contain a CRUD test at some point.

markcowl · 2017-12-20T01:40:09Z

src/ResourceManager/Kubernetes/Commands.Kubernetes/Commands/StartAzureRmKubernetesDashboard.cs

+            _dashPod = dashPod;
+        }
+
+        public override string StatusMessage { get; }


need to implement.

markcowl · 2017-12-20T01:40:45Z

src/ResourceManager/Kubernetes/Commands.Kubernetes/Commands/StopAzureRmKubernetesDashboard.cs

+namespace Microsoft.Azure.Commands.Kubernetes
+{
+    [Cmdlet("Stop", KubeNounStr + "Dashboard")]
+    public class StopDashboard : KubeCmdletBase


I don't know that we need this. If we return a job from the first cmdlet, the user can simply stop it.

It not needed by someone that understands powershell jobs, but it does give a simple way to kill the job if you are not adept with Jobs.

markcowl · 2017-12-20T01:42:05Z

src/ResourceManager/Kubernetes/Commands.Kubernetes/load.ps1

@@ -0,0 +1,3 @@
+Import-Module -Name ./src/Package/Release/ResourceManager/AzureRM.Profile.Netcore/AzureRM.Profile.Netcore.psd1


This was just for testing. I'll drop it before the PR is final.

markcowl · 2017-12-20T01:42:31Z

src/ResourceManager/Kubernetes/Commands.Kubernetes/help/Stop-AzureRmKubernetesDashboard.md

+PS C:\> Stop-AzureRmKubernetes
+```
+
+{{ Add example description here }}


Please ensure that you fill these out in the markdown files.

markcowl · 2017-12-20T01:42:55Z

src/ResourceManager/Kubernetes/NuGet.Config

@@ -0,0 +1,6 @@
+<?xml version="1.0" encoding="utf-8"?>
+<configuration>


I think the changes below this are unintentional, right?

maddieclayton · 2017-12-22T22:53:35Z

@devigned Looks like you are failing because of some signature issues. If you check out the SignatureIssues.csv here you can see what is causing the failure. You can ignore severity 2 issues for the most part, but you should look through the sev 1 issues.

maddieclayton · 2017-12-22T22:56:26Z

src/ResourceManager/Kubernetes/Commands.Kubernetes/Commands/CreateOrUpdateKubeBase.cs

+
+namespace Microsoft.Azure.Commands.Kubernetes
+{
+    [Cmdlet(VerbsCommon.New, KubeNounStr, DefaultParameterSetName = DefaultParamSet)]


All action cmdlets should include supportsshouldprocess: more info here

maddieclayton · 2017-12-22T22:57:42Z

src/ResourceManager/Kubernetes/Commands.Kubernetes/Commands/CreateOrUpdateKubeBase.cs

+        public SwitchParameter AsJob { get; set; }
+
+        [Parameter(Mandatory = false)]
+        public Hashtable Tags { get; set; }


Rename to "Tag" (more info here)

devigned · 2018-01-09T16:14:11Z

@markcowl and @maddieclayton I the PR has had most of the boulders excavated and is ready for nits and finer review. I believe I've addressed all of the feedback thus far. I think I need to clean up some signing errors, but that should be about it. Let me know if there are any other changes you'd like me to make.

devigned · 2018-01-09T23:28:20Z

@markcowl it looks like the errors in the build are caused by a project I haven't touched. Thoughts?

maddieclayton · 2018-01-09T23:32:57Z

@azuresdkci Test this please

maddieclayton · 2018-01-09T23:33:37Z

@devigned That's a Batch test that fails intermittently, started the build again.

devigned · 2018-01-30T19:07:20Z

@markcowl I just rebased again due to another conflicting change merged.

markcowl · 2018-02-02T05:11:47Z

src/ResourceManager/Kubernetes/Commands.Kubernetes/Commands.Kubernetes.csproj

+      <HintPath>..\..\..\packages\AutoMapper.6.2.1\lib\net45\AutoMapper.dll</HintPath>
+      <Private>True</Private>
+    </Reference>
+    <Reference Include="Newtonsoft.Json, Version=6.0.0.0, Culture=neutral, PublicKeyToken=30ad4fe6b2a6aeed, processorArchitecture=MSIL">


@twitchax it is another place we have to change it if we update the common runtime to newtonsoft.json 10, as we plan to do. If this assembly has a specific dependency on newtonsoft.json, other than in its management client, then this should be removed.

markcowl · 2018-02-02T05:17:27Z

...urceManager/Kubernetes/Commands.Kubernetes/Microsoft.Azure.Commands.Kubernetes.Format.ps1xml

@@ -0,0 +1,7 @@
+<?xml version="1.0" encoding="utf-8" ?>


no need for this file

markcowl · 2018-02-02T05:26:41Z

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

cormacpayne · 2018-02-02T16:22:12Z

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

markcowl · 2018-02-03T05:11:14Z

new on-demand run here: https://azuresdkci.westus2.cloudapp.azure.com/job/powershell-demand/422/

azuresdkci added the needs-review label Dec 6, 2017

azuresdkci assigned shahabhijeet Dec 6, 2017

devigned force-pushed the feature/aks branch from 658583d to 55174ef Compare December 6, 2017 19:14

maddieclayton added the Do Not Merge 🚫 label Dec 6, 2017

devigned force-pushed the feature/aks branch from 1b8aa15 to 0922c44 Compare December 13, 2017 18:02

markcowl assigned maddieclayton and unassigned shahabhijeet Dec 18, 2017

devigned force-pushed the feature/aks branch from 3f066cc to 1d86f19 Compare December 18, 2017 21:49

maddieclayton assigned devigned and unassigned maddieclayton Dec 19, 2017

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

markcowl requested changes Dec 20, 2017

View reviewed changes

maddieclayton reviewed Dec 22, 2017

View reviewed changes

devigned force-pushed the feature/aks branch from de6c429 to 170481a Compare January 5, 2018 19:23

devigned changed the title ~~[WIP Do Not Merge] AzureRmKubernetes cmdlets for managed Kubernetes~~ AzureRmKubernetes cmdlets for managed Kubernetes Jan 9, 2018

devigned force-pushed the feature/aks branch from b2938de to 469184f Compare January 9, 2018 16:10

devigned force-pushed the feature/aks branch 2 times, most recently from 1cbf85e to dd6e688 Compare January 9, 2018 16:44

maddieclayton added needs-review and removed needs-revision labels Jan 10, 2018

maddieclayton unassigned devigned Jan 10, 2018

devigned and others added 13 commits January 30, 2018 11:06

place const at top of method

e3e30a1

fix feedback items -- now need to figure out how to fix tests

571947b

remove app.config

fedf6b0

resolving feedback

d55bbd4

add prerelease tag

9bf3c89

fix ref to signed yamldotnet

8752ea1

addressing last bit of feedback

75374e1

Use existing MemoryDataStore if one has been previously instantiated

8a6cfc2

add file fixtures for tests

3d31133

fix the netcore solution from merge conflig

7f97fee

add using to the probe test

19d7ff4

remove src and RM folders from netcore sln

61e2a71

look for YamlDotNet in ./src/packages

f6873d4

devigned force-pushed the feature/aks branch from 3c05e7e to f6873d4 Compare January 30, 2018 19:06

cormacpayne and others added 3 commits January 30, 2018 13:55

Merge branch 'preview' into feature/aks

b0d4ea1

Merge branch 'preview' into feature/aks

2b763f9

Merge branch 'preview' into feature/aks

d8e1403

markcowl previously approved these changes Feb 2, 2018

View reviewed changes

markcowl added Waiting for On Demand and removed needs-review labels Feb 2, 2018

markcowl removed their assignment Feb 2, 2018

Remove empty line in SignatureIssues.csv

ff1a650

cormacpayne dismissed markcowl’s stale review via ff1a650 February 2, 2018 16:21

Merge branch 'preview' into feature/aks

7cd89fb

markcowl approved these changes Feb 5, 2018

View reviewed changes

markcowl merged commit 6d78a52 into Azure:preview Feb 5, 2018

		$location = Get-ProviderLocation "Microsoft.ContainerService/managedClusters"

		New-AzureRmResourceGroup -Name $resourceGroupName -Location $location

		@@ -0,0 +1,3 @@
		Import-Module -Name ./src/Package/Release/ResourceManager/AzureRM.Profile.Netcore/AzureRM.Profile.Netcore.psd1

		@@ -0,0 +1,6 @@
		<?xml version="1.0" encoding="utf-8"?>
		<configuration>

AzureRmKubernetes cmdlets for managed Kubernetes #5087

AzureRmKubernetes cmdlets for managed Kubernetes #5087

Uh oh!

Conversation

devigned commented Dec 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

twitchax commented Dec 7, 2017

Uh oh!

twitchax commented Dec 7, 2017

Uh oh!

maddieclayton commented Dec 19, 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!

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 Dec 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devigned commented Jan 9, 2018

Uh oh!

devigned commented Jan 9, 2018

Uh oh!

maddieclayton commented Jan 9, 2018

Uh oh!

maddieclayton commented Jan 9, 2018

Uh oh!

devigned commented Jan 30, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markcowl commented Feb 2, 2018

Uh oh!

cormacpayne commented Feb 2, 2018

Uh oh!

markcowl commented Feb 3, 2018

Uh oh!

Uh oh!

devigned commented Dec 6, 2017 •

edited

Loading

maddieclayton commented Dec 22, 2017 •

edited

Loading