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

Add Node debian core variant #2058

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

LaurentGoderre
Copy link
Member

@LaurentGoderre LaurentGoderre commented Apr 2, 2024

Add debian core variant without npm or yarn

  • 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.

@LaurentGoderre LaurentGoderre force-pushed the node-core branch 12 times, most recently from 77aa4ec to f6cf994 Compare April 2, 2024 19:39
@LaurentGoderre LaurentGoderre force-pushed the node-core branch 2 times, most recently from 8bf831f to 1205b97 Compare April 3, 2024 15:22
@LaurentGoderre
Copy link
Member Author

Note to reviewer, only check the top commit since the other one is the security update. I rebased on top of it to validate it would work when the base image (the slim variant) didn't already exist.

.gitattributes Outdated Show resolved Hide resolved
@PeterDaveHello
Copy link
Member

Do we also need an opinion from @tianon?

@hulkish

This comment was marked as off-topic.

@tianon
Copy link
Contributor

tianon commented Apr 9, 2024

"slim" is already an established convention in official images (with some slight variance for whether it's "FROM slim" or a slimmed image itself), but "core" is not, so I wonder if "core" is already established nomenclature in Node.js communities?

Also, I think it's probably worth updating the templating to not copy docker-entrypoint.sh (which is copied from the "donor" image, not from the build context).

@LaurentGoderre
Copy link
Member Author

LaurentGoderre commented Apr 10, 2024

We are but painted into a corner because slim already contain package manager so we need a different name.

Why not copy the entry point? Do you think it's better to copy from file?

@tianon
Copy link
Contributor

tianon commented Apr 10, 2024

Well, the Dockerfile as written is already copying the entrypoint from the donor image (which is fine, I think), so having it in the build context again is unnecessary and just makes the diff larger without a real purpose. 😄

@LaurentGoderre
Copy link
Member Author

Fixed

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 16 changed files in this pull request and generated no suggestions.

Files not reviewed (13)
  • 18/bookworm-core/Dockerfile: Language not supported
  • 18/bullseye-core/Dockerfile: Language not supported
  • 18/buster-core/Dockerfile: Language not supported
  • 20/bookworm-core/Dockerfile: Language not supported
  • 20/bullseye-core/Dockerfile: Language not supported
  • 20/buster-core/Dockerfile: Language not supported
  • 21/bookworm-core/Dockerfile: Language not supported
  • 21/bullseye-core/Dockerfile: Language not supported
  • Dockerfile-debian-core.template: Language not supported
  • architectures: Language not supported
  • functions.sh: Language not supported
  • update.sh: Language not supported
  • versions.json: Language not supported
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.

5 participants