sfirke commented on code in PR #44244:
URL: https://github.com/apache/superset/pull/44244#discussion_r4047570776
##########
superset/utils/webdriver.py:
##########
@@ -57,10 +57,9 @@
WindowSize = tuple[int, int]
logger = logging.getLogger(__name__)
-# Installation message for missing Playwright (Cypress doesn't work with
DeckGL)
+# Installation hint appended to errors raised when Playwright or Chromium is
missing
PLAYWRIGHT_INSTALL_MESSAGE = (
- "To complete the migration from Cypress "
- "and enable WebGL/DeckGL screenshot support, install Playwright with: "
+ "Install Playwright and Chromium with: "
"pip install playwright && playwright install chromium"
)
Review Comment:
Good catch, and it applied more broadly than the line it was flagged on:
*every* failure to start the browser was being reported as a missing
dependency, not just some of them. A worker running as root without
`--no-sandbox`, a bad entry in `WEBDRIVER_OPTION_ARGS`, a Chromium killed for
running out of memory, an unwritable temp directory — all of them told the
operator to install Playwright, which was already installed.
Fixed in 069ade1a64 by deleting the message rather than correcting it.
Playwright's own error is now passed through untouched.
That turned out to be better even for the case the message was written for.
When the browser binary really is missing, Playwright says so, names the exact
path where it expected to find it, and gives the command to install it. The
path is the useful part — it's how you spot a wrong `PLAYWRIGHT_BROWSERS_PATH`
or a Docker layer that dropped the browser cache. The old message had none of
that, and its "pip install playwright" advice couldn't ever be right here,
because this code only runs after Playwright has already imported successfully.
The install hint is still used in one place: when the Playwright import
itself fails. There's no underlying error to show there, so a written message
is all that's available.
One consequence worth flagging. When a report fails, this text can be
emailed to the report's recipients, not just written to the worker log. So a
failed screenshot can now put Playwright's wording — and sometimes Chromium's
own output — in front of people who just subscribe to the report. That was
already true of every other error on this path, so it isn't a new kind of
exposure, but it does widen what those emails can contain. The real fix is to
stop using one string for both operator diagnostics and recipient-facing mail,
which is a bigger change than belongs in this PR.
---
🤖 _Drafted by Claude Code, reviewed and approved by @sfirke._
--
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]
