Skip to content
This repository has been archived by the owner on Oct 24, 2023. It is now read-only.

feat: Updating Windows VHD build files to support building for multiple OS versions #3847

Merged
merged 9 commits into from
Sep 23, 2020

Conversation

marosset
Copy link
Contributor

@marosset marosset commented Sep 18, 2020

Reason for Change:

Issue Fixed:

Credit Where Due:

Does this change contain code from or inspired by another project?

  • No
  • Yes

If "Yes," did you notify that project's maintainers and provide attribution?

  • No
  • Yes

Requirements:

Notes:

@codecov
Copy link

codecov bot commented Sep 18, 2020

Codecov Report

Merging #3847 into master will decrease coverage by 0.37%.
The diff coverage is 25.82%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3847      +/-   ##
==========================================
- Coverage   73.20%   72.83%   -0.38%     
==========================================
  Files         148      149       +1     
  Lines       25428    23171    -2257     
==========================================
- Hits        18614    16876    -1738     
+ Misses       5679     5178     -501     
+ Partials     1135     1117      -18     
Impacted Files Coverage Δ
cmd/update.go 22.72% <22.72%> (ø)
cmd/scale.go 12.63% <44.44%> (+0.93%) ⬆️
cmd/root.go 59.65% <100.00%> (-0.66%) ⬇️
pkg/armhelpers/resourceskus.go 50.00% <0.00%> (-10.00%) ⬇️
pkg/armhelpers/subscriptions.go 50.00% <0.00%> (-10.00%) ⬇️
pkg/armhelpers/azurestack/subscriptions.go 50.00% <0.00%> (-10.00%) ⬇️
pkg/i18n/i18n.go 57.77% <0.00%> (-7.68%) ⬇️
pkg/armhelpers/network.go 42.85% <0.00%> (-7.15%) ⬇️
pkg/armhelpers/azurestack/network.go 42.85% <0.00%> (-7.15%) ⬇️
pkg/api/common/net.go 73.91% <0.00%> (-5.40%) ⬇️
... and 142 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4f4b8e7...a855d41. Read the comment docs.

@marosset marosset changed the title [WIP] - Updating Windows VHD build files to support building for multiple OS versions feat: Updating Windows VHD build files to support building for multiple OS versions Sep 18, 2020
@marosset
Copy link
Contributor Author

I decided to not convert the Windows vhd packer file to use the HashiCorp Config Langauge becasue this is a smaller set of changes that accomplishes the same goals.

"resource_group_name": "{{user `resource_group_name`}}",
"capture_container_name": "aksengine-vhds-windows-ws2019",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this will change from ws2019 to 2019 by default, is that okay?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should be ok. This is only used in the paths in the storage account the VHD gets captured into correct?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah should be fine, we get the URI directly from the output so we don't expect it to be in a specific place 👍

@marosset
Copy link
Contributor Author

Let me revert chagnes tot he scale.go file.. not sure how those got picked up...

@CecileRobertMichon
Copy link
Contributor

it's a github beta feature Unchanged files with check annotations, no changes were pushed, it's just showing them to reviewers

@marosset
Copy link
Contributor Author

First time I saw that. Thanks.

Copy link
Contributor

@CecileRobertMichon CecileRobertMichon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the packer/pipeline parts lgtm

@@ -40,6 +40,7 @@ jobs:
-e BUILD_ID=$(Build.BuildId) \
-e BUILD_NUMBER=$(Build.BuildNumber) \
-e CONTAINER_RUNTIME=$(CONTAINER_RUNTIME) \
-e WINDOWS-SERVER-VERSION=2019 \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should 2019 be an environment variable so it can be configured on the job?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possibly. We still need to have a discussion around what images aks-engine should actually be producing. If we want to produce more than just Windows Server 2019 + Docker I think I would rather just update the pipepline to build all the images as different stages and avoid the need to trigger multiple pipelines.
I'm hoping we can resolve that with a different PR tho.

@jsturtevant
Copy link
Contributor

/lgtm

@acs-bot
Copy link

acs-bot commented Sep 22, 2020

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CecileRobertMichon, jsturtevant, 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:
  • OWNERS [CecileRobertMichon,jsturtevant,marosset]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@marosset marosset merged commit 1a3556e into Azure:master Sep 23, 2020
penggu pushed a commit to penggu/aks-engine that referenced this pull request Oct 28, 2020
…le OS versions (Azure#3847)

* Updating configure-windows-vhd.ps1 to support multiple Windows OS versions

* Updating configured-windows-vhd.ps1 to not use containerd from a personal storage account

* Updating windows packer file to take in windows os version specific vars from a file

* updating windows build pipeline/packer.mk to pass new vars file to packer

* formatting configure-windows-vhd-ps1

* vhd-builder-windows.yaml adding missing WINDOWS_SERVER_VERSION to make run-packer-windows-call and added succeeded condition checks everywher

* fixup! updating windows build pipeline/packer.mk to pass new vars file to packer

* fixup! Updating windows packer file to take in windows os version specific vars from a file

* fixup! Updating windows packer file to take in windows os version specific vars from a file
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants