Skip to content

Builds: isolated builders (development environment)#311

Open
humitos wants to merge 17 commits into
mainfrom
humitos/celery-on-ec2
Open

Builds: isolated builders (development environment)#311
humitos wants to merge 17 commits into
mainfrom
humitos/celery-on-ec2

Conversation

@humitos

@humitos humitos commented Jun 15, 2026

Copy link
Copy Markdown
Member

Comment thread dockerfiles/docker-compose.yml
@humitos humitos marked this pull request as ready for review July 14, 2026 08:56
@humitos humitos requested a review from a team as a code owner July 14, 2026 08:56
@humitos humitos requested a review from agjohnson July 14, 2026 08:56
@humitos humitos changed the title Builds: isolated builders Builds: isolated builders (development environment) Jul 14, 2026
humitos added a commit to readthedocs/readthedocs.org that referenced this pull request Jul 14, 2026
Comment thread dockerfiles/docker-compose.yml
# mount as populated and skips ``git clone`` so you can iterate
# on the runner / worker code by editing on the host and
# restarting the container.
- ${PWD}/${RTDDEV_PATH_BUILDER:-../readthedocs-builder}:/opt/readthedocs-builder

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This seems not super consistent, why the different path in /opt/ here? We're using /usr/src/app/ for the app, why not /usr/src/builder/?

Comment on lines +411 to +412
# The entrypoint script itself, bind-mounted so edits don't
# need a rebuild (matches how celery.sh / build.sh wire up).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

How is this different from the normal pattern used in other services in this file?

Speaking generally on what I assume llm generated code, I think this is a case of overly verbose commenting from the llm. I'm noticing similar in a lot of output. Having a lot of comments to wade through actually makes it more confusing and harder work to process. We don't note these things anywhere else in the file, so I need to read through all these comments to figure out what is actually important and different from the rest of the file.

Comment on lines +437 to +444
# Runner venv + managed Python install: host-daemon-recognized
# names of the named volumes defined at the top of this file.
# Compose prefixes volume names with the project (``community``
# for the default community-edition project). The worker passes
# these as ``-v <name>:<path>:ro`` and the daemon resolves them
# as named-volume mounts.
- RTD_HOST_RUNNER_VENV_PATH=community_runner-venv
- RTD_HOST_PYTHON_INSTALL_PATH=community_uv-python

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The comment here isn't super clear. If these are volumes, then why are they referred to as paths? Shouldn't this be:

Suggested change
# Runner venv + managed Python install: host-daemon-recognized
# names of the named volumes defined at the top of this file.
# Compose prefixes volume names with the project (``community``
# for the default community-edition project). The worker passes
# these as ``-v <name>:<path>:ro`` and the daemon resolves them
# as named-volume mounts.
- RTD_HOST_RUNNER_VENV_PATH=community_runner-venv
- RTD_HOST_PYTHON_INSTALL_PATH=community_uv-python
# Runner venv + managed Python install: host-daemon-recognized
# names of the named volumes defined at the top of this file.
# Compose prefixes volume names with the project (``community``
# for the default community-edition project). The worker passes
# these as ``-v <name>:<path>:ro`` and the daemon resolves them
# as named-volume mounts.
- RTD_HOST_RUNNER_VENV_VOLUME=community_runner-venv
- RTD_HOST_PYTHON_INSTALL_VOLUME=community_uv-python

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.

2 participants