Refactor: Multi-stage Docker build for app image #2392

thekaveman · 2024-09-20T23:50:10Z

Closes #2389.

Fixes the issue with the dynamic version calculation being incorrect, because of a dirty git index during Docker build time / pip install time.

There's no reason to have the .git directory in the app image, same with all the extra files, tests, .devcontainer, etc.

Uses the approach from eligibility-server: https://github.com/cal-itp/eligibility-server/blob/main/Dockerfile

How to test

Check out this branch locally, ensure you have no pending/unstaged changes (git status)
Apply an annotated tag e.g. git tag -a 2024.10.999
docker compose build --no-cache client to rebuild the app image
docker compose up -d client to start a new app container
docker compose exec -ti client /bin/bash to connect to running container
python manage.py shell to enter a Django shell with settings loaded
import benefits; print(benefits.VERSION) ✅
exit out of everything, back to your local terminal
bin/build.sh to rebuild the devcontainer
Confirm you can launch the devcontainer ✅
And the app ✅

github-actions · 2024-09-20T23:51:15Z

Coverage report

Click to see where and how coverage changed

File	Statements	Missing	Coverage	Coverage (new stmts)	Lines missing
benefits
routes.py
sentry.py
benefits/core
models.py
views.py
benefits/eligibility
views.py
benefits/enrollment
enrollment.py
views.py
benefits/oauth
views.py
Project Total

_{This report was generated by python-coverage-comment-action}

lalver1

I followed all the steps and this worked for me 👍 Just to make sure, I also ran python -m setuptools_scm inside the container to do another confirmation of the version string and got 2024.9.99 ✅

angela-tran · 2024-09-23T16:47:41Z

Reviewing this

angela-tran · 2024-09-23T16:53:14Z

I'm getting an error when trying to rebuild the app image (step 3)

C:\dev\benefits>docker compose build --no-cache client
[+] Building 5.7s (7/19)                                      docker:desktop-linux 
 => [client internal] load build definition from Dockerfile                   0.0s 
 => => transferring dockerfile: 1.23kB                                        0.0s 
 => [client internal] load metadata for ghcr.io/cal-itp/docker-python-web:ma  0.2s 
 => [client internal] load .dockerignore                                      0.0s 
 => => transferring context: 130B                                             0.0s 
 => ERROR [client internal] load build context                                5.4s 
 => => transferring context: 245.64MB                                         5.4s 
 => CACHED [client build_wheel 1/6] FROM ghcr.io/cal-itp/docker-python-web:m  0.0s 
 => CACHED [client build_wheel 2/6] WORKDIR /build                            0.0s 
 => [client build_wheel 3/6] RUN python -m pip install --upgrade pip &&       4.4s 
------
 > [client internal] load build context:
------
failed to solve: archive/tar: unknown file mode ?rwxr-xr-x

I'll look into this more

edit: the issue I'm seeing might be from an issue with Docker for Windows... see docker/for-win#14083. I think it's been a while since I've rebuilt my app image, so it could be that this has been broken for me for a while and I just hadn't run into it yet.

thekaveman · 2024-09-23T17:49:08Z

@angela-tran are you seeing the same issue with the eligibility-server Docker build?

angela-tran · 2024-09-23T18:00:11Z

@angela-tran are you seeing the same issue with the eligibility-server Docker build?

Hmm good point. I am not seeing it... eligibility-server builds just fine for me. And I just switched to main for benefits and the Dockerfile builds fine for me there. 😐

thekaveman · 2024-09-23T19:53:06Z

Moving back to Draft, we'll hold off on this until Slush.

lalver1 · 2024-10-04T18:20:10Z

I have a Windows laptop that I recently updated for other home projects and I thought about trying this PR on it. I was able to build the image (step 3) but had some trouble (unrelated to this PR) running the container due to line endings (even though we have a .gitattributes file in the repo). Just wanted to comment that building the image worked on Windows 10 (10.0.19045) and Docker Desktop 4.34.2 (167172) in case this helps.

angela-tran · 2024-10-08T19:09:42Z

Trying this again

angela-tran · 2024-10-08T19:19:41Z

Hmm, still getting the same error when running it through either Git Bash or just a normal Command Prompt:

Screenshots

I'm using

Edition	Windows 10 Pro
Version	22H2
OS build	19045.4894

and Docker Desktop 4.34.2 (167172).

angela-tran · 2024-10-08T19:30:41Z

😮 I just cloned a new copy of Benefits and tried docker compose build --no-cache client with it on this branch, and it worked!

Must be something about the state of my main local copy of benefits that I normally use for development.

angela-tran

Ran through the testing steps, and it worked for me 🎉

Standalone app container	Devcontainer

fixes issue with version calculation from a dirty git index during Docker build time / pip install time there's no reason to have the .git directory in the app image, same with all the extra files, tests, .devcontainer, etc. same approach as eligibility-server's Dockerfile

since it is no longer available in the base app image

maintainers, more project urls

angela-tran

Standalone app container and devcontainer both showed 2024.10.999 as the version for me ✅

thekaveman · 2024-10-17T01:07:15Z

@lalver1 and/or @machikoyasuda please review on a Mac environment.

lalver1 · 2024-10-17T13:30:19Z

The testing steps worked for me 👍 but I noticed that inside the container, the command python -m setuptools_scm doesn't work anymore

Running pip list confirms it isn't installed but in the previous version of this branch it worked. I'm not sure what may have changed but I guess this may be a problem since we will eventually call setuptools_scm in the deploy workflow?

thekaveman · 2024-10-17T15:18:54Z

@lalver1

but I noticed that inside the container, the command python -m setuptools_scm doesn't work anymore... this may be a problem since we will eventually call setuptools_scm in the deploy workflow?

Can you say more about the concern here? I think we can definitely install this tool in the image, especially if it was working/available before. But I'm not seeing where this is actually needed within a running container?

During Docker build time, in the first phase, setuptools_scm is available/used for the package build process. But then we start from a fresh environment for the second phase, and install the already-built package (which already has its version number locked from prior usage of setuptools_scm).

Or am I missing something?

angela-tran · 2024-10-17T15:22:09Z

The testing steps worked for me 👍 but I noticed that inside the container, the command python -m setuptools_scm doesn't work anymore
...

Running pip list confirms it isn't installed but in the previous version of this branch it worked. I'm not sure what may have changed but I guess this may be a problem since we will eventually call setuptools_scm in the deploy workflow?

Good callout. I think it is expected to not have setuptools_scm in the resulting image anymore because of the change to split the app container's Dockerfile into two stages.

The first stage is for building a benefits Python package and I think uses the build.system dependencies which includes setuptools_scm. Note this stage is named build_wheel -- see AS build_wheel on line 5. Because this build happens with setuptools_scm installed, the built package has the generated version number.

The second stage copies the build artifact from build_wheel and installs it into the image. The image built by the second stage, named appcontainer is what is actually used as the image built by this Dockerfile. The first stage is discarded.

There are more details if you're interested here: https://docs.docker.com/build/building/multi-stage/

lalver1 · 2024-10-17T15:29:45Z

Ah that makes sense, thanks @thekaveman and @angela-tran ! I agree, we don't need setuptools_scm after the package is built so the behavior that I saw was expected 👍

lalver1

Reviewed on a Mac and it works 👍

thekaveman · 2024-10-17T17:24:34Z

Note @lalver1 that setuptools_scm is available within the devcontainer: https://github.com/cal-itp/benefits/blob/refactor/docker-build/pyproject.toml#L34

lalver1 · 2024-10-17T18:51:47Z

I was confused @thekaveman, now I'm sure that for the first review I ran setuptools_scm in the devcontainer, and like you said, this possible because of https://github.com/cal-itp/benefits/blob/refactor/docker-build/pyproject.toml#L34 and

benefits/.devcontainer/Dockerfile

Line 10 in 1b92352

RUN pip install -e .[dev,test]

I just tried it again to check and sure enough, it works and shows the correct version number. Thanks for clearing it up!

thekaveman self-assigned this Sep 20, 2024

github-actions bot added deployment-dev [auto] Changes that will trigger a deploy if merged to dev docker Application container, devcontainer, Compose, etc. labels Sep 20, 2024

thekaveman force-pushed the refactor/docker-build branch from f4d11a6 to 1d65aa5 Compare September 21, 2024 00:00

thekaveman marked this pull request as ready for review September 21, 2024 00:06

thekaveman requested a review from a team as a code owner September 21, 2024 00:06

thekaveman force-pushed the refactor/docker-build branch from 1d65aa5 to a85b2ee Compare September 21, 2024 01:45

lalver1 previously approved these changes Sep 23, 2024

View reviewed changes

thekaveman requested a review from angela-tran September 23, 2024 15:29

thekaveman marked this pull request as draft September 23, 2024 18:54

angela-tran previously approved these changes Oct 8, 2024

View reviewed changes

thekaveman added 5 commits October 15, 2024 14:18

refactor(pyproject): setuptools find benefits subpackages

f1d1ec0

feat(docker): license file in image

bbb4499

fix(devcontainer): copy in all source

2154721

since it is no longer available in the base app image

chore(pyproject): add more project metadata

34ecd8a

maintainers, more project urls

thekaveman dismissed stale reviews from angela-tran and lalver1 via 34ecd8a October 15, 2024 21:33

thekaveman force-pushed the refactor/docker-build branch from a85b2ee to 34ecd8a Compare October 15, 2024 21:33

thekaveman marked this pull request as ready for review October 15, 2024 21:39

thekaveman requested review from lalver1, angela-tran and machikoyasuda October 15, 2024 21:39

angela-tran approved these changes Oct 15, 2024

View reviewed changes

lalver1 approved these changes Oct 17, 2024

View reviewed changes

thekaveman merged commit c569549 into main Oct 17, 2024
12 checks passed

thekaveman deleted the refactor/docker-build branch October 17, 2024 21:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor: Multi-stage Docker build for app image #2392

Refactor: Multi-stage Docker build for app image #2392

thekaveman commented Sep 20, 2024 •

edited

Loading

github-actions bot commented Sep 20, 2024 •

edited

Loading

lalver1 left a comment

angela-tran commented Sep 23, 2024

angela-tran commented Sep 23, 2024 •

edited

Loading

thekaveman commented Sep 23, 2024

angela-tran commented Sep 23, 2024 •

edited

Loading

thekaveman commented Sep 23, 2024

lalver1 commented Oct 4, 2024

angela-tran commented Oct 8, 2024

angela-tran commented Oct 8, 2024 •

edited

Loading

angela-tran commented Oct 8, 2024

angela-tran left a comment

angela-tran left a comment

thekaveman commented Oct 17, 2024

lalver1 commented Oct 17, 2024

thekaveman commented Oct 17, 2024

angela-tran commented Oct 17, 2024 •

edited

Loading

lalver1 commented Oct 17, 2024

lalver1 left a comment

thekaveman commented Oct 17, 2024

lalver1 commented Oct 17, 2024

Refactor: Multi-stage Docker build for app image #2392

Refactor: Multi-stage Docker build for app image #2392

Conversation

thekaveman commented Sep 20, 2024 • edited Loading

How to test

github-actions bot commented Sep 20, 2024 • edited Loading

Coverage report

lalver1 left a comment

Choose a reason for hiding this comment

angela-tran commented Sep 23, 2024

angela-tran commented Sep 23, 2024 • edited Loading

thekaveman commented Sep 23, 2024

angela-tran commented Sep 23, 2024 • edited Loading

thekaveman commented Sep 23, 2024

lalver1 commented Oct 4, 2024

angela-tran commented Oct 8, 2024

angela-tran commented Oct 8, 2024 • edited Loading

angela-tran commented Oct 8, 2024

angela-tran left a comment

Choose a reason for hiding this comment

angela-tran left a comment

Choose a reason for hiding this comment

thekaveman commented Oct 17, 2024

lalver1 commented Oct 17, 2024

thekaveman commented Oct 17, 2024

angela-tran commented Oct 17, 2024 • edited Loading

lalver1 commented Oct 17, 2024

lalver1 left a comment

Choose a reason for hiding this comment

thekaveman commented Oct 17, 2024

lalver1 commented Oct 17, 2024

thekaveman commented Sep 20, 2024 •

edited

Loading

github-actions bot commented Sep 20, 2024 •

edited

Loading

angela-tran commented Sep 23, 2024 •

edited

Loading

angela-tran commented Sep 23, 2024 •

edited

Loading

angela-tran commented Oct 8, 2024 •

edited

Loading

angela-tran commented Oct 17, 2024 •

edited

Loading