Skip to content
This repository has been archived by the owner on Jan 11, 2023. It is now read-only.

Fix regression in Hybrid clusters using custom Vnets #4038

Closed
wants to merge 2 commits into from

Conversation

tariq1890
Copy link
Contributor

Fixes issue #4027

Signed-off-by: tariqibrahim <tariq.ibrahim@microsoft.com>
@acs-bot
Copy link

acs-bot commented Oct 16, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: tariq1890

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@codecov
Copy link

codecov bot commented Oct 16, 2018

Codecov Report

Merging #4038 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #4038   +/-   ##
=======================================
  Coverage   50.42%   50.42%           
=======================================
  Files         109      109           
  Lines       16895    16895           
=======================================
  Hits         8519     8519           
  Misses       7584     7584           
  Partials      792      792

@ghost ghost assigned jackfrancis Oct 16, 2018
@jackfrancis
Copy link
Member

Added param-->var replacement for vmss

Approach makes sense @tariq1890!

{{else}}
"subnetName": "[concat(parameters('orchestratorName'), '-subnet')]",
"vnetID": "[resourceId('Microsoft.Network/virtualNetworks',variables('virtualNetworkName'))]",
"vnetSubnetID": "[concat(variables('vnetID'),'/subnets/',variables('subnetName'))]",
"virtualNetworkName": "[concat(parameters('orchestratorName'), '-vnet-', parameters('nameSuffix'))]",
"virtualNetworkResourceGroupName": "''",
"masterSubnet": "[parameters('masterSubnet')]",
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to translate this to a variable? Why not add a default value of "" for the parameter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like the idea. I think it's much better and cleaner. I followed this because of my limited ARM knowledge and variables to params delegation is a pattern that has already being followed. Thanks for the suggestion!

@jackfrancis FYI

@jackfrancis
Copy link
Member

@CecileRobertMichon @tariq1890 Is this what we want to do instead? #4058

@tariq1890
Copy link
Contributor Author

Yes!

@ghost ghost removed the in progress label Oct 17, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants