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

build: update Dockerfile -- Outdated Node version v14 --> v18 #7610

Merged
merged 6 commits into from
Jun 28, 2023

Conversation

jbool24
Copy link
Contributor

@jbool24 jbool24 commented Jun 13, 2023

node v14 is no longer maintained. Update node to current LTS version.

node v14 is no longer maintained. Update node to current LTS version.
@changeset-bot
Copy link

changeset-bot bot commented Jun 13, 2023

🦋 Changeset detected

Latest commit: eef4f89

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 0 packages

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@netlify
Copy link

netlify bot commented Jun 13, 2023

Deploy Preview for car-park-attendant-cleat-11576 ready!

Name Link
🔨 Latest commit eef4f89
🔍 Latest deploy log https://app.netlify.com/sites/car-park-attendant-cleat-11576/deploys/6499b2a56fff640008aa12bb
😎 Deploy Preview https://deploy-preview-7610--car-park-attendant-cleat-11576.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@jbool24 jbool24 changed the title Update Dockerfile - Outdated Node verions build: update Dockerfile -- Outdated Node version v14 --> v18 Jun 13, 2023
@mmaietta
Copy link
Collaborator

We already build a node 18 image.
https://github.com/electron-userland/electron-builder/blob/master/docker/build.sh#L24
Tag electronuserland/builder:18 (-wine, -wine-mono, -wine-chrome)

Seems the docs need updating though 😅
https://www.electron.build/multi-platform-build#docker

@jbool24
Copy link
Contributor Author

jbool24 commented Jun 14, 2023

@mmaietta ok, thanks Mike. I'll check and update my scripts with that container.

@jbool24
Copy link
Contributor Author

jbool24 commented Jun 14, 2023

@mmaietta should we not update the latest tag with the current Node LTS version? And in reverse make v14 or v16 available as tags

@mmaietta
Copy link
Collaborator

While I agree that we should, I don't think that's a backward-compatible change.

That being said, I'm not sure how to handle semver tagging with docker images. Right now it just uses the month+year+node version due to historical purposes.

What would you advise as the best practice here?

@jbool24
Copy link
Contributor Author

jbool24 commented Jun 16, 2023

@mmaietta I'm confused what you mean. I see in the build.sh its doing multiple tags here but the one I'm concerned with is line 10. You can see that its double tagging v14 as also 'latest' which is the default that will pull if you just use docker pull electronuserland/build that line is the one I think should swap with v18. In essence this file will reverse its order such that the latest LTS release of node should be added to the top and tagged with 'latest'. I can add the proposal to this PR to make more sense what I'm saying. I don't think this causes backwards compat issues unless the end user did what I did and didn't get more specific on my docker pull command.

# Node 14
docker build --build-arg NODE_VERSION=14.19.3 -t electronuserland/builder:14 -t "electronuserland/builder:14-$DATE" -t electronuserland/builder:latest docker/node

Use the reverse ordering so that the LTS is always going to be the image tagged as 'latest' or 'wine'
make latest agree with node Dockerfile
@jbool24
Copy link
Contributor Author

jbool24 commented Jun 16, 2023

With these changes, I'm proposing that we always tag the latest and wine images with the LTS version of Node. End users can always be specific to make sure they are locally using the exact versions as it always was, such as :14 :14-wine or :14-wine-chrome etc

@mmaietta
Copy link
Collaborator

Makes sense, I'm in favor 🙂

Linux tests are failing though with

+ docker build --build-arg NODE_VERSION=18.16.1 -t electronuserland/builder:18 -t electronuserland/builder:18-06.23 docker/node -t electronuserland/builder:latest docker/node
"docker build" requires exactly 1 argument.

docker/node is being provided twice.

@jbool24
Copy link
Contributor Author

jbool24 commented Jun 22, 2023

Makes sense, I'm in favor 🙂

Linux tests are failing though with


+ docker build --build-arg NODE_VERSION=18.16.1 -t electronuserland/builder:18 -t electronuserland/builder:18-06.23 docker/node -t electronuserland/builder:latest docker/node

"docker build" requires exactly 1 argument.

docker/node is being provided twice.

Thanks for the catching that typo 🙏

@mmaietta
Copy link
Collaborator

Alright, finally got the docker images building (something weird with unsafe-perm true. I'll merge this now but won't push a release yet.

Going to see if we can update pnpm as well to latest in a separate PR as it requires updating the unit test GHA configs as well.

@mmaietta mmaietta merged commit c1cc2e9 into electron-userland:master Jun 28, 2023
12 checks passed
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.

2 participants