Common Code Version of Compute Management Library (Issue #5512) #5709

vladimir-shcherbakov · 2018-03-09T19:03:39Z

Description

Checklist

I have read the Submitting Changes section of CONTRIBUTING.md
The title of the PR is clear and informative
The appropriate change log has been updated
The PR does not introduce breaking changes
If applicable, the changes made in the PR have proper test coverage
For public API changes to cmdlets:
- the changes have gone through a cmdlet design review
- the cmdlet markdown files were generated using the platyPS module

….Profile project so it gets copied to the Package dir.

…ders to make CredScan happy.

markcowl · 2018-03-14T17:09:36Z

src/Common/Commands.Common.Compute.Tests/Commands.Common.Compute.Tests.csproj

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


I think this should go in the profile tests project - if it does not, you will need to create a lot of test infrastructure to make this execute like the other tests.

markcowl · 2018-03-14T17:11:27Z

src/Common/Commands.Common.Compute.Tests/CredentialManager.cs

+    {
+        protected CredentialManager() { }
+
+        private const string ServicePrincipalEnvVariableName = "AZURE_SERVICE_PRINCIPAL";


We already have a test infrastructure that does this -this will make it essentially impossible to run these tests on ci without setting up this environment variable everywhere. Let's dicuss thet best way to bring these tests inline.

markcowl · 2018-03-14T17:11:58Z

src/Common/Commands.Common.Compute.Tests/CredentialManager.cs

+            }
+        }
+
+        public static CredentialManager FromServicePrincipalEnvVariable(string envVariableName = ServicePrincipalEnvVariableName)


We should likely remove this file.

markcowl · 2018-03-14T17:14:34Z

...Common/Commands.Common.Compute.Tests/Version2016-04-preview/ComputeManagementClientShould.cs

+    {
+        public class ComputeManagementClientShould
+        {
+            private IComputeManagementClient Client {get;} 


These all need to be check-in tests. Let's discuss what to do with this per the above.

markcowl · 2018-03-14T17:16:35Z

src/ResourceManager/Profile/Profile.sln

@@ -40,6 +40,10 @@ Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Commands.Resources.Rest", "
 EndProject
 Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Commands.Insights", "..\Insights\Commands.Insights\Commands.Insights.csproj", "{DEA446A1-84E2-46CC-B780-EB4AFDE2460E}"
 EndProject
+Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Commands.Common.Compute", "..\..\Common\Commands.Common.Compute\Commands.Common.Compute.csproj", "{F6D508D1-BE2D-475D-AA0F-DFB5C615CC9D}"


You will also need to add this into each solution that builds profile (which is most ARM solutions, and the Azure solution, I believe).

…rofile

markcowl · 2018-03-20T00:24:34Z

signing run here:https://azuresdkci.westus2.cloudapp.azure.com/view/PowerShell/job/ps-sign/266/

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

cormacpayne · 2018-03-20T18:32:49Z

src/Common/Commands.Common.Compute/Commands.Common.Compute.csproj

+    <ErrorReport>prompt</ErrorReport>
+    <WarningLevel>4</WarningLevel>
+  </PropertyGroup>
+  <PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Release|AnyCPU' ">


@vladimir-shcherbakov using Commands.Common.Authorization as an example, this PropertyGroup should look like the following:

<PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Release|AnyCPU' "> <DebugType>pdbonly</DebugType> <Optimize>true</Optimize> <OutputPath>bin\Release\</OutputPath> <DefineConstants>TRACE;SIGN</DefineConstants> <PlatformTarget>AnyCPU</PlatformTarget> <CodeAnalysisLogFile>bin\Release\Management.Utilities.dll.CodeAnalysisLog.xml</CodeAnalysisLogFile> <CodeAnalysisUseTypeNameInSuppression>true</CodeAnalysisUseTypeNameInSuppression> <CodeAnalysisModuleSuppressionsFile>GlobalSuppressions.cs</CodeAnalysisModuleSuppressionsFile> <ErrorReport>prompt</ErrorReport> <CodeAnalysisRuleSet>MinimumRecommendedRules.ruleset</CodeAnalysisRuleSet> <SignAssembly>true</SignAssembly> <AssemblyOriginatorKeyFile>MSSharedLibKey.snk</AssemblyOriginatorKeyFile> <DelaySign>true</DelaySign> <TreatWarningsAsErrors>true</TreatWarningsAsErrors> <Prefer32Bit>false</Prefer32Bit> </PropertyGroup>

You will also need to copy the MSSharedLibKey.snk file over to this project and ensure that it gets referenced in this .csproj file.

Making these two changes will fix the build error that we are seeing during the sign job that is saying Commands.Common.Compute is not a strongly signed assembly.

@cormacpayne, done.

cormacpayne · 2018-03-22T00:04:09Z

Azure.PowerShell.Netcore.sln

@@ -27,8 +27,6 @@ Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Common.Authentication.Abstr
 EndProject
 Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Common.ResourceManager.Authentication.Netcore", "src\ResourceManager\Common\Commands.Common.Authentication.ResourceManager\Common.ResourceManager.Authentication.Netcore.csproj", "{AF50ACE6-9A6A-4A18-8140-B6C5BDD0EFD6}"
 EndProject
-Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Commands.Compute.Netcore", "src\ResourceManager\Compute\Commands.Compute\Commands.Compute.Netcore.csproj", "{7B61AB3B-9F62-4B96-BCED-13920B56CD3C}"


@vladimir-shcherbakov we don't want to delete this, right? This means that the Compute project won't get built

vladimir-shcherbakov · 2018-03-22T06:09:42Z

Azure.PowerShell.Netcore.sln

@@ -53,6 +51,8 @@ Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Common.Strategies.Netcore",
 EndProject
 Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Commands.Common.Tests", "src\Common\Commands.Common.Tests\Commands.Common.Tests.csproj", "{B0EF35E9-2D7A-4AAE-8A1A-728B6CC8524B}"
 EndProject
+Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Commands.Common.Compute.Netcore", "src\Common\Commands.Common.Compute\Commands.Common.Compute.Netcore.csproj", "{5E5D1F8E-92C8-40EB-AABA-DFD12AFD6D0A}"
+EndProject
 Global


@cormacpayne
It's not deleted.

@vladimir-shcherbakov Commands.Common.Compute.Netcore is being added, but Commands.Compute.Netcore appears to be being removed

cormacpayne · 2018-03-22T16:50:57Z

@vladimir-shcherbakov it looks like you need to add the Commands.Common.Compute project to the solution of the Compute Stack solution: https://github.com/Azure/azure-powershell/blob/preview/src/ResourceManager/Compute/Stack/Compute.sln

…ompute.sln; Commands.Compute.Netcore added to Azure.PowerShell.Netcore.sln

cormacpayne · 2018-03-22T22:25:52Z

@vladimir-shcherbakov also need to add the Commands.Common.Compute project to the stack solution for Network and Storage

…ck/Network.sln and src/ResourceManager/Storage/Stack/Storage.sln

cormacpayne · 2018-03-23T17:04:36Z

@vladimir-shcherbakov okay, hopefully this will finally get signing to work:

Add the Commands.Common.Compute project to the ServerManagement solution
Add the Commands.Common.Compute project to the Storage solution

src/ResourceManager/Storage/Storage.sln src/ResourceManager/ServerManagement/ServerManagement.sln src/Stack.sln

vladimir-shcherbakov · 2018-03-23T19:08:19Z

Signing works! https://azuresdkci.westus2.cloudapp.azure.com/view/PowerShell/job/ps-sign/292/

markcowl · 2018-03-25T03:01:34Z

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

All comments addressed

Common Code Version of Compute Management Library (Issue Azure#5512)

b7cccfe

azuresdkci added the needs-review label Mar 9, 2018

vladimir-shcherbakov and others added 4 commits March 9, 2018 12:21

CI build fix

fdfcacf

proj files moved from Compute soulution to Profile.

91ab984

jenkins build fix.

6bf4e57

Merge remote-tracking branch 'upstream/preview' into iss#5512

f93ffbd

maddieclayton added needs-revision and removed needs-review labels Mar 12, 2018

maddieclayton assigned vladimir-shcherbakov Mar 12, 2018

vladimir-shcherbakov added 3 commits March 12, 2018 14:21

Microsoft.Azure.Commands.Common.Compute project ref added to Commands…

60b64cb

….Profile project so it gets copied to the Package dir.

Examples of the disabled passwords removed from the documentation hea…

5e3309a

…ders to make CredScan happy.

Merge remote-tracking branch 'upstream/preview' into iss#5512

0d20a38

vladimir-shcherbakov requested review from markcowl and cormacpayne March 13, 2018 20:05

markcowl requested changes Mar 14, 2018

View reviewed changes

vladimir-shcherbakov and others added 3 commits March 14, 2018 16:52

Commands.Common.Compute project added to every solution referencing P…

7b18865

…rofile

[Trait(Category.RunType, Category.LiveOnly)] added

fd04b6d

Commands.ScenarioTests.Common proj referenced to get Category.RunType.

4c885dc

markcowl added needs-review and removed needs-revision labels Mar 19, 2018

markcowl assigned markcowl and unassigned vladimir-shcherbakov Mar 19, 2018

markcowl previously approved these changes Mar 20, 2018

View reviewed changes

markcowl added Waiting for On Demand and removed needs-review labels Mar 20, 2018

cormacpayne previously requested changes Mar 20, 2018

View reviewed changes

cormacpayne assigned vladimir-shcherbakov and unassigned markcowl Mar 20, 2018

cormacpayne added the needs-revision label Mar 20, 2018

cormacpayne removed the Waiting for On Demand label Mar 20, 2018

sign job related stuff added.

62fd1f7

vladimir-shcherbakov dismissed markcowl’s stale review via 62fd1f7 March 21, 2018 16:36

vladimir-shcherbakov assigned markcowl and unassigned vladimir-shcherbakov Mar 21, 2018

another try to make sign job pass.

4caf066

cormacpayne reviewed Mar 22, 2018

View reviewed changes

vladimir-shcherbakov commented Mar 22, 2018

View reviewed changes

Commands.Common.Compute.proj added to ResourceManager/Compute/Stack/C…

8bdbcb4

…ompute.sln; Commands.Compute.Netcore added to Azure.PowerShell.Netcore.sln

Commands.Common.Compute.proj added to src/ResourceManager/Network/Sta…

88bac37

…ck/Network.sln and src/ResourceManager/Storage/Stack/Storage.sln

Commands.Common.Compute.proj added to

5fcf1fe

src/ResourceManager/Storage/Storage.sln src/ResourceManager/ServerManagement/ServerManagement.sln src/Stack.sln

markcowl removed the needs-revision label Mar 25, 2018

markcowl removed their assignment Mar 25, 2018

markcowl added the Waiting for On Demand label Mar 25, 2018

markcowl approved these changes Mar 25, 2018

View reviewed changes

markcowl merged commit 8278e93 into Azure:preview Mar 25, 2018

Common Code Version of Compute Management Library (Issue #5512) #5709

Common Code Version of Compute Management Library (Issue #5512) #5709

Uh oh!

Conversation

vladimir-shcherbakov commented Mar 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

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 Mar 20, 2018

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!

cormacpayne commented Mar 22, 2018

Uh oh!

cormacpayne commented Mar 22, 2018

Uh oh!

cormacpayne commented Mar 23, 2018

Uh oh!

vladimir-shcherbakov commented Mar 23, 2018

Uh oh!

markcowl commented Mar 25, 2018

Uh oh!

Uh oh!

vladimir-shcherbakov commented Mar 9, 2018 •

edited

Loading