GetSupportedVersion returns the right win default #3079
Conversation
ghost
assigned 
| func GetSupportedKubernetesVersion(version string) string { | ||
| if k8sVersion := version; AllKubernetesSupportedVersions[k8sVersion] { | ||
| return k8sVersion | ||
| func GetSupportedKubernetesVersion(version string, hasWindows bool) string { |
There was a problem hiding this comment.
we shouldn't silently override. If a value is passed in we should throw an error if invalid.
There was a problem hiding this comment.
I would tend to agree with you but this PR is intended to fix the bug, not change the behavior of default k8s version definition... The silent override was already in place for getting the k8s version for API version v20170701 so I don't think we want to change the behavior in this PR.
There was a problem hiding this comment.
does validation happen before this so we don't have to worry about it?
There was a problem hiding this comment.
We do validate beforehand. So if I pass in something random as orchestratorVersion such as 0.5 I get FATA[0000] error loading API model in generateCmd: error parsing the api model: OrchestratorProfile has unknown orchestrator version: 0.5
The only case where this function overrides the version is if it is empty or if the cluster is a Windows cluster and the passed in version is valid but not supported for Windows, in which case the user gets default. I looked into fixing validation to also take into account "is this a Windows cluster? If so, is the passed in version valid for Windows?" but it's not straight forward since it requires having the agent pool profiles. I can make that an additional change in this PR if we see value in it.
TLDR: Is it okay to override the version if the value passed in for the user is not supported with windows but is a valid version or should we fail instead?
Codecov Report
@@ Coverage Diff @@
## master #3079 +/- ##
==========================================
+ Coverage 51.5% 51.71% +0.21%
==========================================
Files 99 99
Lines 15036 15031 -5
==========================================
+ Hits 7744 7773 +29
+ Misses 6582 6544 -38
- Partials 710 714 +4
Continue to review full report at Codecov.
|
1.6.11 is not supported for windows
| @@ -649,9 +649,6 @@ func (a *Properties) Validate(isUpdate bool) error { | |||
| } | |||
|
|
|||
| if a.OrchestratorProfile.OrchestratorType == Kubernetes { | |||
| if i == 0 { | |||
| continue | |||
There was a problem hiding this comment.
This was causing a bug where we would not check anything for the first agentpool
| @@ -663,25 +660,13 @@ func (a *Properties) Validate(isUpdate bool) error { | |||
| case Swarm: | |||
| case SwarmMode: | |||
| case Kubernetes: | |||
| var version string | |||
| if a.HasWindows() { | |||
There was a problem hiding this comment.
No need to check if the cluster has a Windows pool since this is run on Windows agents
pkg/api/vlabs/validate.go
Outdated
| a.OrchestratorProfile.OrchestratorVersion, | ||
| false) | ||
| } | ||
| if version == "" { |
There was a problem hiding this comment.
Let's put this empty string check back, and remove the common.AllKubernetesWindowsSupportedVersions[version] check
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jackfrancis 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 |
What this PR does / why we need it: Hotfix for ACS RP, wrong default version was returned for windows (1.8.13, which isn't supported).
Which issue this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close that issue when PR gets merged): fixes #Special notes for your reviewer:
If applicable:
Release note: