Fixes #3380 - UI asset files not properly copied to container #3381
Conversation
MANIFEST.in
Outdated
| @@ -0,0 +1 @@ | |||
| recursive-include kinto/plugins/admin/build * | |||
There was a problem hiding this comment.
This is now managed here
Lines 54 to 55 in b67c6c3
There was a problem hiding this comment.
Ah, that explains why the index.html file was showing up but nothing else.
pyproject.toml
Outdated
| @@ -52,7 +52,7 @@ main = "kinto:main" | |||
| include = ["kinto*"] | |||
|
|
|||
| [tool.setuptools.package-data] | |||
| "*" = ["*.tpl", "*.sql", "kinto/plugins/admin/build/*", "VERSION"] | |||
| "*" = ["*.tpl", "*.sql", "*.html", "*.css", "*.js", "*.png", "VERSION"] | |||
There was a problem hiding this comment.
kinto/plugins/admin/build/*
Did this only include index.html? Would a glob of kinto/plugins/admin/build/**/* work? If so, I think that'd also take care of VERSION
There was a problem hiding this comment.
Found the pattern that works best I think:
plugins/admin/build/**
Dockerfile
Outdated
| FROM python:3.10-slim-bullseye as python-builder | ||
| RUN python -m venv /opt/venv | ||
| RUN apt-get update && apt-get install -y --no-install-recommends build-essential libpq-dev | ||
| RUN apt-get update && apt-get install -y --no-install-recommends build-essential libpq-dev ca-certificates curl |
There was a problem hiding this comment.
Can we change the base container to FROM python:3.10 instead? That way we can get rid of this entire line.
There was a problem hiding this comment.
Good call, makes building faster too.
| COPY scripts/pull-kinto-admin.sh . | ||
| RUN bash pull-kinto-admin.sh |
There was a problem hiding this comment.
It'd be nice to find a way to do this before we COPY kinto/ kinto/. Right now, any time we modify a Kinto source file, we'll have to redownload kinto-admin. We really only need to redownload Kinto Admin when VERSION changes.
Maybe we can pull kinto admin an earlier layer, then copy the files to the right place after we copy the kinto directory
There was a problem hiding this comment.
Made a change to do that, but it's a little janky because it's working around a circular dependency.
There was a problem hiding this comment.
Okay, you can leave it for now then -- since we're just curling, it's much faster that having to do a whole build step.
Dockerfile
Outdated
| ARG KINTO_VERSION=1 | ||
| ENV SETUPTOOLS_SCM_PRETEND_VERSION_FOR_KINTO=${KINTO_VERSION} \ | ||
| PATH="/opt/venv/bin:$PATH" | ||
| # At this stage we only fetch and build all dependencies. | ||
| WORKDIR /pkg-kinto | ||
| COPY constraints.txt . | ||
| COPY pyproject.toml . | ||
| COPY constraints.txt pyproject.toml ./ |
There was a problem hiding this comment.
We could pip install --upgrade pip && pip install -r constraints.txt after this layer, again for cache preservation purposes.
Similar to my other comment, right now, any time we change a Kinto source file, we'll need to upgrade pip, then reinstall all of the dependencies. If we move this step up here, we'll only need to do that when constraints.txt or pyproject.toml change.
|
|
||
| # download and unzip release | ||
| curl -OL "https://github.com/Kinto/kinto-admin/releases/download/${TAG}/kinto-admin-release.tar" | ||
| rm -r ./kinto/plugins/admin/build || echo "admin/build folder doesn't exist yet" |
There was a problem hiding this comment.
| rm -r ./kinto/plugins/admin/build || echo "admin/build folder doesn't exist yet" | |
| rm -rf ./kinto/plugins/admin/build |
Or is this too dangerous?
There was a problem hiding this comment.
If this script was only used for docker I'd be fine with the -f but the echo makes me feel better when running it through make on my machine. I don't think it's really dangerous though.
scripts/pull-kinto-admin.sh
Outdated
| rm -r ./kinto/plugins/admin/build || echo "admin/build folder doesn't exist yet" | ||
| mkdir ./kinto/plugins/admin/build | ||
| tar -xf kinto-admin-release.tar -C ./kinto/plugins/admin/build && rm kinto-admin-release.tar | ||
| echo "$VERSION" > ./kinto/plugins/admin/build/VERSION |
There was a problem hiding this comment.
Maybe write a comment here that after Kinto/kinto-admin@8400176 is deployed, we don't have to do this step?
Fixes #3380 - UI files not properly copied to container.
Also pulling the pre-built single-server files going forward to make things easier.