AP-660: Adds static_files plugin to serve DAG run files by danschmidt5189 · Pull Request #62 · BerkeleyLibrary/mokelumne

danschmidt5189 · 2026-05-09T15:28:36Z

Summary

Adds a static_files plugin to serve files from the ./storage directory (configurable).
Wires that into a new "Files" view of the DAG Run page. These run in sandboxed iFrames which don't allow downloads. To workaround this, the static_files plugin accepts an optional embed query parameter that tells the file server to return Content-Type: text/plain for files that the browser would otherwise force to download (even with Content-Disposition: inline).
Adds unit and e2e tests for the new plugin. The bulk of logic testing is in the unit tests; the e2e just assert that the files are served and require authentication.
Retains the old ./public URL path, but on reflection I think we should just update that to ./storage as well. (As @anarchivist said on the last PR: they're not "public" files.)

Discussion: Moves file dir/URL helpers into the plugin

The methods used to locate served files on disk and by URL were previously part of the mokelumne.utils.helpers package, resulting in some duplication of configuration—you would have to set both MOKELUMNE_PUBLIC_* and the new STATIC_FILES_* variables correctly. I chose to consolidate that into the plugin so there'd be a single source of truth.

Copilot

Pull request overview

Introduces an Airflow FastAPI plugin for serving per-run “static” artifacts from a shared storage mount, and migrates the batch-image summary workflow from the legacy public/ directory approach to the new plugin-backed storage/ layout.

Changes:

Added static_files Airflow plugin (config/helpers/routes + HTML template) with unit and e2e coverage.
Replaced ./public → ./storage host mount and removed public_dir() / public_path_to_url() utilities (and their unit tests).
Updated summarise_job DAG to write outputs under the plugin’s per-run directory and generate URLs via static_path_to_url.

Reviewed changes

Copilot reviewed 13 out of 15 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
`test/unit/test_storage.py`	Removes tests for deleted legacy public storage helpers.
`test/unit/test_static_files.py`	Adds unit tests for path safety, URL building, and route behavior.
`test/e2e/test_static_files.py`	Adds thin e2e checks for plugin discovery and auth gating in Airflow.
`storage/.keep`	Ensures `storage/` exists in-repo for bind-mounting and tests.
`pyproject.toml`	Adds explicit `fastapi`/`httpx` test deps for plugin unit tests.
`plugins/static_files/templates/listing.html`	Adds directory-listing template.
`plugins/static_files/routes.py`	Implements file/directory serving routes with embed-mode MIME handling.
`plugins/static_files/helpers.py`	Adds safe path resolution, per-run dir creation, and path→URL mapping helpers.
`plugins/static_files/config.py`	Introduces plugin configuration via env vars + Airflow config fallback.
`plugins/static_files/__init__.py`	Registers the plugin’s FastAPI app, external view, and macros.
`mokelumne/util/storage.py`	Removes legacy public storage directory helpers.
`mokelumne/dags/summarise_job.py`	Migrates summary output to static-files storage + URL generation.
`mokelumne/dags/notify_user.py`	Removes unused legacy URL helper import after migration.
`docker-compose.yml`	Switches bind mount from `./public` to `./storage`.
`.gitignore`	Ignores `storage/*` while keeping `storage/.keep`; adds `uv.lock`.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

Copilot reviewed 14 out of 16 changed files in this pull request and generated 5 comments.

Copilot

Pull request overview

Copilot reviewed 14 out of 16 changed files in this pull request and generated 6 comments.

awilfox · 2026-05-11T17:50:15Z

        <dl>
-            <dt><a href="{public_path_to_url(proc_path)}">{proc_count} images processed</a></dt>
+            <dt><a href="{static_path_to_url(proc_path, embed=1)}">{proc_count} images processed</a></dt>
            <dd>{proc_count - proc_failures} succeeded, {proc_failures} failed</dd>
-            <dt><a href="{public_path_to_url(fetched_path)}">{fetch_count} images fetched</a></dt>
+            <dt><a href="{static_path_to_url(fetched_path, embed=1)}">{fetch_count} images fetched</a></dt>
            <dd>{fetch_success} succeeded, {fetch_failures} failed</dd>
-            <dt><a href="{public_path_to_url(skipped_path)}">{skip_count} records skipped</a></dt>
+            <dt><a href="{static_path_to_url(skipped_path, embed=1)}">{skip_count} records skipped</a></dt>
            <dd>Records did not match filter criteria</dd>


I think this is a good point.

agreed with copilot here. would these work as as a download in the embedded context if the links had target="_blank" as part of their attributes?

+        raise HTTPException(status_code=403, detail="Path resolves outside the storage root")
+
+    if path.is_dir():
+        index = path / "index.html"


danschmidt5189 · 2026-05-10T21:53:45Z

+from airflow.plugins_manager import AirflowPlugin
+
+from static_files.config import (
+    STATIC_FILES_DAG_RUN_TAB_LABEL,
+    STATIC_FILES_EMBED_PARAM,
+    STATIC_FILES_PLUGIN_NAME,
+    STATIC_FILES_ROOT,
+    STATIC_FILES_ROUTE,
+    STATIC_FILES_URL_PREFIX,
+)
+from static_files.helpers import static_file_path, static_files_root, static_path_to_url
+from static_files.routes import files_router


This isn't an issue and the tests all pass, ignore this.

this doesn't have to happen now, but i'm curious if we could wrap the plugin into part of the mokelumne package... 🤔

awilfox

Did not review most of the DAG changes because they'll need massaging once #61 is merged.

Did not review the test code yet, either.

Otherwise this is my complete review.

awilfox · 2026-05-11T17:48:58Z

    build: !reset
    develop: !reset
    image: ${DOCKER_APP_IMAGE}
-    volumes: !reset


Is there a way we can do &airflow-common-volume-overrides or such so that we don't duplicate this in every container? Just thinking for future maintainability, especially if we either change this volume, or add more in the future.

I attempted a new override but that doesn't work — it doesn't propagate the !reset, so build is retained.

So you can't do !reset + &common-overrides? A shame.

@awilfox It seems like the best you can do is what I just refactored it to do:

image and volumes can be set in the override anchor.

Resetting build and develop still needs to be done per-service.

Compose seems to push you towards "add-only". E.g. compose.yml would contain ONLY elements common to all environments, and we'd layer on compose.dev.yml to add build and develop and compose.ci.yml to add CI-specific volumes. Bigger change, though, not for this PR IMO (or maybe any).

awilfox · 2026-05-11T17:49:06Z

        condition: service_healthy
    entrypoint: /bin/bash
    restart: on-failure:3
+    # Runs at root to chown the storage directory. Necessary in GitHub Actions


Suggested change

# Runs at root to chown the storage directory. Necessary in GitHub Actions

# Runs as root to chown the storage directory. Necessary in GitHub Actions

awilfox · 2026-05-11T17:50:15Z

        <dl>
-            <dt><a href="{public_path_to_url(proc_path)}">{proc_count} images processed</a></dt>
+            <dt><a href="{static_path_to_url(proc_path, embed=1)}">{proc_count} images processed</a></dt>
            <dd>{proc_count - proc_failures} succeeded, {proc_failures} failed</dd>
-            <dt><a href="{public_path_to_url(fetched_path)}">{fetch_count} images fetched</a></dt>
+            <dt><a href="{static_path_to_url(fetched_path, embed=1)}">{fetch_count} images fetched</a></dt>
            <dd>{fetch_success} succeeded, {fetch_failures} failed</dd>
-            <dt><a href="{public_path_to_url(skipped_path)}">{skip_count} records skipped</a></dt>
+            <dt><a href="{static_path_to_url(skipped_path, embed=1)}">{skip_count} records skipped</a></dt>
            <dd>Records did not match filter criteria</dd>


I think this is a good point.

jason-raitz

r+ assuming @awilfox's comments are handled. I don't see anything else glaring at me.
It will be great to have this implemented. Thank you for plumbing the depths of the Airflow plugin docs.

anarchivist

r+; all my comments are clarificatory and non-blocking.

anarchivist · 2026-05-11T21:27:17Z

        <dl>
-            <dt><a href="{public_path_to_url(proc_path)}">{proc_count} images processed</a></dt>
+            <dt><a href="{static_path_to_url(proc_path, embed=1)}">{proc_count} images processed</a></dt>
            <dd>{proc_count - proc_failures} succeeded, {proc_failures} failed</dd>
-            <dt><a href="{public_path_to_url(fetched_path)}">{fetch_count} images fetched</a></dt>
+            <dt><a href="{static_path_to_url(fetched_path, embed=1)}">{fetch_count} images fetched</a></dt>
            <dd>{fetch_success} succeeded, {fetch_failures} failed</dd>
-            <dt><a href="{public_path_to_url(skipped_path)}">{skip_count} records skipped</a></dt>
+            <dt><a href="{static_path_to_url(skipped_path, embed=1)}">{skip_count} records skipped</a></dt>
            <dd>Records did not match filter criteria</dd>


agreed with copilot here. would these work as as a download in the embedded context if the links had target="_blank" as part of their attributes?

anarchivist · 2026-05-11T21:29:02Z

+from airflow.plugins_manager import AirflowPlugin
+
+from static_files.config import (
+    STATIC_FILES_DAG_RUN_TAB_LABEL,
+    STATIC_FILES_EMBED_PARAM,
+    STATIC_FILES_PLUGIN_NAME,
+    STATIC_FILES_ROOT,
+    STATIC_FILES_ROUTE,
+    STATIC_FILES_URL_PREFIX,
+)
+from static_files.helpers import static_file_path, static_files_root, static_path_to_url
+from static_files.routes import files_router


this doesn't have to happen now, but i'm curious if we could wrap the plugin into part of the mokelumne package... 🤔

anarchivist · 2026-05-11T21:30:59Z

+STATIC_FILES_ROUTE = os.getenv('STATIC_FILES_ROUTE', 'files')
+"""Name of the route added to the DAG Run view linked to the file server."""
+
+STATIC_FILES_URL_PREFIX = os.getenv('STATIC_FILES_URL_PREFIX', '/public')


nit: do we still want this fallback to be /public or should it be /storage?

Or even ./files? I left it as public so older files would work, but come to think of it they wouldn't anyway because of the directory restructuring, so I agree that a change of some kind is in order.

anarchivist · 2026-05-11T21:36:03Z

+
+# URL prefix the plugin is registered under in Airflow (see
+# STATIC_FILES_URL_PREFIX in plugins/static_files/config.py).
+PLUGIN_URL_PREFIX = "/public"


same nit i bring up above.

awilfox

r+, though I'm pretty sure this will break the DAG until it is fixed. I could write the small patch and push it to this branch if desired/needed.

- Adds a static_files plugin that serves files from ./storage (configurable). - Adds a "Files" tab to the DagRun page that shows files created for that run. Refactors existing URLs/storage dirs to use that new location. - Extracts Jinja2 templating for the main app into mokelumne.templates. Templates need to be rendered slightly differently for emails vs. on-disk indexes, and this cleans up that code.

danschmidt5189 force-pushed the AP-660-static_files branch 4 times, most recently from 01d3051 to 97efcbf Compare May 10, 2026 14:59

danschmidt5189 requested a review from Copilot May 10, 2026 15:21

Copilot started reviewing on behalf of danschmidt5189 May 10, 2026 15:21 View session

Copilot AI reviewed May 10, 2026

View reviewed changes

Comment thread plugins/static_files/routes.py Outdated

Comment thread mokelumne/plugins/static_files/helpers.py

Comment thread plugins/static_files/__init__.py Outdated

Comment thread mokelumne/dags/summarise_job.py Outdated

danschmidt5189 force-pushed the AP-660-static_files branch from 4f6433b to 5faa082 Compare May 10, 2026 18:42

danschmidt5189 requested a review from Copilot May 10, 2026 18:42

Copilot started reviewing on behalf of danschmidt5189 May 10, 2026 18:42 View session

danschmidt5189 mentioned this pull request May 10, 2026

AP-660: Adds /public viewer #43

Closed

danschmidt5189 changed the title ~~Ap 660 static files~~ AP-660: Adds static_files plugin to serve DAG run files May 10, 2026

Copilot AI reviewed May 10, 2026

View reviewed changes

Comment thread plugins/static_files/helpers.py Outdated

Comment thread plugins/static_files/routes.py Outdated

Comment thread plugins/static_files/__init__.py Outdated

Comment thread mokelumne/plugins/static_files/routes.py Outdated

Comment thread mokelumne/plugins/static_files/routes.py

danschmidt5189 self-assigned this May 10, 2026

danschmidt5189 marked this pull request as ready for review May 10, 2026 18:50

danschmidt5189 requested a review from Copilot May 10, 2026 18:50

Copilot started reviewing on behalf of danschmidt5189 May 10, 2026 18:50 View session

Copilot AI reviewed May 10, 2026

View reviewed changes

danschmidt5189 force-pushed the AP-660-static_files branch from 5605f5e to 4e40811 Compare May 10, 2026 22:01

awilfox reviewed May 11, 2026

View reviewed changes

jason-raitz approved these changes May 11, 2026

View reviewed changes

anarchivist approved these changes May 11, 2026

View reviewed changes

danschmidt5189 force-pushed the AP-660-static_files branch 2 times, most recently from 0112820 to b339299 Compare May 12, 2026 23:26

awilfox approved these changes May 13, 2026

View reviewed changes

danschmidt5189 force-pushed the AP-660-static_files branch 2 times, most recently from 9044d3f to f96f2b7 Compare May 13, 2026 18:04

danschmidt5189 force-pushed the AP-660-static_files branch from f96f2b7 to af29ca0 Compare May 13, 2026 18:13

danschmidt5189 merged commit 9303280 into main May 13, 2026
5 checks passed

danschmidt5189 deleted the AP-660-static_files branch May 13, 2026 18:29

	# Runs at root to chown the storage directory. Necessary in GitHub Actions
	# Runs as root to chown the storage directory. Necessary in GitHub Actions

Conversation

danschmidt5189 commented May 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Discussion: Moves file dir/URL helpers into the plugin

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

awilfox left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danschmidt5189 May 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jason-raitz left a comment

Choose a reason for hiding this comment

Uh oh!

anarchivist left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

awilfox left a comment

Choose a reason for hiding this comment

danschmidt5189 commented May 9, 2026 •

edited

Loading

danschmidt5189 May 13, 2026 •

edited

Loading