Ujjwaljain16 commented on code in PR #44248:
URL: https://github.com/apache/superset/pull/44248#discussion_r4015344396
##########
.github/workflows/scheduled-docker-image-refresh.yml:
##########
@@ -151,20 +151,64 @@ jobs:
LATEST_RELEASE: ${{ needs.config.outputs.latest-release }}
FORCE_LATEST_FLAG: ${{ needs.config.outputs.force-latest == '1' &&
'--force-latest' || '' }}
run: |
+ set -euo pipefail
+
# Reuses the same supersetbot invocation as the release
# publisher (`tag-release.yml`), so the resulting tags are
# identical to what a manual release dispatch would produce —
# just with a freshly-pulled base image layer underneath.
# `--force-latest` is only passed when the config job confirmed the
# fetched release is the newest one (see FORCE_LATEST_FLAG above).
+ #
+ # supersetbot's preset -> Dockerfile-target mapping tracks current
+ # master, but this job builds an arbitrary *historical* release ref
+ # (checked out above). A preset introduced after that release won't
+ # have a matching stage in its Dockerfile. That's exactly what broke
+ # here (#44220): `superset` was added as both a new Dockerfile stage
+ # and the preset backing the plain tags (`latest`, `<version>`,
+ # per-SHA) by #44100. Releases cut before it, like 6.1.0, still have
+ # their plain tags built from the pre-existing `lean` stage — that
+ # was the plain tag's source before #44100 split it out (see
+ # docs/admin_docs/installation/docker-builds.mdx as of that commit)
+ # — so `lean` is the correct substitute, not a guess.
+ #
+ # Detect the mismatch by asking supersetbot what it would actually
+ # run (--dry-run) and checking the target stage it picked against
+ # the Dockerfile actually checked out for this release.
+ EXTRA_FLAGS=""
+ DRY_RUN_CMD="$(supersetbot docker \
+ --preset "$BUILD_PRESET" \
+ --context release \
+ --context-ref "$LATEST_RELEASE" \
+ $FORCE_LATEST_FLAG \
+ --platform linux/amd64 \
+ --dry-run)"
+ # `|| true`: under `set -e`/`pipefail`, a `--dry-run` command with no
+ # `--target` in its output (grep matches nothing) would otherwise
+ # abort the whole step right here with no diagnostic at all, rather
+ # than leaving TARGET_STAGE empty so the check below can skip
+ # cleanly.
+ TARGET_STAGE="$(grep -oE -- '--target [A-Za-z0-9_.-]+'
<<<"$DRY_RUN_CMD" | tail -1 | cut -d' ' -f2 || true)"
+
+ if [ -n "$TARGET_STAGE" ] && ! grep -qiE "^[[:space:]]*FROM .* AS
${TARGET_STAGE}\$" Dockerfile; then
Review Comment:
right the previous tests never touched this logic at all i have factored the
target-detection/fallback decision out of the YAML into a pure function in
scripts/docker_release_target.py (same pattern as scripts/change_detector.py:
stdlib-only
no app imports, no subprocess/filesystem access in the core function), with
12 direct pytest cases in
tests/unit_tests/scripts/docker_release_target_test.py covering the lean-only
fallback, the pass-through case, refusal on an unrelated preset/missing-target
mismatch, refusal when no safe substitute exists at all, and a hard failure if
supersetbot's dry-run output ever stops naming a --target that last one closes
a real fragility the shell version had (a set -e/pipefail interaction that
could silently swallow that exact case)
one of the new tests uses the real 6.1.0 stage list and asserts the exact
--target lean --label target=lean string, so it doubles as the #44220
regression test.
tests/unit_tests/scheduled_docker_image_refresh_test.py is untouched it
covers checkout refs, notifier labels, and change-detection, which are separate
concerns from what's being added here
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
