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

Fixes #3380 - UI asset files not properly copied to container #3381

Merged
merged 6 commits into from
Feb 2, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .github/workflows/publish.yml
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,8 @@ jobs:
node --version
npm --version

- name: Build Admin UI
run: make build-kinto-admin
- name: Pull Admin UI
run: make pull-kinto-admin

- name: Install pypa/build
run: python3 -m pip install build
Expand Down
4 changes: 2 additions & 2 deletions .github/workflows/test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -229,8 +229,8 @@ jobs:
which firefox
firefox --version

- name: make build-kinto-admin
run: make build-kinto-admin
- name: make pull-kinto-admin
run: make pull-kinto-admin

- name: Install dependencies
run: make install-dev
Expand Down
17 changes: 6 additions & 11 deletions Dockerfile
Original file line number Diff line number Diff line change
@@ -1,22 +1,18 @@
# Mozilla Kinto server

FROM node:lts-bullseye-slim as node-builder
RUN apt-get update && apt-get install -y --no-install-recommends ca-certificates curl
COPY scripts/build-kinto-admin.sh .
COPY /kinto/plugins/admin ./kinto/plugins/admin
RUN bash build-kinto-admin.sh

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
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we change the base container to FROM python:3.10 instead? That way we can get rid of this entire line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call, makes building faster too.

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 MANIFEST.in ./
COPY kinto/ kinto/

COPY scripts/pull-kinto-admin.sh .
RUN bash pull-kinto-admin.sh
Comment on lines +12 to +13
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made a change to do that, but it's a little janky because it's working around a circular dependency.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.


RUN pip install --upgrade pip && \
pip install ".[postgresql,memcached,monitoring]" -c constraints.txt && \
pip install kinto-attachment kinto-emailer httpie
Expand All @@ -27,7 +23,6 @@ RUN groupadd --gid 10001 app && \
useradd --uid 10001 --gid 10001 --home /app --create-home app

COPY --from=python-builder /opt/venv /opt/venv
COPY --from=node-builder /kinto/plugins/admin/build ./kinto/plugins/admin/build

ENV KINTO_INI=/etc/kinto/kinto.ini \
PORT=8888 \
Expand Down
1 change: 1 addition & 0 deletions MANIFEST.in
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
recursive-include kinto/plugins/admin/build *
Copy link
Contributor

Choose a reason for hiding this comment

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

This is now managed here

kinto/pyproject.toml

Lines 54 to 55 in b67c6c3

[tool.setuptools.package-data]
"*" = ["*.tpl", "*.sql", "*.html", "VERSION"]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, that explains why the index.html file was showing up but nothing else.

9 changes: 3 additions & 6 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ help:
@echo " install-monitoring enable monitoring features like StatsD and Newrelic"
@echo " install-postgres install postgresql support"
@echo " install-dev install dependencies and everything needed to run tests"
@echo " build-kinto-admin build the Kinto admin UI plugin (requires npm)"
@echo " pull-kinto-admin pull the Kinto admin UI plugin (requires npm)"
@echo " serve start the kinto server on default port"
@echo " migrate run the kinto migrations"
@echo " lint run the code linters"
Expand Down Expand Up @@ -67,8 +67,8 @@ $(DOC_STAMP): $(PYTHON) docs/requirements.txt
constraints.txt: constraints.in
pip-compile -o constraints.txt constraints.in

build-kinto-admin: need-npm
scripts/build-kinto-admin.sh
pull-kinto-admin:
scripts/pull-kinto-admin.sh

$(SERVER_CONFIG):
$(VENV)/bin/kinto init --ini $(SERVER_CONFIG)
Expand Down Expand Up @@ -105,9 +105,6 @@ format: install-dev
tdd: install-dev
$(VENV)/bin/ptw --runner $(VENV)/bin/py.test

need-npm:
@npm --version 2>/dev/null 1>&2 || (echo "The 'npm' command is required to build the Kinto Admin UI." && exit 1)

need-kinto-running:
@curl http://localhost:8888/v0/ 2>/dev/null 1>&2 || (echo "Run 'make runkinto' before starting tests." && exit 1)

Expand Down
4 changes: 2 additions & 2 deletions docs/tutorials/install.rst
Original file line number Diff line number Diff line change
Expand Up @@ -316,9 +316,9 @@ Kinto Admin
-----------

In order to run a local `Kinto Admin UI <https://github.com/Kinto/kinto-admin>`_ from a cloned
repo, the ``npm`` command must be available in order to build it with ::
repo, run ::

make build-kinto-admin
make pull-kinto-admin


Go further
Expand Down
2 changes: 1 addition & 1 deletion kinto/plugins/admin/VERSION
Original file line number Diff line number Diff line change
@@ -1 +1 @@
3.0.1
3.0.3
4 changes: 2 additions & 2 deletions kinto/plugins/admin/public/help.html
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ <h2>Build and run locally</h2>
<p>In order to get a local Kinto Admin running at this address, just run the
following command:
</p>
<code>make build-kinto-admin</code>
<code>make pull-kinto-admin</code>
<p>Restart the server and refresh!</p>
</section>
<section>
Expand All @@ -22,4 +22,4 @@ <h2>...or use our online version!</h2>
and set the server address to <em>http://localhost:8888/v1</em>.
</section>
</body>
</html>
</html>
2 changes: 1 addition & 1 deletion kinto/plugins/admin/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ def admin_home_view(request):
This requires the Admin UI to be built with ``ASSET_PATH="/v1/admin/"``.
"""
# Default location of the Admin UI is relative to this plugin source folder,
# as built with the ``make build-kinto-admin`` command.
# as pulled with the ``make pull-kinto-admin`` command.
admin_assets_path = request.registry.settings["admin_assets_path"] or os.path.join(
HERE, "build"
)
Expand Down
29 changes: 0 additions & 29 deletions scripts/build-kinto-admin.sh

This file was deleted.

14 changes: 14 additions & 0 deletions scripts/pull-kinto-admin.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
#!/bin/bash
set -euo pipefail

VERSION=$(cat kinto/plugins/admin/VERSION)
TAG="v${VERSION}"

echo $PWD

# 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"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

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
Loading