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

Fix Docker and upgrade Node #415

Merged
merged 35 commits into from
Sep 25, 2024
Merged

Fix Docker and upgrade Node #415

merged 35 commits into from
Sep 25, 2024

Conversation

gabalafou
Copy link
Contributor

@gabalafou gabalafou commented Sep 4, 2024

This PR builds on #412 and does the following:

  • Uses Corepack in GitHub workflows, Dockerfile, and other places to ensure that the version of Yarn specified in package.json is used.
    • Why: Using one version of Yarn in one place, but a different version of Yarn in another can cause various local or CI workflows to fail (dev, release, etc.)
  • Specifies that the minimum supported version of Node.js is now v20 instead of v18
    • Why: Still allows people to install and run the app with 18, but makes it clear that we are only officially supporting versions 20 and above, which reduces potential maintenance burden for us
  • Updates the Node version used across GitHub CI jobs and dev environments to v22
    • Why: this is the latest even version of Node.
  • Changes the Docker Compose file to only mount the folders containing application source code, not the entire repo.
    • Why: This was causing one of the dev setups to fail. The failing setup was the one in which conda-store-ui runs in a Docker container. (See below for explanation.)
  • Deletes environment setup instructions in the README. Points contributors to a future page in the Conde Store docs. I opened a PR for this future docs page (on top of another PR). So this PR should not be merged until both 📝 Update local-setup-ui.md pavithraes/conda-store#6 and [DOC] Update contributing guidelines conda-store#763 are merged.
    • Why: There should be a single-source of truth for the instructions, otherwise it will be easy for them to diverge from each other in the future.

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 the node_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 the yarn install step. This is because yarn 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.

gabalafou added a commit to conda-incubator/conda-store that referenced this pull request Sep 4, 2024
Copy link
Contributor Author

@gabalafou gabalafou left a 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" ]
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.

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

Comment on lines +94 to +95
- ./src:/usr/src/app/src
- ./style:/usr/src/app/style
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.

yarn.lock Outdated
Copy link
Contributor Author

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

Copy link
Contributor Author

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.

.yarnrc.yml Show resolved Hide resolved
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)

@@ -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
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@trallard
Copy link
Collaborator

trallard commented Sep 4, 2024

Follow-ups

The Dockerfile implicitly uses the version of Yarn that comes bundled with the node:18.18-alpine3.18 image, which is Yarn v1.22.19.

In our package.json we have "node": ">=18.0.0" so it is perhaps a good idea to bump the base image to [22.8.0-alpine3.20](https://hub.docker.com/layers/library/node/22.8.0-alpine3.20/images/sha256-85235be9f710c1ca817cb67892544b7f0cf994ef05380e8ac0d2ea58dce8a988?context=explore) if we want to stick to alpine images. But see below...

So I decided it will make things simpler if we just use Yarn 1.22.19.

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 conda-store to provide consistency across both setups and that removes the need to downgrade yarn here.

@@ -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

Copy link
Contributor Author

@gabalafou gabalafou Sep 5, 2024

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

.yarnrc.yml Show resolved Hide resolved
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 && \
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.

@gabalafou
Copy link
Contributor Author

@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 volumes key in the Docker Compose file (see diff).

I created two follow-up issues:

  1. [ENH] - Bump to latest Node.js #416
  2. Match Docker base image with conda-store repo #417

@gabalafou gabalafou changed the title Fix Yarn and Docker Fix Docker Sep 5, 2024
environment_dev.yml Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
@gabalafou gabalafou linked an issue Sep 10, 2024 that may be closed by this pull request
Copy link
Collaborator

@trallard trallard left a 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!

.github/workflows/build.yml Outdated Show resolved Hide resolved
.github/workflows/release.yml Outdated Show resolved Hide resolved
environment_dev.yml Outdated Show resolved Hide resolved
@@ -141,7 +141,7 @@
"whatwg-fetch": "^3.6.2"
},
"engines": {
"node": ">=18.0.0"
"node": ">=20.0.0"
Copy link
Collaborator

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

@trallard
Copy link
Collaborator

I think this is pretty much ready but until @pavithraes's PR is merged I will mark this as do not merge

Dockerfile Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
environment_dev.yml Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
environment_dev.yml Outdated Show resolved Hide resolved
Copy link
Contributor Author

@gabalafou gabalafou left a 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

README.md Outdated Show resolved Hide resolved
@gabalafou
Copy link
Contributor Author

gabalafou commented Sep 23, 2024

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 yarn install step in the Docker instructions that actually shouldn't be there because it's really the Docker Compose config that needs to be fixed and which this PR does fix, the issue with bind-mounting the entire repo directory versus the two directories with watchable source files).

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.

@trallard trallard merged commit f79368f into conda-incubator:main Sep 25, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ENH] - Bump to latest Node.js
2 participants