-
Notifications
You must be signed in to change notification settings - Fork 560
Fix regression in Hybrid clusters using custom Vnets #4038
Conversation
Signed-off-by: tariqibrahim <tariq.ibrahim@microsoft.com>
[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 |
Codecov Report
@@ Coverage Diff @@
## master #4038 +/- ##
=======================================
Coverage 50.42% 50.42%
=======================================
Files 109 109
Lines 16895 16895
=======================================
Hits 8519 8519
Misses 7584 7584
Partials 792 792 |
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')]", |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
@CecileRobertMichon @tariq1890 Is this what we want to do instead? #4058 |
Yes! |
Fixes issue #4027