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

Split CI dependencies and add ./deps:/deps mount. #22934

Merged
merged 2 commits into from
Dec 18, 2024
Merged

Conversation

KevinMind
Copy link
Contributor

@KevinMind KevinMind commented Dec 10, 2024

Fixes: mozilla/addons#15234
Fixes: mozilla/addons#15034

Description

  • Introduced a new script install_deps.py to streamline the installation of pip and npm dependencies based on specified targets.
  • Updated docker-compose.yml to include a new volume for dependencies and adjusted service configurations accordingly.
  • Modified Makefile-docker to incorporate checks for CI dependencies and streamline the update process.

Context

Adds a dependency mount to load dependencies from the host. Additionally optimizes the logic for installing dependencies in one batch.

Which dependencies to run is set during make up and not "mutable" because we pass it to the container via the .env file. to change it, re-run make up with a different value.

Note

You can cheat and install development depenednecies in a production running container is you run the command inside the container shell, where make will accept your OLYMPIA_DEPS=development argument

Testing

The value of OLYMPIA_DEPS defaults to the value of DOCKER_TARGET but can be overriden so there are 4 different scenarios.

Standard development mode

make up DOCKER_TARGET=development

Expect:

  • volume called data_deps is created
  • it is synced to your ./deps folder.
  • containers start up and install prod/dev dependencies.
  • Theoretically this should re-enable tracking in IDE
  • running make check in the container should pass

Dev mode with Prod dependencies

make up DOCKER_TARGET=development OLYMPIA_DEPS=production

Expect:

  • everything from above, but missing development dependencies.
  • pytest will not work

Important

You will see in the logs during make up that the ./deps directory is cleaned. That is because we are running a development image but specifying we want production dependencies. To ensure we don't end up with stale dependencies we have to clean up the directory and freshly install, otherwise some dev dependencies could be left behind. we DON'T do this on local images that are installing development dependencies as we should not need any removed

Prod mode

This should happen if the DOCKER_TARGET is production

make up DOCKER_TARGET=production

Expect:

  • Everything from above AND
  • ./deps directory is cleared and dependencies are freshly installed
  • expect only prod dependencies to be installed
  • verify this with make check and (for example) that debug toolbar is missing on the web
  • no pytest or ruff
  • verify with make check

Prod mode with Dev dependencies

This does not currently work on a running container We set the OLYMPIA_DEPS variable during startup and make does not support passing arguments from the host makefile, into the container, and into the host again, at least not easily.

So to run prod mode with testing, run a production build with dev dependencies.

make up DOCKER_TARGET=production OLYMPIA_DEPS=development

This will build the production image and install the development dependencies at runtime.

Expect:

  • Everything from above AND
  • now you should be able to run pytest and ruff

Checklist

  • Add #ISSUENUM at the top of your PR to an existing open issue in the mozilla/addons repository.
  • Successfully verified the change locally.
  • The change is covered by automated tests, or otherwise indicated why doing so is unnecessary/impossible.
  • Add before and after screenshots (Only for changes that impact the UI).
  • Add or update relevant docs reflecting the changes made.

@KevinMind
Copy link
Contributor Author

@diox we can simplify this if we decide it's okay to add everything in ci.txt to prod.txt but that means those dependencies ship to production. I'm happy either way.

@KevinMind KevinMind requested a review from eviljeff December 10, 2024 21:48
@KevinMind KevinMind changed the title Add deps mount: Split CI dependencies and add ./deps:/deps mount. Dec 10, 2024
@KevinMind KevinMind force-pushed the addons-15066-deps branch 6 times, most recently from f29c0f8 to 7b7cfbb Compare December 11, 2024 17:26
Makefile-docker Outdated Show resolved Hide resolved
Makefile-docker Outdated Show resolved Hide resolved
Makefile-docker Outdated Show resolved Hide resolved
@eviljeff
Copy link
Member

@KevinMind CI is failing with ERROR: failed to solve: failed to compute cache key: failed to calculate checksum of ref d292b4cc-acf7-406d-b50f-82233a76aea4::5khdhdz6z7m0rspdr4p5sj9wn: "/requirements/ci.txt": not found

@KevinMind
Copy link
Contributor Author

@KevinMind CI is failing with ERROR: failed to solve: failed to compute cache key: failed to calculate checksum of ref d292b4cc-acf7-406d-b50f-82233a76aea4::5khdhdz6z7m0rspdr4p5sj9wn: "/requirements/ci.txt": not found

I split off the second part of this patch as it was not strictly necessary and left that line. It's gone now.

@KevinMind KevinMind requested a review from eviljeff December 11, 2024 19:24
@KevinMind KevinMind force-pushed the addons-15066-deps branch 5 times, most recently from 3446792 to 4c38966 Compare December 11, 2024 22:28
@KevinMind KevinMind marked this pull request as draft December 12, 2024 12:18
@KevinMind KevinMind force-pushed the addons-15066-deps branch 5 times, most recently from 6759637 to 54107e7 Compare December 12, 2024 17:11
@KevinMind KevinMind marked this pull request as ready for review December 12, 2024 18:24
@eviljeff
Copy link
Member

I just did make up and got

Error response from daemon: failed to mount local volume: mount /run/desktop/mnt/host/wsl/docker-desktop-bind-mounts/Ubuntu/3827bdbcbfb76834bf7a50fd8746e67e97d66dbb404c4ef6a62eef6bf805c55f:/var/lib/docker/volumes/addons-server_data_deps/_data, flags: 0x1000: no such file or directory

(I didn't do anything from the testing section, to check the scenario for a typical use just after the patch has been merged onto master) (edited)

@KevinMind KevinMind force-pushed the addons-15066-deps branch 3 times, most recently from 605615f to bc34f2d Compare December 16, 2024 13:59
@KevinMind
Copy link
Contributor Author

@eviljeff can you try now? I added a .gitkeep in the ./deps directory. If that dir doesn't exist then docker will bail creating the volume.

Copy link
Member

@eviljeff eviljeff left a comment

Choose a reason for hiding this comment

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

works locally but:

  • I expected DOCKER_TARGET=production to also remove the javascript formatting utilities (it didn't), not just ruff
  • CI is failing

@KevinMind KevinMind marked this pull request as draft December 17, 2024 13:57
@KevinMind KevinMind force-pushed the addons-15066-deps branch 6 times, most recently from fb4da16 to 07223be Compare December 18, 2024 14:52
@KevinMind KevinMind marked this pull request as ready for review December 18, 2024 14:57
@KevinMind KevinMind force-pushed the addons-15066-deps branch 8 times, most recently from 0f7c111 to c59e883 Compare December 18, 2024 19:25
@KevinMind KevinMind force-pushed the addons-15066-deps branch 2 times, most recently from afcdf87 to 62a4562 Compare December 18, 2024 22:11
@KevinMind KevinMind merged commit 91b0721 into master Dec 18, 2024
49 checks passed
@KevinMind KevinMind deleted the addons-15066-deps branch December 18, 2024 22:35
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.

add deps mount for host mounted dependencies [Task]: Fix dependency resolution in <your-favourite-ide>
2 participants