fix(ci): stop py311/py312 docker builds from silently matching lean - #42509
fix(ci): stop py311/py312 docker builds from silently matching lean#42509rusackas wants to merge 1 commit into
Conversation
docker.yml's build step appends --build-arg PY_VER=3.11.14-slim-trixie via --extra-flags for every matrix leg, so it can pin dev/lean to the Dockerfile's supported Python version even though supersetbot's own default for those two presets is stale. But docker/buildx keeps the last value for a repeated --build-arg key, and that override was landing after supersetbot's own --build-arg PY_VER for the py311/py312 presets too, clobbering the one thing that's supposed to make those two presets different from lean. All three have been building the same image. Exclude py311/py312 from the override so their own PY_VER pin survives.
|
Bito Automatic Review Skipped - Files Excluded |
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Fixes the CI docker build matrix so the py311/py312 presets no longer get their PY_VER build arg unintentionally overridden (making them silently build the same image as lean).
Changes:
- Builds
EXTRA_FLAGSdynamically to conditionally append thePY_VER=3.11.14-slim-trixieoverride only for non-py311/py312presets - Adds inline documentation explaining the repeated
--build-arg PY_VER“last value wins” behavior and why the exclusion is needed
|
The suggestion to use an allowlist instead of an exclusion list is appropriate. It improves maintainability by ensuring that new presets default to the standard behavior, preventing accidental silent overrides. You can implement this by checking if the preset is in the intended list of presets that require the override. .github/workflows/docker.yml |
SUMMARY
While looking into whether
docker/bake-actionwould help the docker build matrix (dev/lean/websocket/dockerize/py311/py312), I noticed something unrelated to bake:docker.ymlappends--build-arg PY_VER=3.11.14-slim-trixievia--extra-flagsfor every preset in the matrix, so it can pindev/leanto the Dockerfile's actual supported Python version (supersetbot's own default for those two is stale). Butdocker/buildxkeeps the last value for a repeated--build-argkey, and that override was landing after supersetbot's own--build-arg PY_VERforpy311/py312too, silently clobbering the one thing that's supposed to make those two presets different fromlean.lean,py311, andpy312have been building the same image.This excludes
py311/py312from that override so their ownPY_VERpin actually survives.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A, CI-only change.
TESTING INSTRUCTIONS
zizmorandpre-commitboth pass clean. The real test is the docker-build matrix on this PR itself:py311andpy312should now show a differentbase=label thanleanin the "Print docker stats" step (docker history $IMAGE_TAG), where before all three matched.ADDITIONAL INFORMATION
While in here, a few other things came up that I'm treating as separate follow-ups rather than folding into this PR:
docker/bake-actionitself:dev/lean/py311/py312all share one Dockerfile and its ancestor stages (notably the Node/frontend build stage, which doesn't even depend onPY_VER), so building them via onedocker buildx bakeinvocation instead of 4 separate matrix legs would cut real redundant work.websocketanddockerizeare unrelated Dockerfiles with no sharedFROMancestry, so they wouldn't gain anything from being folded in. This is a bigger lift: it means teaching supersetbot to emit a bake file, and it trades runner-level parallelism (today's legs run concurrently on separate runners) for one job doing the shared work once, so it's a compute/cost win more than a guaranteed wall-clock win unless paired with a beefier runner. I'll open that as its own PR against supersetbot.There's already a registry-backed buildx cache (
--cache-from/--cache-toagainstapache/superset-cache:<pyVer>) per preset, so this isn't a from-scratch-every-time situation, just one where shared stages get pulled/pushed redundantly across legs instead of built once.Unrelated oddity I didn't touch: the build step references
$EVENT,$RELEASE, and$FORCE_LATEST, none of which are set anywhere in this workflow. supersetbot falls back to deriving context/ref fromGITHUB_EVENT_NAME/GITHUB_REFwhen they're empty, so this looks harmless as-is, but the dead variables are worth a cleanup pass sometime.Has associated issue:
Required feature flags:
Changes UI
Includes DB Migration
Introduces new feature or API
Removes existing feature or API