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.

Sign up for GitHub

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

Fix Docker and upgrade Node #415

Merged
merged 35 commits into from
Sep 25, 2024
Merged

Fix Docker and upgrade Node #415

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
3c64705
Fix Yarn and Docker
gabalafou Sep 4, 2024
7e66225
The actual changes needed
gabalafou Sep 4, 2024
3bf256a
revert yarn version change
gabalafou Sep 5, 2024
a2b6ca2
add comment to environment_dev.yml
gabalafou Sep 5, 2024
5afbab2
revert changes to yarn.lock
gabalafou Sep 5, 2024
b28e273
add --immutable flag to yarn install in Dockerfile
gabalafou Sep 5, 2024
53a4f3e
leave quotes
gabalafou Sep 5, 2024
ebd1be5
update GitHub workflows
gabalafou Sep 5, 2024
dc866d2
update README
gabalafou Sep 5, 2024
596ebdd
Merge branch 'main' into main
gabalafou Sep 5, 2024
1aa51c0
Update environment_dev.yml
gabalafou Sep 5, 2024
0e6ee65
Update Dockerfile
gabalafou Sep 5, 2024
2b1c742
Update Dockerfile
gabalafou Sep 5, 2024
ab76abb
Move corepack enable step up (to hopefully fix failing GitHub CI jobs)
gabalafou Sep 5, 2024
0924268
Update README.md
gabalafou Sep 6, 2024
08f986c
Update .github/workflows/release.yml
gabalafou Sep 6, 2024
0634f9d
Update .github/workflows/build.yml
gabalafou Sep 6, 2024
d8498ea
upgrade node
gabalafou Sep 6, 2024
7fcee93
do not test node 18 (drop support for it)
gabalafou Sep 6, 2024
565a4c3
put alpine back
gabalafou Sep 6, 2024
d40a7cc
make node more specific
gabalafou Sep 6, 2024
9a400b8
Merge remote-tracking branch 'upstream/main' into fix-docker
gabalafou Sep 9, 2024
4aea59e
move corepack enable command to correct place
gabalafou Sep 9, 2024
ef25374
correct list numbering in readme
gabalafou Sep 9, 2024
8ce35b8
Merge branch 'main' into main
gabalafou Sep 13, 2024
8687fd7
Apply suggestions from code review
gabalafou Sep 23, 2024
d93e3a5
Merge branch 'main' into fix-docker
gabalafou Sep 23, 2024
d74bd4d
Update environment_dev.yml
gabalafou Sep 23, 2024
27e15ca
Update Dockerfile
gabalafou Sep 23, 2024
f83e2f9
Update Dockerfile
gabalafou Sep 23, 2024
a2dd673
Update environment_dev.yml
gabalafou Sep 23, 2024
211eafc
Update Dockerfile
gabalafou Sep 23, 2024
d2841b3
correct docker image
gabalafou Sep 23, 2024
daa6589
Update environment_dev.yml
gabalafou Sep 23, 2024
bf85545
Update README.md
gabalafou Sep 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
925 changes: 0 additions & 925 deletions .yarn/releases/yarn-4.4.0.cjs
View file
Open in desktop
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not needed for Yarn classic (v1)

Copy link
Contributor Author

@gabalafou gabalafou Sep 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Using Corepack now, not yarnPath, so this file should be deleted

This file was deleted.

2 changes: 0 additions & 2 deletions .yarnrc.yml
View file
Open in desktop
trallard marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -1,3 +1 @@
nodeLinker: node-modules

yarnPath: .yarn/releases/yarn-4.4.0.cjs
trallard marked this conversation as resolved.
Show resolved Hide resolved
6 changes: 4 additions & 2 deletions Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,16 @@
FROM node:18.18-alpine3.18
gabalafou marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Collaborator

@trallard trallard Sep 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Based on the env descriptors, this will default to 22.x so let's update here

Suggested change
FROM node:18.18-alpine3.18
FROM node:22-alpine3.20

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could it be:

Suggested change
FROM node:18.18-alpine3.18
FROM node:22-alpine3

I think I've also seen in the wild:

Suggested change
FROM node:18.18-alpine3.18
FROM node:22

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is the one I was suggesting https://hub.docker.com/layers/library/node/22-alpine3.20/images/sha256-85235be9f710c1ca817cb67892544b7f0cf994ef05380e8ac0d2ea58dce8a988?context=explore
I messed up when adding the suggestion


RUN corepack enable

WORKDIR /usr/src/app

COPY . .
RUN --mount=type=cache,target=/root/.yarn YARN_CACHE_FOLDER=/root/.yarn \
gabalafou marked this conversation as resolved.
Show resolved Hide resolved
yarn install --network-timeout 600000 && \
yarn install --immutable --network-timeout 600000 && \
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the --immutable flag here as a precaution. The Docker build will fail if the Yarn install process would result in a different yarn.lock file, which could happen for example if two different versions of Yarn are used.

if [[ ! -f ".env" ]]; then mv .env.example .env; fi;



EXPOSE 8000

CMD [ "yarn", "webpack-dev-server", "--port", "8000" ]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This command doesn't actually exist anymore but it was never updated. Probably because the image is never run with this default command but rather a command override is supplied by Docker Compose.

CMD [ "yarn", "run", "start:ui" ]
5 changes: 3 additions & 2 deletions docker-compose.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ services:

conda-store-ui:
build: .
command: "yarn run start:ui --port 8000 --history-api-fallback"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can't actually pass these options this way.

command: "yarn run start:ui"
profiles:
- local-dev
ports:
Expand All @@ -91,7 +91,8 @@ services:
condition: service_healthy
platform: linux/amd64
volumes:
- .:/usr/src/app
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was the real source of the "node_modules state file not found" error, not the Yarn version

- ./src:/usr/src/app/src
- ./style:/usr/src/app/style
Comment on lines +94 to +95
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are really the only folders that can be watched for hot-reloading. Mounting everything was causing the .env file (which gets copied in the build step, see the Dockerfile) to go missing.

healthcheck:
test:
["CMD", "curl", "--fail", "http://localhost:8000"]
Expand Down
5 changes: 3 additions & 2 deletions environment_dev.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,9 @@ channels:
- conda-forge
dependencies:
- python=3.10
- yarn>=4.4.0
- nodejs>=18.0
# Match the version of Node.js defined for the Conda dev environment (environment_dev.yml).
gabalafou marked this conversation as resolved.
Show resolved Hide resolved
# Also, do not install Yarn separately, use Corepack (comes with Node.js).
- nodejs=18.18
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
- nodejs=18.18
- nodejs=18.18

but why? package.json has >=18.0 so this is our canonical version. So let's just bump all to >=22.0<23.0 explicitly to avoid mismatch

See https://endoflife.date/nodejs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking about doing that in a follow-up PR, hence:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, just saw your comment on that issue (#416). Since I'm already neck-deep in all of this, I'll go ahead and update Node

- pytest
- pip
- pip:
Expand Down
Loading