Skip to content
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

Use release-keys from git #1507

Closed

Conversation

johanneswuerbach
Copy link

Description

Use release keys from git instead of maintaining a hardcoded list fetched from a key servers, related to #1500

Motivation and Context

Testing Details

Example Output(if appropriate)

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)
  • Others (non of 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.

done \
&& git clone https://github.com/nodejs/release-keys.git \
&& release-keys/cli.sh import \
&& rm -Rf release-keys \
Copy link
Member

Choose a reason for hiding this comment

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

Seems like the clear for the CLI should be called at the end with the other cleanup and remove the repo at that point

Copy link
Contributor

@BenjD90 BenjD90 Jul 1, 2021

Choose a reason for hiding this comment

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

and maybe remove bash (apk del bash) at the end ?

gpg --batch --keyserver hkp://ipv4.pool.sks-keyservers.net --recv-keys "$key" || \
gpg --batch --keyserver hkp://pgp.mit.edu:80 --recv-keys "$key" ; \
done \
&& git clone https://github.com/nodejs/release-keys.git \
Copy link
Member

Choose a reason for hiding this comment

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

Could probably skip the git dep, and maybe curl https://github.com/nodejs/release-keys/archive/refs/heads/master.zip to keep a smaller footprint than all the repo history (even though it's small now)

Copy link
Member

Choose a reason for hiding this comment

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

hardcoding master seems inconvenient, what about just git clone --depth 1?

Copy link
Author

Choose a reason for hiding this comment

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

I found it somehow odd to not pin the commit here or validate the downloaded artefact afterwards, maybe it would make sense to create a releases in https://github.com/nodejs/release-keys? While those are only keys, we are executing a random bash script here.

@@ -1,6 +1,6 @@
FROM alpine:3.11

ENV NODE_VERSION 16.3.0
ENV NODE_VERSION 16.4.0
Copy link
Member

Choose a reason for hiding this comment

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

this change should not be included in this PR

Copy link
Author

Choose a reason for hiding this comment

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

Is there a way to re-generate those files from the template without updating the version? I tested ./update.sh -b, but that just fails.

Copy link
Member

Choose a reason for hiding this comment

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

I'd just revert the changes manually, I don't think our script supports "generate from template, but keep current version"

@nschonni
Copy link
Member

nschonni commented Jul 2, 2021

Thanks @johanneswuerbach ! I'm going to close this from the comments by the Docker Official images folks that this approach isn't what they're looking for upstream. I've opened #1510 to just try a new keyserver address instead

@nschonni nschonni closed this Jul 2, 2021
@johanneswuerbach johanneswuerbach deleted the use-release-keys branch July 2, 2021 14:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants