Skip to content

Conversation

@henry416
Copy link
Member

@henry416 henry416 commented Jun 18, 2018

Description

Checklist

@henry416
Copy link
Member Author

henry416 commented Jun 18, 2018

@markcowl As discussed with @MikhailTryakhov, I am to push the changes to this Network-2018-06-01 branch which will be later updated with generated SDK and merged into preview branch for the June 29th Powershell release.

#6418

@praries880 praries880 self-requested a review June 20, 2018 00:21
@praries880 praries880 self-assigned this Jun 20, 2018
@praries880
Copy link
Contributor

@henry416 The Jenkins build is breaking because of the following compilation error :

VirtualNetworkGateway\UpdateAzureVirtualNetworkGatewayCommand.cs(82,35): error CS0117: 'VpnClientProtocol' does not contain a definition for 'OpenVPN' [c:\workspace\powershell\src\ResourceManager\Network\Commands.Network\Commands.Network.csproj]
16:17:22 VirtualNetworkGateway\NewAzureVirtualNetworkGatewayCommand.cs(153,35): error CS0117: 'VpnClientProtocol' does not contain a definition for 'OpenVPN' [c:\workspace\powershell\src\ResourceManager\Network\Commands.Network\Commands.Network.csproj]

Are you missing a library reference?

[ValidateSet(
MNM.VpnClientProtocol.SSTP,
MNM.VpnClientProtocol.IkeV2)]
MNM.VpnClientProtocol.IkeV2,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@henry416
@markcowl had the following comment in the related PR : "Change this from a ValidateSet to an ArgumentCompleter" . Kindly address it.

[ValidateSet(
MNM.VpnClientProtocol.SSTP,
MNM.VpnClientProtocol.IkeV2)]
MNM.VpnClientProtocol.IkeV2,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@henry416
@markcowl had the following comment in the related PR : "Change this from a ValidateSet to an ArgumentCompleter" . Kindly address it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@praries880 Will not change, as described in the previous PR. This breaks the existing powershell inputs.

MNM.VpnClientProtocol.SSTP,
MNM.VpnClientProtocol.IkeV2)]
MNM.VpnClientProtocol.IkeV2,
MNM.VpnClientProtocol.OpenVPN)]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems the Models do not have the value OpenVPN, is this something that is not currently released?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@praries880 This is why we are merging to this particular branch instead of preview branch. @MikhailTryakhov will eventually import the updated nuget network SDK into this, which will contain the updated model.

Copy link
Contributor

@MikhailTryakhov MikhailTryakhov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@henry416 to do it we need to push SDK nuget package to local feed not receive broken build.
Again, use documentation I shared, add local SDK to PowerShell and test it in this PR. No need to push untested code here

Link to docs

@henry416
Copy link
Member Author

@MikhailTryakhov Generated the nuget and put it in LocalFeed.

@praries880
Copy link
Contributor

@henry416 you need to add the library version in the netcore csproj as well...

the netcore build is broken :
https://travis-ci.org/Azure/azure-powershell/builds/394784341#L1315

@henry416
Copy link
Member Author

@praries880 Netcore build has been fixed

@henry416
Copy link
Member Author

@praries880 Local build works, not sure why automation is failing...

@praries880
Copy link
Contributor

@henry416 The netcore build/test fail seems to be because the new library you added is not getting loaded

I will check to see if you need to modify some other file to get it to work.

I am looking into what caused the Jenkins tests to fail.

@praries880
Copy link
Contributor

@henry416 for the Jenkins failure, you need to update the reference to the network dll in these projects as well : https://github.com/Azure/azure-powershell/blob/preview/TestMappings.json#L119-L126

@MikhailTryakhov
Copy link
Contributor

@henry416 did you use my scipt? It's described in docs I shared.. It'll cutomize config as it's needed

@henry416
Copy link
Member Author

@MikhailTryakhov script doesn't work, but I have fixed it. I'm looking into why the UT didn't get picked up.

@henry416
Copy link
Member Author

@praries880 @MikhailTryakhov Pushed a test case placement fix, should be green now

@henry416
Copy link
Member Author

@praries880 RepoTasks.Cmdlets.Tests.NewFormatPs1XmlCommandShould.SaveOnlyMarkedPropertiesInSpecifiedViews
What exactly is this test case?

@henry416
Copy link
Member Author

@praries880 @markcowl Could you assist with this error? Local build shows UTs to be fine. Cut off is today.

@maddieclayton
Copy link
Contributor

@azuresdkci Test this please

@henry416
Copy link
Member Author

@markcowl @praries880 @MikhailTryakhov Ready to merge for June 29th release

Copy link
Contributor

@MikhailTryakhov MikhailTryakhov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@praries880
Copy link
Contributor

@praries880 praries880 merged commit 421de37 into Azure:Network-2018-06-01 Jun 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants