Powershell changes to onboard additional traffic analytics parameters with existing networkwatcher flowlog parameters #6253

sayghosh · 2018-05-18T17:17:33Z

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

MikhailTryakhov · 2018-05-18T18:05:57Z

sayghosh · 2018-05-19T13:00:56Z

The build failed to to 2 test failures (1 failure unrelated to my changes). But my local build of the repo is working fine.Please advise what to do here. Below are the errors:

PS: I have ran few validation tests on my changes and the tests are successful.

sayghosh · 2018-05-20T07:46:48Z

I have made fixes and after that also I see build issues but the traces do not show any error. In the build artifacts msbuild.err is 0B.

https://azuresdkci.westus2.cloudapp.azure.com/job/powershell/4632/artifact/

I see this in console output:

Is this an intermittent issue?

PS: In local I have ran the FlowLog test and its functional.

sayghosh · 2018-05-20T13:07:46Z

Looks like the above build fail was intermittent :)

maddieclayton · 2018-05-21T16:20:48Z

@sayghosh Have you filled out a cmdlet design review? If so, please link in the description. If not, please fill one out here: https://github.com/Azure/azure-powershell-cmdlet-review-pr/issues

sayghosh · 2018-05-21T17:02:36Z

@maddieclayton Here is the PR: https://github.com/Azure/azure-powershell-cmdlet-review-pr/issues/60#issuecomment-387464726

maddieclayton · 2018-05-21T17:56:52Z

src/ResourceManager/Network/Commands.Network.Test/ScenarioTests/NetworkWatcherAPITests.ps1

@@ -503,24 +503,21 @@ function Test-FlowLog
    # Setup
    $resourceGroupName = Get-ResourceGroupName
    $nwName = Get-ResourceName
-    $location = "eastus"
+    $location = "eastus2euap"


Please use Get-Location for both of these: https://github.com/Azure/azure-powershell/wiki/Azure-Powershell-Developer-Guide#using-common-code

Did not update as the same location field is used to create many resources and no all of them are available in new france regions. Let me know your thoughts here

You should use a different location variable for each resource type and use Get-Location. This guarentees that you will be able to rerecord even when permissions to different regions change, or when you are in a different cloud (China, Fairfax).

Actually the NSG, NetworkWatcher and Storage Account has to be in same azure region while the workspace can be in any indepedent region, that is why there are 2 location variables used here. And to enable Traffic analytics, both the networkwatcher and WS has to be in a Traffic Analytics supported region. But currently Traffic Analytics is availble only on 14 azure regions. FOr now I can add a comment to it with reason why I could not use Get-Location there. Does this make sense? PLease adivse.

Okay, adding the comment should be fine.

maddieclayton · 2018-05-21T17:58:16Z

src/ResourceManager/Network/Commands.Network.Test/ScenarioTests/NetworkWatcherAPITests.ps1


    try 
    {
        # Create Resource group
        New-AzureRmResourceGroup -Name $resourceGroupName -Location "$location"

-        # Create the Virtual Network
-        $subnet = New-AzureRmVirtualNetworkSubnetConfig -Name $subnetName -AddressPrefix 10.0.1.0/24


Was this just never used?

No, this was never used

maddieclayton · 2018-05-21T17:58:38Z

src/ResourceManager/Network/Commands.Network.Test/packages.config

  <package id="Microsoft.Azure.Management.Redis" version="3.1.1-preview" targetFramework="net45" />
  <package id="Microsoft.Azure.Management.ResourceManager" version="1.6.0-preview" targetFramework="net452" />
  <package id="Microsoft.Azure.Management.Resources" version="2.20.0-preview" targetFramework="net45" />
  <package id="Microsoft.Azure.Management.Storage" version="6.1.0-preview" targetFramework="net45" />
+  <package id="Microsoft.Azure.OperationalInsights" version="0.9.0.1-preview" targetFramework="net452" />


Don't think you need this package.

maddieclayton · 2018-05-21T17:59:04Z

src/ResourceManager/Network/Commands.Network.Test/Commands.Network.Test.csproj

@@ -47,6 +47,9 @@
    <Reference Include="Hyak.Common">
      <HintPath>..\..\..\packages\Hyak.Common.1.0.3\lib\portable-net403+win+wpa81\Hyak.Common.dll</HintPath>
    </Reference>
+    <Reference Include="Microsoft.Azure.Commands.OperationalInsights">


I don't think you need this.

maddieclayton · 2018-05-21T18:03:35Z

src/ResourceManager/Network/Commands.Network.Test/Commands.Network.Test.csproj

@@ -87,6 +93,9 @@
      <HintPath>..\..\..\packages\Microsoft.Azure.Management.Redis.3.1.1-preview\lib\net45\Microsoft.Azure.Management.Redis.dll</HintPath>
      <Private>True</Private>
    </Reference>
+    <Reference Include="Microsoft.Azure.OperationalInsights, Version=0.9.0.1, Culture=neutral, PublicKeyToken=31bf3856ad364e35, processorArchitecture=MSIL">


I don't think you need this package - if it does not work without this package let me know.

maddieclayton · 2018-05-21T18:11:40Z

...anager/Network/Commands.Network/NetworkWatcher/SetAzureNetworkWatcherConfigFlowLogCommand.cs

+
+                        if (ParameterSetName.Contains(TAByDetails))
+                        {
+                            if (this.WorkspaceResourceId == null || this.WorkspaceGUId == null || this.WorkspaceLocation == null)


You don't need to have this check - powershell will not allow the user to not enter mandatory parameters for a parameter set.

maddieclayton · 2018-05-21T18:13:40Z

...anager/Network/Commands.Network/NetworkWatcher/SetAzureNetworkWatcherConfigFlowLogCommand.cs

+                                throw new System.ArgumentException("Either the Workspace parameter or all of WorkspaceResourceId,WorkspaceGUId,WorkspaceLocation must be provided");
+                            }
+
+                            string[] workspaceDetailsComponents = this.WorkspaceResourceId.Split('/');


You can use this: https://github.com/Azure/azure-powershell/blob/preview/src/ResourceManager/Common/Commands.ResourceManager.Common/Utilities/Models/ResourceIdentifier.cs

None of these APIs validate check of exact 8 components (They check >= 8). Is it advisabel to use that check still?

maddieclayton · 2018-05-21T18:13:53Z

...anager/Network/Commands.Network/NetworkWatcher/SetAzureNetworkWatcherConfigFlowLogCommand.cs

+                        }
+                        else
+                        {
+                            if (this.Workspace == null)


remove this check.

maddieclayton · 2018-05-21T18:15:20Z

...ResourceManager/ServiceFabric/Commands.ServiceFabric.Test/ScenarioTests/TestServiceFabric.cs

@@ -118,7 +118,7 @@ public void TestRemoveAzureRmServiceFabricNode()
            TestController.NewInstance.RunPsTest("Test-RemoveAzureRmServiceFabricNode");
        }

-        [Fact]
+        [Fact(Skip = "Test is failing and needs a fix from the owning team")]


What is happening here?

The first error.

Can you work with @juhacket to get this test passing? I don't want to submit a PR that could possibly be breaking a cmdlet.

Sure, I have sent a note. But since the test does not use the cmdlets I touched, this PR should not add any new breaking change and I guess the test might already be failing in the merged changes.

I am more worried about the bump to the network version breaking the cmdlet than any changes you made. As I would likely gate the release of the new Network module on this change (and because code complete is not until Monday), I'd like to get this test rerecorded before merging this PR.

maddieclayton · 2018-05-21T18:16:28Z

src/ResourceManager/Sql/Commands.Sql.Test/packages.config

@@ -6,7 +6,7 @@
  <package id="Microsoft.Azure.Gallery" version="2.6.2-preview" targetFramework="net45" />
  <package id="Microsoft.Azure.Graph.RBAC" version="3.2.0-preview" targetFramework="net45" />
  <package id="Microsoft.Azure.Management.Authorization" version="2.0.0" targetFramework="net45" />
-  <package id="Microsoft.Azure.Management.Network" version="19.0.0-preview" targetFramework="net452" />
+  <package id="Microsoft.Azure.Management.Network" version="19.0.1-preview" targetFramework="net452" />
  <package id="Microsoft.Azure.Management.Resources" version="2.20.0-preview" targetFramework="net45" />
  <package id="Microsoft.Azure.Management.Sql" version="1.13.0-preview" targetFramework="net452" />
  <package id="Microsoft.Azure.Management.Storage" version="2.4.0-preview" targetFramework="net45" />


Please remove/revert changes to the files after this:
src/StackAdmin/AzureRM/AzureRM.psm1
src/StackAdmin/AzureStack/AzureStack.psm1
tools/AzureRM/AzureRM.psm1
tools/LocalFeed/Microsoft.Azure.Management.Network.19.0.1-preview.nupkg
tools/LocalFeed/Microsoft.Azure.Management.Network.19.0.1-preview.symbols.nupkg

Ping - these files need to be reverted or deleted before the PR is accepted.

I shall do these after all changes are accepted. As doing these will cause the build to break.

It will not cause the build to break - they are created during the msbuild, but reverting them will not cause the build to break.

…to one

maddieclayton · 2018-05-22T18:29:28Z

All changes are approved - this will be merged once the ServiceFabric changes have been merged.

maddieclayton · 2018-05-24T05:31:46Z

@sayghosh If you get a moment today, can you resolve the merge conflicts above? If you don't have time, I'll do it tomorrow, but would like to get that done ASAP so we can run final tests and merge.

sayghosh · 2018-05-24T06:19:14Z

@maddieclayton I am working on them and updating the PR after a quick validation on my side.

maddieclayton · 2018-05-24T17:45:09Z

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

sayghosh · 2018-05-24T19:05:40Z

@azuresdkci test this please

sayghosh · 2018-05-24T19:20:30Z

@maddieclayton Again some test falied which is unrelated:

Retriggered the build anyhow.

sayghosh · 2018-05-24T19:34:54Z

@maddieclayton On demaind build succeeded: https://azuresdkci.westus2.cloudapp.azure.com/view/PowerShell/job/powershell-demand/879/

… with existing networkwatcher flowlog parameters (Azure#6253) * Powershell changes satge 1 * Some small changes in required assembly list * Basic commandlets working * Basic implementations done * Implementatioan and test file changes * Workspace object usage coded * Changes done * Optional and mandatory variables sorted * Version updates * Updating test * Reverting changes in service fabric test and squashing all commits into one * Reverting service fabric changes * Adding help generation text * Cleaning the signatureIssues.csv file * Pushing minor string update to retrigger build * Revising the service fabric changes

sayghosh added 11 commits May 9, 2018 13:45

Powershell changes satge 1

046dbee

Some small changes in required assembly list

433c297

Basic commandlets working

8ec0fa8

Basic implementations done

fb4655c

Implementatioan and test file changes

3d1a7ef

Workspace object usage coded

868b861

Changes done

865082d

Optional and mandatory variables sorted

3cc698a

Version updates

e3b9110

Resolving conflicts

2f1b9ed

Updating test

ca77eed

azuresdkci added the needs-review label May 18, 2018

sayghosh force-pushed the preview branch 2 times, most recently from 9f31554 to a61494d Compare May 19, 2018 12:58

sayghosh force-pushed the preview branch 2 times, most recently from c95e351 to ebc8abb Compare May 20, 2018 05:13

EvgenyAgafonchikov previously approved these changes May 21, 2018

View reviewed changes

EvgenyAgafonchikov requested a review from MiYanni May 21, 2018 10:16

maddieclayton added the Cmdlet Review Required ⚠️ label May 21, 2018

maddieclayton added Cmdlet Review Complete and removed Cmdlet Review Required ⚠️ labels May 21, 2018

maddieclayton self-assigned this May 21, 2018

maddieclayton suggested changes May 21, 2018

View reviewed changes

maddieclayton added the needs-revision label May 21, 2018

sayghosh force-pushed the preview branch from 232b26c to 2fdfdfe Compare May 22, 2018 17:04

Reverting changes in service fabric test and squashing all commits in…

d53a9df

…to one

sayghosh force-pushed the preview branch 2 times, most recently from 98c6948 to 54ebe15 Compare May 22, 2018 18:25

Reverting service fabric changes

b9a8657

sayghosh force-pushed the preview branch from 54ebe15 to b9a8657 Compare May 22, 2018 18:26

maddieclayton unassigned sayghosh May 22, 2018

maddieclayton removed the Do Not Merge 🚫 label May 22, 2018

maddieclayton previously approved these changes May 22, 2018

View reviewed changes

Resolving conflicts

4aa6eb0

sayghosh dismissed maddieclayton’s stale review via 4aa6eb0 May 24, 2018 08:22

Adding help generation text

5c1eee0

sayghosh force-pushed the preview branch from d813205 to 5c1eee0 Compare May 24, 2018 12:24

sayghosh added 3 commits May 24, 2018 21:35

Cleaning the signatureIssues.csv file

fef626c

Pushing minor string update to retrigger build

19dd13d

Revising the service fabric changes

3e52bb2

maddieclayton approved these changes May 24, 2018

View reviewed changes

maddieclayton added Waiting for On Demand and removed Cmdlet Review Complete needs-revision labels May 24, 2018

maddieclayton merged commit c1a2b35 into Azure:Network-2018-05-01 May 24, 2018

Powershell changes to onboard additional traffic analytics parameters with existing networkwatcher flowlog parameters #6253

Powershell changes to onboard additional traffic analytics parameters with existing networkwatcher flowlog parameters #6253

Uh oh!

Conversation

sayghosh commented May 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Uh oh!

MikhailTryakhov commented May 18, 2018

Uh oh!

sayghosh commented May 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sayghosh commented May 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sayghosh commented May 20, 2018

Uh oh!

maddieclayton commented May 21, 2018

Uh oh!

sayghosh commented May 21, 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!

sayghosh May 22, 2018 • 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

sayghosh commented May 18, 2018 •

edited

Loading

sayghosh commented May 19, 2018 •

edited

Loading

sayghosh commented May 20, 2018 •

edited

Loading

sayghosh May 22, 2018 •

edited

Loading

sayghosh commented May 24, 2018 •

edited

Loading