Netcore + KeyVault #4879

twitchax · 2017-10-28T10:33:27Z

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.

markcowl · 2017-10-30T18:05:21Z

src/Common/Commands.Common.Authentication/AzureSessionInitializer.cs

@@ -183,15 +183,15 @@ public AdalSession()
            {
            }

-            public override TraceLevel AuthenticationLegacyTraceLevel
+            public override System.Diagnostics.TraceLevel AuthenticationLegacyTraceLevel


Is this necessary? Is this class in a different namespace in NetCore or something?

Yes, Newtonsoft.

'TraceLevel' is an ambiguous reference between 'System.Diagnostics.TraceLevel' and 'Newtonsoft.Json.TraceLevel'

markcowl · 2017-10-30T18:06:59Z

src/Common/Commands.Common.Authorization/Common.Authorization.Netcore.csproj

+
+  <ItemGroup Condition=" '$(TargetFramework)' == 'netcoreapp2.0' ">
+    <PackageReference Include="Microsoft.Azure.Common" Version="2.1.4" />
+    <PackageReference Include="Microsoft.Rest.ClientRuntime.Azure.Authentication" Version="2.2.12" />


I believe these should come from common dependencies

I just grabbed some of the most common dependencies. Did you want me to shove all of them in there? 😄

markcowl · 2017-10-30T18:07:24Z

src/Common/Commands.Common.Graph.RBAC/Common.Rbac.Netcore.csproj

+  </PropertyGroup>
+
+  <ItemGroup Condition=" '$(TargetFramework)' == 'netcoreapp2.0' ">
+    <PackageReference Include="Microsoft.Azure.Common" Version="2.1.4" />


same comment

markcowl · 2017-10-30T18:07:47Z

src/Common/Commands.Common.Network/Common.Network.Netcore.csproj

+  </PropertyGroup>
+
+  <ItemGroup Condition=" '$(TargetFramework)' == 'netcoreapp2.0' ">
+    <PackageReference Include="Microsoft.Azure.Common" Version="2.1.4" />


same comment

markcowl · 2017-10-30T18:08:31Z

src/Common/Commands.Common.Storage/Common.Storage.Netcore.csproj

  </PropertyGroup>

  <ItemGroup Condition=" '$(TargetFramework)' == 'netcoreapp2.0' ">
-    <PackageReference Include="Hyak.Common" Version="1.1.3" />
-    <PackageReference Include="Microsoft.ApplicationInsights" Version="2.2.0" />
+    <PackageReference Include="Microsoft.ApplicationInsights" Version="2.4.0" />


same comments about package references

markcowl · 2017-10-30T18:13:17Z

src/ResourceManager/Profile/Commands.Profile/Commands.Profile.Netcore.csproj

@@ -23,14 +24,13 @@
  </PropertyGroup>

  <ItemGroup Condition=" '$(TargetFramework)' == 'netcoreapp2.0' ">
-    <PackageReference Include="Hyak.Common" Version="1.1.3" />


same comment on common dependencies

markcowl · 2017-10-30T18:13:48Z

...rceManager/Resources/Commands.ResourceManager/Cmdlets/Commands.Resources.Rest.Netcore.csproj

  </PropertyGroup>

  <ItemGroup Condition=" '$(TargetFramework)' == 'netcoreapp2.0' ">
-    <PackageReference Include="Hyak.Common" Version="1.1.3" />


Same comment on depednencies - assuem that this applies across the board for all modules

markcowl · 2017-10-30T18:14:51Z

src/ResourceManager/Resources/Commands.Resources/Extensions/ResourceExtensions.cs


 namespace Microsoft.Azure.Commands.Resources.Models
 {
    public static class ResourceExtensions
    {
-#if !NETSTANDARD
-        public static DeploymentExtended Deployment(this DeploymentGetResult deployment)


these aren't used anywhere in the desktop code?

Not since I moved us away from Microsoft.Azure.Management.Resources to Microsoft.Azure.Management.ResourceManager.

markcowl · 2017-10-30T18:15:16Z

...Manager/Resources/Commands.Resources/Models.ResourceGroups/ResourceClient.ResourceManager.cs

@@ -47,55 +42,6 @@ public partial class ResourcesClient
        /// </summary>
        /// <param name="parameters">The get parameters</param>
        /// <returns>List of resources</returns>
-#if !NETSTANDARD
-        public virtual List<PSResource> FilterPSResources(BasePSResourceParameters parameters)


This isn't used anywhere?

Same comment as above: this was based on the old ResourceManager SDK.

markcowl · 2017-10-30T18:17:24Z

tools/InstallationTests/NetcoreTests/Resources/PolicyTests.ps1

@@ -24,12 +24,14 @@ function Test-PolicyDefinitionCRUD

 	# Test
 	$actual = New-AzureRMPolicyDefinition -Name $policyName -Policy SamplePolicyDefinition.json
+	Start-Sleep 5


We shopuld actually have a test/retry loop here, as it may take more than 5 seconds.

I'm hedging for xUnit support mid November. :)

Good luck with that. Meanwhile, there is an actual common function that does retries. Please use it here

Here is the retry function: https://github.com/Azure/azure-powershell/blob/preview/src/ResourceManager/Common/Commands.ScenarioTests.ResourceManager.Common/Common.ps1#L338

markcowl · 2017-11-15T02:10:47Z

src/Common/Commands.Common.Authorization/Common.Authorization.Netcore.csproj

+    <PackageReference Include="System.Security.SecureString" Version="4.3.0" />
+    <PackageReference Include="System.Xml.XmlDocument" Version="4.3.0" />
+    <PackageReference Include="System.Xml.XmlSerializer" Version="4.3.0" />
+    <PackageReference Include="Newtonsoft.Json" Version="10.0.3" />


Newtonsoft.Json is part of common dependencies, right? This applies to all usages

markcowl · 2017-11-15T02:11:39Z

src/Common/Commands.Common.Authentication/Common.Authentication.Netcore.csproj

    <PackageReference Include="Microsoft.Azure.Common" Version="2.1.4" />
    <PackageReference Include="Microsoft.Rest.ClientRuntime.Azure.Authentication" Version="2.2.12" />
    <PackageReference Include="System.Collections.Specialized" Version="4.3.0" />
    <PackageReference Include="System.Reflection" Version="4.3.0" />
    <PackageReference Include="System.Security.SecureString" Version="4.3.0" />
    <PackageReference Include="System.Xml.XmlDocument" Version="4.3.0" />
    <PackageReference Include="System.Xml.XmlSerializer" Version="4.3.0" />
-    <PackageReference Include="Newtonsoft.Json" Version="10.0.1" />
-    <PackageReference Include="Microsoft.PowerShell.SDK" Version="6.0.0-beta.3" />
+    <PackageReference Include="Newtonsoft.Json" Version="10.0.3" />


Newtonsoft.Json should come form Common.NetCore, right?

markcowl · 2017-11-15T02:14:33Z

src/ResourceManager/KeyVault/AzureRM.KeyVault.Netcore.psd1

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


All dependencies not included in AzureRM.Profile.NetCore.psd1 will need to be listed here in RequiredAssemblies (as I obtusely noted below). This generally includes the management client assembly

markcowl · 2017-11-15T02:17:45Z

src/ResourceManager/Profile/AzureRM.Profile.Netcore.psd1

@@ -54,13 +54,13 @@ PowerShellVersion = '5.1'
 # RequiredModules = @()

 # Assemblies that must be loaded prior to importing this module
-# RequiredAssemblies = @()
+RequiredAssemblies = 'netcoreapp2.0\Microsoft.Azure.Commands.Common.Authentication.ResourceManager.dll'


See the desktop version of thsi file - you will need to include the dependencies mentioned there (basically everything in your common targets file)

markcowl · 2017-11-15T02:19:21Z

src/ResourceManager/Resources/Commands.Resources/Commands.Resources.csproj

    <Reference Include="Microsoft.Azure.ResourceManager, Version=2.0.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35, processorArchitecture=MSIL">
      <HintPath>..\..\..\packages\Microsoft.Azure.Management.Resources.2.20.0-preview\lib\net40\Microsoft.Azure.ResourceManager.dll</HintPath>
    </Reference>
+    <Reference Include="Microsoft.Rest.ClientRuntime, Version=2.0.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35, processorArchitecture=MSIL">


This module needs to get these assemblies form the common dependencies target

Still not getting these assembl;ies form common

markcowl · 2017-11-15T02:21:52Z

src/ResourceManager/Resources/Commands.Resources/packages.config

  <package id="Microsoft.Azure.Management.Resources" version="2.20.0-preview" targetFramework="net45" />
-</packages>
+  <package id="Microsoft.Rest.ClientRuntime" version="2.3.6" targetFramework="net452" />


Remove - these are retrieved by the dependency on ResourceManager common

Same comment.

markcowl · 2017-11-15T02:23:23Z

tools/InstallationTests/NetcoreTests/Common.ps1

@@ -360,7 +360,7 @@ Gets valid resource name
 #>
 function Get-ResourceName
 {
-  return "some-unique-rg-name"
+  return "some-unique-res-name"


why not the same?

rs$(Get-RandomLetters)

There were some problems with making this random because some of the tests were not exactly "unit". We are throwing all of these away once xUnit is ready, correct?

markcowl · 2017-11-15T02:24:48Z

tools/InstallationTests/NetcoreTests/Resources/PolicyTests.ps1

@@ -24,12 +24,14 @@ function Test-PolicyDefinitionCRUD

 	# Test
 	$actual = New-AzureRMPolicyDefinition -Name $policyName -Policy SamplePolicyDefinition.json
+	Start-Sleep 5


Good luck with that. Meanwhile, there is an actual common function that does retries. Please use it here

markcowl · 2017-11-15T02:25:31Z

tools/InstallationTests/NetcoreTests/Websites/Common.ps1

@@ -12,13 +12,18 @@
 # limitations under the License.
 # ----------------------------------------------------------------------------------

+function Get-RandomLetters
+{
+	return -join ((97..122) | Get-Random -Count 5 | % {[char]$_})


twitchax · 2017-11-16T22:09:23Z

@azuresdkci retest this please

twitchax · 2017-11-17T00:11:47Z

@azuresdkci retest this please

twitchax · 2017-11-17T08:19:53Z

@maddieclayton, it looks like the LocationCompleter/ResourceGroupCompleter tests can intermittently fail due to timeout.

There is a 5 second timeout, which is probably good for live usage; however, we might want to make it configurable for tests.

maddieclayton · 2017-11-17T16:51:03Z

Yikes I'm on it

twitchax · 2017-11-18T05:20:18Z

Hey @markcowl, can you take a look when you get a chance? :D

twitchax · 2017-11-22T09:27:25Z

@markcowl, ping. 😺

Sorry to bother you.

devigned · 2017-11-28T00:10:10Z

Looking forward to this getting merged

markcowl · 2017-11-28T21:14:52Z

src/ResourceManager/Compute/AzureRM.Compute.Netcore.psd1

-# RequiredAssemblies = @()
+RequiredAssemblies = 'netcoreapp2.0\AutoMapper.dll',
+    'netcoreapp2.0\Microsoft.Azure.Management.Compute.dll',
+    'netcoreapp2.0\Microsoft.Azure.Management.KeyVault.dll',


We will need to look at removing these in a future release

markcowl · 2017-11-28T21:15:46Z

src/ResourceManager/Compute/Commands.Compute/Commands.Compute.Netcore.csproj

@@ -23,23 +27,20 @@
  </PropertyGroup>

  <ItemGroup Condition=" '$(TargetFramework)' == 'netcoreapp2.0' ">
-    <PackageReference Include="Hyak.Common" Version="1.1.3" />
-    <PackageReference Include="Microsoft.Azure.Common" Version="2.1.4" />
-    <PackageReference Include="Microsoft.Rest.ClientRuntime.Azure.Authentication" Version="2.2.12" />
    <PackageReference Include="Microsoft.Azure.Management.ResourceManager" Version="1.6.0-preview" />


We will need to repl;ace these with common code references fairly soon, but this needs to happen for the deskop version as well

markcowl · 2017-11-28T21:18:48Z

src/ResourceManager/Resources/Commands.Resources/Commands.Resources.csproj

    <Reference Include="Microsoft.Azure.ResourceManager, Version=2.0.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35, processorArchitecture=MSIL">
      <HintPath>..\..\..\packages\Microsoft.Azure.Management.Resources.2.20.0-preview\lib\net40\Microsoft.Azure.ResourceManager.dll</HintPath>
    </Reference>
+    <Reference Include="Microsoft.Rest.ClientRuntime, Version=2.0.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35, processorArchitecture=MSIL">


Still not getting these assembl;ies form common

markcowl · 2017-11-28T21:19:33Z

src/ResourceManager/Resources/Commands.Resources/packages.config

  <package id="Microsoft.Azure.Management.Resources" version="2.20.0-preview" targetFramework="net45" />
-</packages>
+  <package id="Microsoft.Rest.ClientRuntime" version="2.3.6" targetFramework="net452" />


Same comment.

markcowl · 2017-11-28T21:22:07Z

tools/InstallationTests/NetcoreTests/Resources/PolicyTests.ps1

@@ -24,12 +24,14 @@ function Test-PolicyDefinitionCRUD

 	# Test
 	$actual = New-AzureRMPolicyDefinition -Name $policyName -Policy SamplePolicyDefinition.json
+	Start-Sleep 5


Here is the retry function: https://github.com/Azure/azure-powershell/blob/preview/src/ResourceManager/Common/Commands.ScenarioTests.ResourceManager.Common/Common.ps1#L338

markcowl · 2017-11-28T21:25:01Z

@twitchax You will need to fix the resource manager module dependencies as you did for the other modules (as noted), since this will impact backward compatibility. I would really like you to use the retry function in the tests, but will not block the PR for it.

twitchax · 2017-11-29T03:46:07Z

@azuresdkci test this please.

twitchax · 2017-11-29T06:30:41Z

@markcowl, take a look? :)

markcowl · 2017-11-30T03:56:04Z

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

packaging run here: https://azuresdkci.westus2.cloudapp.azure.com/view/PowerShell/job/posh-pack/38/

twitchax · 2017-11-30T07:10:46Z

@markcowl...green! 🚀

cormacpayne · 2017-11-30T16:08:06Z

On-demand: https://azuresdkci.westus2.cloudapp.azure.com/view/PowerShell/job/powershell-demand/214/

azuresdkci added the needs-review label Oct 28, 2017

azuresdkci assigned shahabhijeet Oct 28, 2017

cormacpayne assigned markcowl and cormacpayne and unassigned shahabhijeet Oct 29, 2017

markcowl requested changes Oct 30, 2017

View reviewed changes

markcowl assigned twitchax and unassigned markcowl and cormacpayne Oct 30, 2017

markcowl added needs-revision and removed needs-review labels Oct 30, 2017

cormacpayne mentioned this pull request Oct 30, 2017

Netcore is back in action. #4847

Closed

13 tasks

twitchax force-pushed the netcore branch from d49ef05 to 6891be9 Compare November 1, 2017 09:00

markcowl added the Do Not Merge 🚫 label Nov 2, 2017

This was referenced Nov 7, 2017

[AzureRM.Netcore] a misleading error message when you are not logged in #4487

Closed

AzureRm.Netcore Login-AzureRmAccount not working on Mac and Linux with Service Principal #4476

Closed

twitchax force-pushed the netcore branch 3 times, most recently from 325d741 to 20e7f83 Compare November 9, 2017 21:57

twitchax added needs-review and removed needs-revision Do Not Merge 🚫 labels Nov 14, 2017

markcowl requested changes Nov 15, 2017

View reviewed changes

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

twitchax force-pushed the netcore branch from cbaae66 to 9306409 Compare November 16, 2017 10:01

twitchax added needs-review and removed needs-revision labels Nov 16, 2017

maddieclayton assigned markcowl and unassigned twitchax Nov 18, 2017

maddieclayton mentioned this pull request Nov 21, 2017

Make timeout adjustable and rerecord test with longer timeout for completers #5017

Merged

13 tasks

Netcore + KeyVault + Docker + Travis.

9cf77ac

twitchax force-pushed the netcore branch from 9306409 to 9cf77ac Compare November 27, 2017 22:19

markcowl requested changes Nov 28, 2017

View reviewed changes

Remove unnecessary references and fix tests.

4277d2c

markcowl approved these changes Nov 30, 2017

View reviewed changes

markcowl removed the needs-review label Nov 30, 2017

markcowl removed their assignment Nov 30, 2017

markcowl added the Waiting for On Demand label Nov 30, 2017

markcowl merged commit b82f28b into Azure:preview Nov 30, 2017

twitchax deleted the netcore branch December 3, 2017 23:24

Netcore + KeyVault #4879

Netcore + KeyVault #4879

Uh oh!

Conversation

twitchax commented Oct 28, 2017 • 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!

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!

twitchax commented Nov 16, 2017

Uh oh!

twitchax commented Nov 17, 2017

Uh oh!

twitchax commented Nov 17, 2017

Uh oh!

maddieclayton commented Nov 17, 2017

Uh oh!

twitchax commented Nov 18, 2017

Uh oh!

twitchax commented Oct 28, 2017 •

edited

Loading