fix: custom cloud Azure CNI config for Windows #3228
Conversation
parts/k8s/windowsazurecnifunc.ps1
Outdated
| @@ -68,7 +68,7 @@ Set-AzureCNIConfig | |||
| $KubeServiceCIDR, | |||
| [Parameter(Mandatory=$true)][string] | |||
| $VNetCIDR, | |||
| [Parameter(Mandatory=$true)][string] | |||
| [Parameter(Mandatory=$false)][string] | |||
| $TargetEnvironment | |||
There was a problem hiding this comment.
If this is no longer required can you set to a default value by turning the last line into
$TargetEnvironment = ""
There was a problem hiding this comment.
Or maybe we should always pass this and set the value differently - that would clean up the Set-AzureCNIConfig call too i think
Codecov Report
@@ Coverage Diff @@
## master #3228 +/- ##
=======================================
Coverage 71.43% 71.43%
=======================================
Files 147 147
Lines 25653 25653
=======================================
Hits 18324 18324
Misses 6187 6187
Partials 1142 1142
Continue to review full report at Codecov.
|
parts/k8s/kuberneteswindowssetup.ps1
Outdated
| @@ -353,8 +353,7 @@ try | |||
| -KubeClusterCIDR $global:KubeClusterCIDR ` | |||
| -MasterSubnet $global:MasterSubnet ` | |||
| -KubeServiceCIDR $global:KubeServiceCIDR ` | |||
| -VNetCIDR $global:VNetCIDR ` | |||
| -TargetEnvironment $TargetEnvironment | |||
| -VNetCIDR $global:VNetCIDR -IsAzureStack {{if IsAzureStackCloud}}$true{{else}}$false{{end}} | |||
There was a problem hiding this comment.
Can you move the if/else to a variable assignment before the Set-AzureCNICall and all add a comment explaining what is actually going on?
There was a problem hiding this comment.
and revert back to one param per line (for consistency?)
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jackfrancis, marosset 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 |
|
This change fixed #3245 |
Reason for Change:
This PR only adds the
"environment": "mas"Azure CNI config if we're in an Azure Stack custom cloud profile context.Issue Fixed:
Requirements:
Notes: