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.
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
Add Official Support for Windows Docker Images #2136
base: main
Are you sure you want to change the base?
Add Official Support for Windows Docker Images #2136
Changes from 6 commits
d5e9215
c77f154
880fca8
788df2f
47843c9
537bc56
0547704
b98f859
48e25c2
d5532b8
a8724fb
dbe54a6
e3485a7
79bf2fe
41d4d9f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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
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.
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.
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
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:
To this:
We validate the checksum against the supplied checksum
NODE_CHECKSUM
. In the first version, we used the sum extracted from theSHASUMS256.txt.asc
file. Maybe I misunderstood you. Did you mean to replace these lines or add a new check but keep the verification fromSHASUMS
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.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