GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Add Official Support for Windows Docker Images by zZHorizonZz · Pull Request #2136 · nodejs/docker-node

Conversation

@zZHorizonZz

@zZHorizonZz zZHorizonZz commented Sep 3, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

Description

This PR introduces official support for building windows based docker images for node. At the moment, these images don't include Yarn, I noticed that other PR(s) are pushing to remove it from the Linux images as well, so I followed suit. Although maybe we could enable corepack by default? As currently, based on the discussion in the PR to enable corepack by default in Node itself, it seems like they will be rewriting it because npm has some security concers, so that may take a couple of years.

Motivation and Context

I've seen a few attempts to add windows support over the years, but those PRs seem to have stalled. So, I thought I'd take a look at it. I've combined some useful bits from those previous efforts and added my own improvements to get things working smoothly on newer versions and new build system.

Testing Details

I've successfully built and tested these images locally. Plus, with the new Windows pipeline I added, they've been tested against both Windows Server 2019 and 2022 trough github actions.

Example Output (if appropriate)

None provided

Types of changes

  • Documentation
  • Version change (Update, remove or add more Node.js versions)
  • Variant change (Update, remove or add more variants, or versions of variants)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Other (none of the above)

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING.md document.
  • All new and existing tests passed.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
stormwind85 and iancward reacted with thumbs up emoji
All reactions
  • 👍 2 reactions
@zZHorizonZz
Squash commits
d5e9215
@zZHorizonZz
zZHorizonZz force-pushed the main branch from cb0fb52 to d5e9215 Compare September 3, 2024 10:44
LaurentGoderre
LaurentGoderre reviewed Sep 3, 2024
View reviewed changes
Comment thread Dockerfile-windows.template Resolved

Uh oh!

There was an error while loading. Please reload this page.
zZHorizonZz added 2 commits September 4, 2024 10:54
@zZHorizonZz
Add Official Support for Windows Docker Images
c77f154
@zZHorizonZz
Add fallback to gpg
880fca8
@zZHorizonZz

zZHorizonZz commented Sep 4, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Author

I see something weird going on in the GitHub action when running the gpg --keyserver command. If I run the build locally, it works, but if it runs in the GitHub action, it's throwing errors:

gpg: keyserver receive failed: Server indicated a failure
gpg: keyserver receive failed: Server indicated a failure
gpg: keyserver receive failed: Server indicated a failure
gpg: keyserver receive failed: Server indicated a failure
gpg: keyserver receive failed: Server indicated a failure
gpg: keyserver receive failed: Server indicated a failure
gpg: keyserver receive failed: Server indicated a failure
gpg: keyserver receive failed: Server indicated a failure
gpg: keyserver receive failed: Server indicated a failure
gpg: keyserver receive failed: Server indicated a failure
gpg: keyserver receive failed: Server indicated a failure
gpg: keyserver receive failed: Server indicated a failure
gpg: Signature made 08/22/24 14:16:19 Coordinated Universal Time
gpg:                using RSA key 890C08DBEAB4DFCF[55](https://github.com/zZHorizonZz/docker-node-windows/actions/runs/10681419694/job/29605009626#step:4:56)5EF4
gpg: Can't check signature: No public key

It almost seems like it's not able to call the servers outside.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
LaurentGoderre
LaurentGoderre reviewed Sep 4, 2024
View reviewed changes
Comment thread Dockerfile-windows.template Resolved

Uh oh!

There was an error while loading. Please reload this page.
LaurentGoderre
LaurentGoderre reviewed Sep 4, 2024
View reviewed changes
Comment thread Dockerfile-windows.template Resolved

Uh oh!

There was an error while loading. Please reload this page.
zZHorizonZz added 3 commits September 5, 2024 07:03
@zZHorizonZz
Added verbose logging and sha256 checksum for gpg
788df2f
@zZHorizonZz
Added link to gpg key and remove verbose logging
47843c9
@zZHorizonZz
Updated update.sh to allow skiping everything besides windows
537bc56
@zZHorizonZz

zZHorizonZz commented Sep 5, 2024

Copy link
Copy Markdown
Author

I see something weird going on in the GitHub action when running the gpg --keyserver command. If I run the build locally, it works, but if it runs in the GitHub action, it's throwing errors:

gpg: keyserver receive failed: Server indicated a failure
gpg: keyserver receive failed: Server indicated a failure
gpg: keyserver receive failed: Server indicated a failure
gpg: keyserver receive failed: Server indicated a failure
gpg: keyserver receive failed: Server indicated a failure
gpg: keyserver receive failed: Server indicated a failure
gpg: keyserver receive failed: Server indicated a failure
gpg: keyserver receive failed: Server indicated a failure
gpg: keyserver receive failed: Server indicated a failure
gpg: keyserver receive failed: Server indicated a failure
gpg: keyserver receive failed: Server indicated a failure
gpg: keyserver receive failed: Server indicated a failure
gpg: Signature made 08/22/24 14:16:19 Coordinated Universal Time
gpg:                using RSA key 890C08DBEAB4DFCF[55](https://github.com/zZHorizonZz/docker-node-windows/actions/runs/10681419694/job/29605009626#step:4:56)5EF4
gpg: Can't check signature: No public key

It almost seems like it's not able to call the servers outside.

The only thing I can think of is that it's being rate limited on the keyserver side, as the requests are coming from GitHub where I imagine a lot of people are sending requests. But in that case, I don't understand why the Linux build would work.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
LaurentGoderre
LaurentGoderre reviewed Sep 5, 2024
View reviewed changes
Comment thread Dockerfile-windows.template Resolved

Uh oh!

There was an error while loading. Please reload this page.
zZHorizonZz added 3 commits September 6, 2024 06:58
@zZHorizonZz
Remove empty smoke test layer
0547704
@zZHorizonZz
Remove empty smoke test layer
b98f859
@zZHorizonZz
Fix linting errors
48e25c2
@LaurentGoderre

LaurentGoderre commented Sep 9, 2024

Copy link
Copy Markdown
Member

Can you create a docs update PR for this?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
zZHorizonZz added 3 commits September 9, 2024 17:29
@zZHorizonZz
Renamed windows variant to windowsservercore
d5532b8
@zZHorizonZz
Fixed github action after rename
a8724fb
@zZHorizonZz
Fixed github action after rename
dbe54a6
@zZHorizonZz

zZHorizonZz commented Sep 9, 2024

Copy link
Copy Markdown
Author

Can you create a docs update PR for this?

I have updated the README to include a section about the windowsservercore variant.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@zZHorizonZz
Updated README.md to include information about windoservercore images.
e3485a7
@zZHorizonZz
zZHorizonZz force-pushed the main branch from 2aa62eb to e3485a7 Compare September 9, 2024 16:02
@zZHorizonZz

zZHorizonZz commented Sep 10, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Author

@LaurentGoderre

Can you create a docs update PR for this?

Maybe I understand it wrong, but did you mean to update the README or is there some separate documentation I should update? I didn't find any other doc files in this project.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@LaurentGoderre

LaurentGoderre commented Sep 10, 2024

Copy link
Copy Markdown
Member

There's a separate repo for the documentation on Hub but I realise the Node one is out of sync so I will fix that seperatly.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@LaurentGoderre
LaurentGoderre requested review from PeterDaveHello, SimenB and nschonni September 10, 2024 13:42
@LaurentGoderre

LaurentGoderre commented Sep 10, 2024

Copy link
Copy Markdown
Member

I will let the other members review it but LGTM
zZHorizonZz reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
tianon
tianon reviewed Sep 12, 2024
View reviewed changes
Comment thread Dockerfile-windows.template Resolved

Uh oh!

There was an error while loading. Please reload this page.
tianon
tianon reviewed Sep 12, 2024
View reviewed changes
Comment thread Dockerfile-windows.template Resolved

Uh oh!

There was an error while loading. Please reload this page.
tianon
tianon reviewed Sep 12, 2024
View reviewed changes
Comment thread Dockerfile-windows.template Resolved

Uh oh!

There was an error while loading. Please reload this page.
LaurentGoderre
LaurentGoderre reviewed Sep 19, 2024
View reviewed changes
Comment thread Dockerfile-windows.template Outdated
gpg --batch --decrypt --output SHASUMS256.txt SHASUMS256.txt.asc ; \
Invoke-WebRequest $('https://nodejs.org/dist/v{0}/node-v{0}-win-x64.zip' -f $env:NODE_VERSION) -OutFile 'node.zip' -UseBasicParsing ; \
$sum = $(cat SHASUMS256.txt.asc | sls $(' node-v{0}-win-x64.zip' -f $env:NODE_VERSION)) -Split ' ' ; \
if ((Get-FileHash node.zip -Algorithm sha256).Hash -ne $sum[0]) { Write-Error 'SHA256 mismatch' } ; \

@LaurentGoderre LaurentGoderre Sep 19, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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.

@zZHorizonZz to meet what @tianon said, I think you could change these 2 line to be

if ((Get-FileHash node.zip -Algorithm sha256).Hash -ne $NODE_CHECKSUM) { Write-Error 'SHA256 mismatch' } ; \

The reason why this is better is that the images get rebuilt when the base image changes so if the server was compromised, the key would be changed at the same time as the archive. This means that the arcgive can't change or rebuilds would fail.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@zZHorizonZz zZHorizonZz Sep 20, 2024

Copy link
Copy Markdown
Author

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.

@LaurentGoderre It's not really just about changing these two lines. If we check the checksum, we don't really need GPG or multistage, which makes it simpler. So, what I'll do is remove the GPG installation and SHA256 download and replace it with verification through the NODE_CHECKSUM variable. I'll also remove the multistage as that's not necessary if we're going this way.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@LaurentGoderre LaurentGoderre Sep 20, 2024

Copy link
Copy Markdown
Member

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.

Aren't these two different checks though? One checks it wasn't modified, the other checks it came from the release team

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@zZHorizonZz zZHorizonZz Sep 20, 2024

Copy link
Copy Markdown
Author

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'm starting to get a little bit confused here.

If we change these two lines:

$sum = $(cat SHASUMS256.txt.asc | sls $('  node-v{0}-win-x64.zip' -f $env:NODE_VERSION)) -Split ' ' ; \
    if ((Get-FileHash node.zip -Algorithm sha256).Hash -ne $sum[0]) { Write-Error 'SHA256 mismatch' } ; \

To this:

if ((Get-FileHash node.zip -Algorithm sha256).Hash -ne $NODE_CHECKSUM) { Write-Error 'SHA256 mismatch' } ; 

We validate the checksum against the supplied checksum NODE_CHECKSUM. In the first version, we used the sum extracted from the SHASUMS256.txt.asc file. Maybe I misunderstood you. Did you mean to replace these lines or add a new check but keep the verification from SHASUMS as well? I got confused because @tianon is also discussing whether we should use GPG at all. I looked through the Golang Windows images, and they are also using only one check with a static SHA256.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@LaurentGoderre LaurentGoderre Sep 20, 2024

Copy link
Copy Markdown
Member

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.

Ah yeah, my bad. We might be good to just have the hardcoded checksum and no gpg

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
zZHorizonZz reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
zZHorizonZz added 2 commits September 20, 2024 07:26
@zZHorizonZz
Remove gpg, multistage from windowservercore images and use NODE_CHEC…
79bf2fe 
…KSUM

Signed-off-by: Daniel Fiala <[email protected]>
@zZHorizonZz
Fixed build-test-windows.yml
41d4d9f 
Signed-off-by: Daniel Fiala <[email protected]>
@zZHorizonZz
zZHorizonZz force-pushed the main branch from a88fb68 to 41d4d9f Compare September 20, 2024 06:24
@zZHorizonZz
zZHorizonZz requested review from LaurentGoderre and tianon September 20, 2024 06:29
@zZHorizonZz

zZHorizonZz commented Sep 22, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Author

So since this has been merged, it means that users won't be able to use corepack without installing it (In the future). So do we want to install yarn or not? I personally think the decision on what to use for a package manager should be up to the user. So I would personally not install yarn at all and keep it the way it is, just plain Node.js, or install corepack so that users can then enable the package manager of their choice themselves.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@LaurentGoderre

LaurentGoderre commented Sep 23, 2024

Copy link
Copy Markdown
Member

@zZHorizonZz I personally think it would be good to not include yarn in this variant by default since it's new and wouldn't break anything but I could see people wanting the variants to be consistent. I will let others weigh in on this.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@zZHorizonZz

zZHorizonZz commented Sep 23, 2024

Copy link
Copy Markdown
Author

Yeah, the reason why I'm even discussing this is because of this issue you created, where I see an inclination to not just include yarn even in linux images, or create an image variant with yarn pre-installed.
LaurentGoderre reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@iancward

iancward commented Oct 25, 2024

Copy link
Copy Markdown

Looks like this will address #2063.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@PeterDaveHello
PeterDaveHello requested a review from Copilot December 9, 2024 18:29
Copilot AI reviewed Dec 9, 2024
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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.

Copilot reviewed 3 out of 9 changed files in this pull request and generated 2 suggestions.

Files not reviewed (6)
  • 22/windowsservercore-ltsc2019/Dockerfile: Language not supported
  • 22/windowsservercore-ltsc2022/Dockerfile: Language not supported
  • Dockerfile-windows.template: Language not supported
  • architectures: Language not supported
  • functions.sh: Language not supported
  • update.sh: Language not supported
Comments skipped due to low confidence (1)

genMatrix.js:22

  • The function name 'getNodeVerionDirs' should be 'getNodeVersionDirs'.
const getNodeVerionDirs = (base) => getChildDirectories(base)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread genMatrix.js
];

const nodeDirRegex = /^\d+$/;
// Directories starting with 'windowsservercore-' are excluded from the matrix windows-2019 are excluded for example

Copilot AI Dec 9, 2024

Copy link

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.

[nitpick] The phrase 'windows-2019 are excluded for example' seems to be a typo and should be rephrased for clarity.

Suggested change
// Directories starting with 'windowsservercore-' are excluded from the matrix windows-2019 are excluded for example
// Directories starting with 'windowsservercore-' are excluded; 'windows-2019' is an example

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Copilot uses AI. Check for mistakes.
Comment thread genMatrix.js
.some((file) => testFiles.includes(file));

// Returns a list of the child directories in the given path
// Returns a list of the child directories in the given path, excluding those starting with 'windows-'

Copilot AI Dec 9, 2024

Copy link

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.

[nitpick] The comment should be 'excluding those starting with 'windowsservercore-'.

Suggested change
// Returns a list of the child directories in the given path, excluding those starting with 'windows-'
// Returns a list of the child directories in the given path, excluding those starting with 'windowsservercore-'

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Copilot uses AI. Check for mistakes.
@zZHorizonZz

zZHorizonZz commented Dec 10, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Author

Should I fix the nitpicks that Copilot made? I haven't really used this on GitHub, but there are commit suggestion actions available for both of these.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vaclavholusa

vaclavholusa commented Jan 5, 2025

Copy link
Copy Markdown

I would suggest using nanoserver instead of servercore as it's much lighter..
I'm building my own image using the following code in my dockerfile:

FROM mcr.microsoft.com/windows/nanoserver:ltsc2022 AS base

RUN curl.exe -o node.zip https://nodejs.org/dist/v20.17.0/node-v20.17.0-win-x64.zip && \
  mkdir "C:\\nodejs" && \
  tar.exe -xf node.zip -C "C:\\nodejs" --strip-components=1

ARG NODE_PATH="C:\nodejs"
ENV PATH "${NODE_PATH};C:\Windows\System32"
RUN npm config set registry https://registry.npmjs.org/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vaclavholusa vaclavholusa mentioned this pull request Jan 5, 2025
Images for Windows #2063
Open
@Matthew-Ricks-USBE

Matthew-Ricks-USBE commented Feb 4, 2026

Copy link
Copy Markdown

This PR hasn't been worked on in over a year. What's the process for handling stale PRs? I'm still interested in this, but I don't have the chops to carry it forward. Or has another solution for Windows images been implemented and I'm just unaware?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@MikeMcC399

MikeMcC399 commented May 29, 2026

Copy link
Copy Markdown
Contributor

This PR is very outdated. It's around 482 commits behind the default branch and not surprisingly has multiple conflicts.

Adding Windows images would be a major change and that would need feedback from the current members of the Node.js Docker team.

Moving to draft, pending further review / action.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@MikeMcC399
MikeMcC399 marked this pull request as draft May 29, 2026 06:58
@MikeMcC399

MikeMcC399 commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Closing, as stale, outdated and having conflicts.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@MikeMcC399 MikeMcC399 closed this Jul 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@nschonni nschonni Awaiting requested review from nschonni
@SimenB SimenB Awaiting requested review from SimenB
@PeterDaveHello PeterDaveHello Awaiting requested review from PeterDaveHello
@tianon tianon Awaiting requested review from tianon
@LaurentGoderre LaurentGoderre Awaiting requested review from LaurentGoderre

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

8 participants

@zZHorizonZz @LaurentGoderre @iancward @vaclavholusa @Matthew-Ricks-USBE @MikeMcC399 @tianon
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.