-
Notifications
You must be signed in to change notification settings - Fork 21
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
Fix Docker and upgrade Node #415
Conversation
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.
self review
@@ -11,4 +11,4 @@ RUN --mount=type=cache,target=/root/.yarn YARN_CACHE_FOLDER=/root/.yarn \ | |||
|
|||
EXPOSE 8000 | |||
|
|||
CMD [ "yarn", "webpack-dev-server", "--port", "8000" ] |
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.
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.
@@ -81,7 +81,7 @@ services: | |||
|
|||
conda-store-ui: | |||
build: . | |||
command: "yarn run start:ui --port 8000 --history-api-fallback" |
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.
Can't actually pass these options this way.
- ./src:/usr/src/app/src | ||
- ./style:/usr/src/app/style |
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.
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.
yarn.lock
Outdated
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.
Changing the yarn version back three version caused the entire yarn lockfile to be rewritten
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.
In other words, this change is to be expected.
.yarn/releases/yarn-4.4.0.cjs
Outdated
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.
Not needed for Yarn classic (v1)
environment_dev.yml
Outdated
@@ -6,8 +6,7 @@ channels: | |||
- conda-forge | |||
dependencies: | |||
- python=3.10 | |||
- yarn>=4.4.0 | |||
- nodejs>=18.0 | |||
- nodejs=18.18 # do not install Yarn separately, will get Yarn via Corepack |
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.
Follow-ups
In our
I am not a massive fan of alpine-based images, this might have been chosen for size but we could change the base to match the base we have in |
@@ -91,7 +91,8 @@ services: | |||
condition: service_healthy | |||
platform: linux/amd64 | |||
volumes: | |||
- .:/usr/src/app |
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.
This was the real source of the "node_modules state file not found" error, not the Yarn version
.yarn/releases/yarn-4.4.0.cjs
Outdated
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.
Using Corepack now, not yarnPath, so this file should be deleted
WORKDIR /usr/src/app | ||
|
||
COPY . . | ||
RUN --mount=type=cache,target=/root/.yarn YARN_CACHE_FOLDER=/root/.yarn \ | ||
yarn install --network-timeout 600000 && \ | ||
yarn install --immutable --network-timeout 600000 && \ |
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.
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.
@trallard it was good of you to push back against the Yarn downgrade. It wasn't necessary. The real culprit of the bug I was trying to solve was in the I created two follow-up issues: |
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 left a few comments and a couple of nits, but overall this is in a much better place now, thank you!
@@ -141,7 +141,7 @@ | |||
"whatwg-fetch": "^3.6.2" | |||
}, | |||
"engines": { | |||
"node": ">=18.0.0" | |||
"node": ">=20.0.0" |
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.
My only hesitation about pinning on the conda environment, as listed before, is that with hard pins, one has to be quite disciplined about updating them regularly. Otherwise, one misses out on security updates and the like.
I know this is true of the Docker image too, but unless it is infra/core system deps or to avoid hard incompatibilities then it is best to not add a hard pin to major.minor
I think this is pretty much ready but until @pavithraes's PR is merged I will mark this as |
Co-authored-by: Tania Allard <taniar.allard@gmail.com>
Co-authored-by: Tania Allard <taniar.allard@gmail.com>
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.
Could this get another review?
There are two important changes since the last review.
In Dockerfile:
-FROM node:22.8.0-alpine3.20
+FROM node:22-alpine
In Conda environment_dev.yml:
-- nodejs=22.8.0
+- nodejs=22
I decoupled this PR from @pavithraes's PR. Now it points to the current docs page on UI development. These instructions are just as accurate as the current repo readme (which is to say pretty accurate except that there is a missing I left a comment on conda-store#763 that a follow-up task to 763 is to update the link in the conda-store-ui readme. As such, I am going to remove the DO-NOT-MERGE label. |
This PR builds on #412 and does the following:
About the bug and Yarn
My initial pass at this PR tried to downgrade Yarn based on the following false assumption:
I realized that trying to use Yarn 4 was causing a very hard to debug error when trying to run conda-store-ui in Docker, something about node_module state file.Later, I realized that the real source of the error was in the Docker Compose file. First, Docker Compose would build the Docker image according to the Dockerfile, which does
yarn install
, which creates thenode_modules
folder inside the Docker image. Next, Docker Compose would bind-mount the entire repo root directory to the conda-store-ui Docker container, thus undoing theyarn install
step. This is becauseyarn install
was not run locally, so the local directory has no node_modules directory. So then when the local directory is mounted to the container directory, it effectively deletes the node_modules directory.