-
Notifications
You must be signed in to change notification settings - Fork 260
ci: add windows build pool #1073
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
28c59f1 to
ab07c78
Compare
|
/azp run |
|
Azure Pipelines failed to run 2 pipeline(s). |
9be77fb to
9c70186
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s), but failed to run 1 pipeline(s). |
caa3d26 to
e6e3d23
Compare
e6e3d23 to
088fc39
Compare
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
98464ec to
3442788
Compare
80a0462 to
798a12f
Compare
03a1af5 to
306e3b7
Compare
306e3b7 to
3291fcc
Compare
7b787fb to
d4ba7c7
Compare
e49a0ca to
9eea7d0
Compare
eff70ca to
b9d003e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of my comments are minor.
If they make sense, please take them, but I am ok as is.
| name: "$(BUILD_POOL_NAME_DEFAULT_WINDOWS)" | ||
| steps: | ||
| - powershell: | | ||
| powershell.exe -command "& { . .\windows.ps1; azure-npm-image $(tag)-windows-amd64 }" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: naming to clarify what they are?
windows.ps1 -> npm-image-builder.ps1
azure-npm-image -> build-azure-npm-image
| addPipelineData: false | ||
|
|
||
| - powershell: | | ||
| docker tag acnpublic.azurecr.io/azure-npm:$(tag)-windows-amd64 acnpublic.azurecr.io/azure-npm:$(tag)-windows-amd64-test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to declare acnpublic.azurecr.io/azure-npm:$(tag)-windows-amd64 and acnpublic.azurecr.io/azure-npm:$(tag)-windows-amd64-test in Variables fields?
If so, better use variables to avoid multiple changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, but I'd like to leave this as is for the moment, and take that in #1079 since that's the theme for that pr, and this will be refactored again there
| displayName: "Push" | ||
| - powershell: | | ||
| mkdir .\output\images\windows\ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am wondering why we store this image as file?
I think in linux, we also store the image as file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think to release it, when we cut a release on this particular pipeline build these files are what get artifact'ed into binaries you see in github release.
| all-images: azure-npm-image azure-cns-image | ||
| else | ||
| all-binaries: azure-cnm-plugin azure-cni-plugin azure-cns | ||
| all-binaries: azure-cnm-plugin azure-cni-plugin azure-cns azure-npm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems possible to extract it above ifeq ($(GOOS),linux) since now linux and windows build same one.
| @@ -0,0 +1,20 @@ | |||
| FROM golang:windowsservercore-ltsc2022 AS builder | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using golang:windowsservercore-ltsc2022 instead of golang:1.17 is for copying netapi32.dll?
Curious what netapi32.dll does? I guess it is related to HNS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's due to this issue: docker-library/golang#348
klog has a dependency on os/user, so we need to take that dll from servercore to be able to use nanoserver
| $env:NPM_AI_ID = "014c22bd-4107-459e-8475-67909e96edcb" | ||
| $env:NPM_AI_PATH="$env:ACN_PACKAGE_PATH/npm.aiMetadata" | ||
|
|
||
| if ($null -eq $env:VERSION) { $env:VERSION = $args[0] } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is $env:VERSION always null? If not, it seems like that something is wrong.
I am not sure, is it better to return error when it is not null?
Is it slightly better to get above three as argument as well for maintenance?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
version is tuneable via argument so the script can be used when building locally, and tbh arguments are much easier to work with in powershell than env's imo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the other three are very static and should not be changed
Reason for Change:
For building windows based images
Issue Fixed:
Requirements:
Notes: