villebro commented on PR #44250:
URL: https://github.com/apache/superset/pull/44250#issuecomment-5751332348
@sadpandajoe I think this is ready for a decision. Every finding from the
four review rounds is now either fixed or explicitly deferred with a reason,
and I've closed the last outstanding structural ask (exercising the release ref
swap, in f32361cfc2).
**Fixed since your last pass:** PR validation builds cancel again instead of
queueing behind an abandoned merge ref; master publishing rejects a superseded
SHA at the tip boundary, with the delayed-A/fast-B case asserted; the command
check now pins *why* `DOCKERHUB_TOKEN` is hidden from Supersetbot rather than
suppressing it in the harness; the five hard-coded `apache/superset-cache`
consumers are bound to the Dockerfile's `ARG PY_VER` by an invariant test; and
the release ref swap is simulated against a synthetic older tree.
I also verified the riskiest unknown rather than assuming it: the
`DOCKERHUB_TOKEN=` blanking is safe on `--push`, a path no PR build exercises.
In supersetbot 0.5.45 that variable is read once, at `src/docker.js:125`, only
to decide whether to append `--cache-to`; push auth comes from
`docker/login-action` writing to the Docker config. So it cannot break master
or release publishing.
**Deliberately not in this PR**, so it's on the record rather than ambiguous:
1. **Orphaned build definitions.** `dockerize.Dockerfile` and
`superset-websocket/Dockerfile` still exist and are now built by nothing —
Supersetbot's `dockerize` preset points at the former and `websocket` at the
latter. Removing them is the logical completion of retiring those variants, but
it's a user-visible removal and I'd rather not widen this diff to include it.
Happy to do it in a follow-up.
2. **Sharing the Supersetbot invocation.** The command check assembles its
own command, so it cannot fail if the token blanking is deleted from
`docker.yml` itself. Extracting `scripts/docker-build.sh` (the move
`docker-build-plan.sh` already made for the event/ref decision) would close
that, but it puts the live publish path behind new indirection.
3. **`base` label and one stale `--cache-from`.** Supersetbot sets `--label
base=${pyVer}` from its own `BASE_PY_IMAGE = '3.10-slim-bookworm'` for
`superset`/`dev`/`lean`, while our `--build-arg PY_VER` override makes them
actually build `3.11.14-slim-trixie`, and its `--cache-from` points at the same
stale ref. Pre-existing, cosmetic plus one wasted registry lookup, fixable the
same way the `PY_VER` override works.
4. **Compose cache ref.** The Compose files pull the `superset` preset's
cache ref while defaulting to building the `dev` target, which now exports to
`…-trixie-dev`, so they get shared base layers rather than the dev cache.
Deliberate: `SUPERSET_BUILD_TARGET` is variable, so no single ref is correct,
and the base ref is the useful default.
**On the residual risk, plainly:** the master publish path — `--push`,
multi-arch, real `master*` tags, the guard comparing against a live branch tip
— cannot execute on a pull request. Further review rounds can't reduce that;
only merging and watching the first master push can. I'd suggest landing this
and checking that run directly: confirm `master`, `master-dev`, `master-lean`,
`master-py311` and `master-py312` actually move (they've been frozen since
2024-03-13, which is the bug this fixes) and that the guard logged
`publish=true`. It reverts cleanly if not.
Could you approve, or name specifically what still blocks? If any of the
four deferred items is a blocker for you rather than a follow-up, I'll fold it
in.
--
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]
