feat: Introduce a new Docker environment for development #5355
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 3 Skipped Deployments
|
github-actions
bot
added
api
Docker builds report
|
Uffizzi Ephemeral Environment Deploying☁️ https://app.uffizzi.com/github.com/Flagsmith/flagsmith/pull/5355 ⚙️ Updating now by workflow run 15008393060. What is Uffizzi? Learn more! |
emyller
force-pushed
the
feat/dockerized-local-env
branch
from
2947c16 to
51120a3
Compare
github-actions
bot
added
feature
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5355 +/- ##
=======================================
Coverage 97.63% 97.63%
=======================================
Files 1234 1234
Lines 42992 42992
=======================================
Hits 41974 41974
Misses 1018 1018 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
github-actions
bot
added
feature
There was a problem hiding this comment.
I approve the general idea of making life easier for contributors preferring Docker-based development environments.
That said, I'm not happy at all being faced with a prospect of maintaining:
- A new Dockerfile, completely independent from the current one, making the build toolchain for backend un-DRY again.
- A new task runner separate from the existing Makefile — confusing to contributors and probably redundant.
- A yet another Docker Compose file (though I appreciate it's probably intended to replace one or more of the existing ones).
Overall, I feel that, at this moment, this PR introduces more new boilerplate aimed to solve one specific use case than I can tolerate.
I suggest we take a step back and align ourselves on the initial intent. I'm curious to learn about every roadblock you've encountered when setting up your local dev env, and how they are solved by throwing Docker into the equation.
If we decide that the use case warrants adding new tooling, I will insist on keeping our build toolchain as DRY and as standardised as possible — i.e., introducing a new target stage into the original Dockerfile, integrating Compose with our Make targets, exploring established solutions like devcontainers, devenv.sh, etc. etc.
api/Dockerfile
Outdated
| # NOTE: Why not Alpine? Because Alpine demands installation of OS software | ||
| # prior to running app code, which leads to maintenance work and a larger image | ||
| # size compared to Debian — considering Debian layers are shared across images. |
There was a problem hiding this comment.
I don't understand how having to maintain a new Dockerfile is a viable trade-off here. I'll prefer a unified build toolchain to a neater dev container image; I'd rather this file be a new target stage in the existing root Dockerfile.
There was a problem hiding this comment.
Agreed! For now this Dockerfile is only experimental. It aligns nicely with the proposal but it would have a long way to go before becoming an actual thing.
api/Dockerfile
Outdated
| # Install Python packages | ||
| WORKDIR /tmp | ||
| ARG POETRY_VERSION_CONSTRAINT | ||
| ARG INSTALL_DEV_DEPENDENCIES |
There was a problem hiding this comment.
Why would we want to not install dev dependencies in a development container?
There was a problem hiding this comment.
Good question! The ability to not install dev dependencies allows us to reuse the image setup in other contexts, e.g. production. But then again, this Dockerfile is only experimental.
api/Dockerfile
Outdated
| # NOTE: Forcefully delete optional private dependencies for now since | ||
| # Poetry needs to fetch them in order to calculate the dependency tree. See | ||
| # https://github.com/python-poetry/poetry/issues/4562 | ||
| # TODO: Improve management of enterprise features |
There was a problem hiding this comment.
This is, at least in part, being tracked in #4764 (comment).
api/Dockerfile
Outdated
| COPY api/pyproject.toml api/poetry.lock ./ | ||
| RUN \ | ||
| pip install -q poetry${POETRY_VERSION_CONSTRAINT:-} &&\ | ||
| poetry config virtualenvs.create false &&\ | ||
| poetry config cache-dir /var/cache/pypoetry &&\ | ||
| ### | ||
| # NOTE: Forcefully delete optional private dependencies for now since | ||
| # Poetry needs to fetch them in order to calculate the dependency tree. See | ||
| # https://github.com/python-poetry/poetry/issues/4562 | ||
| # TODO: Improve management of enterprise features | ||
| sed -ie '/tool.poetry.group.auth-controller/,+1d' pyproject.toml &&\ | ||
| sed -ie '/tool.poetry.group.saml/,+1d' pyproject.toml &&\ | ||
| sed -ie '/tool.poetry.group.ldap/,+1d' pyproject.toml &&\ | ||
| sed -ie '/tool.poetry.group.workflows/,+1d' pyproject.toml &&\ | ||
| sed -ie '/tool.poetry.group.licensing/,+1d' pyproject.toml &&\ | ||
| poetry lock &&\ | ||
| ### | ||
| poetry install --no-root $([ "$INSTALL_DEV_DEPENDENCIES" != "true" ] && echo "--without dev") &&\ | ||
| rm -rf /var/cache/pypoetry &&\ | ||
| rm -rf /tmp/* |
There was a problem hiding this comment.
Private dependency problem aside (which, IMO, should not be solved here), why isn't this make install with POETRY_CACHE_DIR set to a mounted path?
There was a problem hiding this comment.
Yeah I'd rather use #4764 to improve private dependency management. This sed trickery only exists to favor this PoC.
why isn't this make install with POETRY_CACHE_DIR set to a mounted path?
Another good question. I can list a few reasons why not to use make install within the Docker build:
- IMO
makeis better suited for persons interacting with the shell directly. Especially, assumingmakecommands are going to run Docker underneath [if we eventually decide in favor of it], I'd rather not introduce inconsistency by having some commands that are intended to run from within a container instead of from the outside. - Currently
make installwill: 1. install pip, 2. install Poetry in a venv, then finally 3. install Python packages.pipis already available in the Python Docker image, there's no need to reinstall.- The Poetry install script will setup a venv and install Poetry. A Docker env has no benefit out of Python virtualenvs, or really anything else this script does besides finally
pip install poetry. - Though currently a necessary step to setup the local env,
make installcould be disregarded entirely thanks to./run— or Docker Compose, really.
- Finally,
RUN --mountto improve build efficiency sounds like a great idea! We'd have to explore that.
docker/new/docker-compose.yml
Outdated
| context: ../../ | ||
| dockerfile: api/Dockerfile | ||
| args: | ||
| POETRY_VERSION_CONSTRAINT: ==2.1.2 |
There was a problem hiding this comment.
I'd rather pin to a specific version (see Makefile).
api/poetry.lock
Outdated
| @@ -1,4 +1,4 @@ | |||
| # This file is automatically @generated by Poetry 2.1.1 and should not be changed by hand. | |||
| # This file is automatically @generated by Poetry 2.1.2 and should not be changed by hand. | |||
There was a problem hiding this comment.
Makefile POETRY_VERSION is still 2.1.1
There was a problem hiding this comment.
Thanks for catching the inconsistency. I tend to upgrade patch versions if they're harmless, but I shouldn't upgrade this yet, not in this PR.
github-actions
bot
added
feature
|
@khvn26 I agree with every comment. The shape of this PR right now is more of a foundation to discuss, rather than an actual patch to merge. I’ve updated the status to Draft accordingly. I’ve introduced a [tiny] new boilerplate as the shortest possible path to validate the PoC as an end. And only if we decide to pursue it to the benefit of users, then most definitely we’ll have to look into the means. This patch barely scratches the surface of the ultimate intent, which is a combination of long-term benefits:
For now, looking at the local environment, the serve:
./run
# Encourage developers to name migrations
MIGRATION_NAME ?= $(shell read -p 'Migration name (snake_case): ' name; echo $$name)
migration:
./run python manage.py makemigrations -n $(MIGRATION_NAME)
docs:
...While I hope that makes sense! Please let me know your thoughts. I will add to the other comments shortly. |
|
If the team decides this is worth pursuing, I'd love to write an RFC and publish it eventually. Likely it would be a WIP lasting as long as it takes until I learn all the ins and outs of the product, unless it gets help from other folks who'd like to join an adventure when the time is right. For now this PR is really nothing but a PoC to discuss the idea of Dockerizing the product from local to production. |
github-actions
bot
added
feature
github-actions
bot
added
feature
github-actions
bot
added
feature
I have added information todocs/if required so people know about the feature!Changes
Introduce a new Docker environment, initially aimed at the local environment.
The design is intended to allow developers to run the code as easily as possible.
In the future, this format might contribute to remote environments as well (needs discussion).
How did you test this code?
./api/run— gets the application running in one command../api/run pytest --sw --pdb api/tests/— or run any other command.cd api; ./run pytest -x tests/— current directory is passed to the container.Ideally any command should work just by prefixing it with
./api/run.