fix: Remove unnecessary transform on VMSS upgrade #1633
Conversation
|
/azp run pr-e2e |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Thanks @yangl900!
==> Running Go linter <==
golangci-lint has version 1.17.1 built from 4ba2155 on 2019-06-10T09:06:49Z
Running inside container
pkg/engine/transform/transform.go:28:2: U1000: const `virtualMachineProfileFieldName` is unused (unused)
virtualMachineProfileFieldName = "virtualMachineProfile"
^
pkg/engine/transform/transform.go:40:2: U1000: const `vmssResourceType` is unused (unused)
vmssResourceType = "Microsoft.Compute/virtualMachineScaleSets"
^ |
|
/azp run pr-e2e |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run pr-e2e |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
==> Running Go linter <== |
Codecov Report
@@ Coverage Diff @@
## master #1633 +/- ##
==========================================
+ Coverage 76.31% 76.36% +0.04%
==========================================
Files 129 129
Lines 18727 18703 -24
==========================================
- Hits 14292 14282 -10
+ Misses 3652 3643 -9
+ Partials 783 778 -5 |
|
added #1638 for tracking. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: CecileRobertMichon, yangl900 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
|
||
| if err = transformer.NormalizeForVMSSScaling(ku.logger, templateMap); err != nil { | ||
| ku.logger.Errorf("unable to update template, error: %v.", err) | ||
| if err = transformer.NormalizeMasterResourcesForScaling(ku.logger, templateMap); err != nil { |
There was a problem hiding this comment.
NormalizeMasterResourcesForScaling doesn't have a unit test and wasn't actually called anywhere (here or in AKS). But NormalizeForVMSSScaling is called in AKS' arminteractor.go. Is the plan to switch that usage as well? (Could we add a unit test? Maybe copy-and-paste from TestNormalizeForVMSSScaling would almost do it.)
There was a problem hiding this comment.
yes, I actually prepared a unit test yesterday, but didn't finish. :) Will send a PR today.
The caller at AKS side is actually ACS only, so it's on deprecation path.
4 participants
Reason for Change:
VMSS upgrade path used a template normalization, which removes OS image reference. This doesn't seem right because we expect OS image version get updated from previous version. Also the scaling normalization is only called by upgrade path, this alone is very weird...
Issue Fixed:
Fixes #1638
Requirements:
Notes: