fix: don’t assign distro if image ref is provided #2055
Conversation
| @@ -404,7 +404,7 @@ func (p *Properties) setExtensionDefaults() { | |||
| } | |||
|
|
|||
| func (p *Properties) setMasterProfileDefaults(isUpgrade, isScale bool, cloudName string) { | |||
| if p.MasterProfile.Distro == "" { | |||
| if p.MasterProfile.Distro == "" && p.MasterProfile.ImageRef == nil { | |||
There was a problem hiding this comment.
maybe we should assign a Distro value custom or something like that so it's explicit and doesn't run into unexpected usages of empty distro value.
There was a problem hiding this comment.
I have a preference for using empty string as a more expressive/semantic value of "this cluster is not using any distro configuration"
There was a problem hiding this comment.
@mboersma @devigned @PatrickLang @marosset any thoughts here?
There was a problem hiding this comment.
I think setting to nil is most clear if we want to say "no distro set." But that may have unfortunate ripple effects in the code, so I'm fine with empty string.
There was a problem hiding this comment.
Hmm... might be taste, but I'd vote for either *string to indicate emptiness or just document what "" means. I worry that adding a default that is not a golang idiomatic default value would further complicate. If we were to move down that path, then I think I'd not use a string, but rather a type (an enumeration).
There was a problem hiding this comment.
I don't understand this part
adding a default that is not a golang idiomatic default value would further complicate
Is it non-idiomatic to assign significance to the zero value of a type? (in this case a string, so "")
There was a problem hiding this comment.
I'm sorry for not being clear. "" is the idiomatic default value for string. In contrast, a default of "custom" would not be. If we were to use "custom" as the default value, I would vote we use a named type for Distro rather than a string.
jackfrancis
force-pushed
the
fix-imageref-distro
branch
from
344faa6
to
8a61b74
Compare
Codecov Report
@@ Coverage Diff @@
## master #2055 +/- ##
==========================================
+ Coverage 76.59% 76.59% +<.01%
==========================================
Files 135 135
Lines 20618 20622 +4
==========================================
+ Hits 15792 15796 +4
Misses 3903 3903
Partials 923 923 |
|
We should add a cluster config to use in a jenkins to validate this as well. |
|
@jackfrancis I took another look at this PR and I don't see how it fixes issue #2036. In order to fix it we need to change CSE to skip apt package installs for custom images that aren't using the AKS distro but have those packages pre-installed because they are based on the AKS VHD, does that make sense? |
jackfrancis
force-pushed
the
fix-imageref-distro
branch
from
8a61b74
to
b4d3280
Compare
|
This tested out O.K. using the packer pipeline defined in #2118 🎉 |
|
@CecileRobertMichon I don't quite understand by I'm happy to remove the reference to #2036 if this doesn't address that. (i.e., I'd like to merge this as-is) |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: devigned, 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 |
Reason for Change:
This PR addresses an unresolved collision between
imageReferenceconfiguration and our curated OS images (which are delivered via thedistroconfiguration property). Specifically, unknown outcomes will occur if we associate any specificdistrovalue with a cluster spec that includes a customimageReferenceconfiguration.Includes additional validation to fail fast when
imageReference+distroconfigurations are used.Issue Fixed:
Requirements:
Notes: