Skip to content

[Network] VPN Gateway fixes#1474

Merged
tjprescott merged 3 commits intoAzure:masterfrom
tjprescott:VpnGatewayFixes
Dec 1, 2016
Merged

[Network] VPN Gateway fixes#1474
tjprescott merged 3 commits intoAzure:masterfrom
tjprescott:VpnGatewayFixes

Conversation

@tjprescott
Copy link
Member

@tjprescott tjprescott commented Nov 30, 2016

Fixes #1458 and partially addresses #818.

  • Adds convenience arguments to the vpn-gateway update command.
  • Removes --subnet-id and replaces it with --vnet which accepts the name or ID of a VNet. The subnet name is fixed (service requires it to be 'GatewaySubnet'). breaking change
  • Updates tests to check the new convenience arguments for update.

@tjprescott tjprescott added Breaking Change Network az network vnet/lb/nic/dns/etc... labels Nov 30, 2016
@tjprescott tjprescott added this to the Sprint 8 (December Update) milestone Nov 30, 2016
@mention-bot
Copy link

@tjprescott, thanks for your PR! By analyzing the history of the files in this pull request, we identified @derekbekoe to be a potential reviewer.

@tjprescott
Copy link
Member Author

tjprescott commented Nov 30, 2016

cc/ @aanandr

Copy link
Contributor

@yugangw-msft yugangw-msft left a comment

Choose a reason for hiding this comment

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

Suggest 2 minor change. Feel free to merge thereafter.

gateway1_id = '/subscriptions/{}/resourceGroups/{}/providers/Microsoft.Network/virtualNetworkGateways/{}'.format(subscription_id, rg, self.gateway1_name)
gateway2_id = '/subscriptions/{}/resourceGroups/{}/providers/Microsoft.Network/virtualNetworkGateways/{}'.format(subscription_id, rg, self.gateway2_name)

self.cmd('network vpn-connection create -n myconnection -g {} --shared-key 123 --vnet-gateway1-id {} --vnet-gateway2-id {}'.format(rg, gateway1_id, gateway2_id))
Copy link
Contributor

Choose a reason for hiding this comment

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

should we add any verification here?

Copy link
Member Author

Choose a reason for hiding this comment

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

In this case, the command returns nothing, so there's nothing to verify as long as it doesn't fail. I might add more of the VPN-connection commands to this test when I go through that one, but that will involve re-recording so I will hold off for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

sounds good.

register_cli_argument('network vpn-gateway', 'gateway_name', help='Virtual network gateway name')
register_cli_argument('network vpn-gateway', 'gateway_type', help='The gateway type.', **enum_choice_list(gatewayType))
register_cli_argument('network vpn-gateway', 'sku', help='VPN gateway SKU.', **enum_choice_list(sku))
register_cli_argument('network vpn-gateway', 'vpn_type', help='VPN gateway type.', **enum_choice_list(vpnType))
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest rename to vpn_gateway_type. vpn_type could mean site to site or point to site

Copy link
Member Author

Choose a reason for hiding this comment

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

I renamed it for the custom command, but left the fat client template alone for fear that it would require me to re-record the 1-2 hour long test.

Copy link
Contributor

@yugangw-msft yugangw-msft Dec 1, 2016

Choose a reason for hiding this comment

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

@tjprescott, understood. Let us get to it later. I have merged my PR for no-wait, and it will get re-recording faster

@yugangw-msft
Copy link
Contributor

:shipit:

@tjprescott tjprescott merged commit 449598a into Azure:master Dec 1, 2016
@tjprescott tjprescott deleted the VpnGatewayFixes branch December 7, 2016 16:37
@haroldrandom haroldrandom added Breaking Change cla-not-required Network az network vnet/lb/nic/dns/etc... labels Oct 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Breaking Change cla-not-required Network az network vnet/lb/nic/dns/etc...

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants