conda-incubator / conda-store-ui

conda-store-ui is a frontend for conda-store powered by react
https://conda-incubator.github.io/conda-store-ui/
BSD 3-Clause "New" or "Revised" License
14 stars 21 forks source link

Fix Docker and upgrade Node #415

Closed gabalafou closed 2 months ago

gabalafou commented 2 months ago

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

trallard commented 2 months ago

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.

gabalafou commented 2 months ago

@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. https://github.com/conda-incubator/conda-store-ui/issues/416
  2. https://github.com/conda-incubator/conda-store-ui/issues/417
trallard commented 2 months ago

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

gabalafou commented 2 months ago

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.