sadpandajoe commented on code in PR #39300:
URL: https://github.com/apache/superset/pull/39300#discussion_r3277626989
##########
superset-frontend/playwright.config.ts:
##########
@@ -132,6 +133,26 @@ export default defineConfig({
// No storageState = clean browser with no cached cookies
},
},
+ ...(process.env.INCLUDE_EMBEDDED
+ ? [
+ {
+ // Embedded dashboard tests - validates the full embedding flow:
+ // external app -> SDK -> iframe -> guest token -> dashboard
render.
+ // Each spec file mutates per-dashboard embedding state (UUID,
+ // allowed_domains) on a single shared Superset, so files must not
+ // run in parallel even if more are added later.
+ name: 'chromium-embedded',
+ testMatch: '**/tests/embedded/**/*.spec.ts',
+ fullyParallel: false,
Review Comment:
Thanks — this is accurate, but Playwright doesn't support a project-level
`workers` override (the `workers` setting is top-level only). The current
safeguards are:
- Only one spec file matches `**/tests/embedded/**/*.spec.ts` today
- That spec uses `test.describe.configure({ mode: 'serial' })` at the file
level
- The comment at `playwright.config.ts:141-143` already warns future
contributors that adding a second embedded spec requires running with
`--workers=1`
I'd rather not add a contortion (e.g. CI-time spec-count assert) for a
constraint that the comment already documents. If a second embedded spec is
added later, the right move is to update the CI invocation, not bake a
workaround into the config.
##########
docker/pythonpath_dev/superset_config_docker_light.py:
##########
@@ -36,3 +38,32 @@
# Disable Celery entirely for lightweight mode
CELERY_CONFIG = None # type: ignore[assignment,misc]
+
+# Honor SUPERSET_FEATURE_<NAME> env vars on top of any flags inherited from
+# superset_config. Lets local dev/e2e enable features (e.g. EMBEDDED_SUPERSET)
+# without editing shipped config files. Only the literal string "true"
+# (case-insensitive) is treated as enabled — "1"/"yes"/"on" are not, matching
+# the strict-string convention used elsewhere in Superset's env parsing.
+FEATURE_FLAGS = {
+ **FEATURE_FLAGS, # noqa: F405
+ **{
+ name[len("SUPERSET_FEATURE_") :]: value.strip().lower() == "true"
+ for name, value in os.environ.items()
+ if name.startswith("SUPERSET_FEATURE_")
+ },
+}
+
+if os.environ.get("SUPERSET_FEATURE_EMBEDDED_SUPERSET", "").strip().lower() ==
"true":
+ # Disable Talisman so /embedded/<uuid> doesn't return
X-Frame-Options:SAMEORIGIN.
+ # Without this, browsers refuse to render Superset inside an iframe from a
+ # different origin (i.e. the embedded SDK use case). Production/CI
configures
+ # Talisman with explicit `frame-ancestors`; for the lightweight local
stack we
+ # just turn it off.
+ TALISMAN_ENABLED = False
Review Comment:
This is intentional for the lightweight dev stack only, and the comment at
L58-61 already documents it:
> Production/CI configures Talisman with explicit `frame-ancestors`; for the
lightweight local stack we just turn it off.
`docker-compose-light.yml` is a developer convenience stack — it isn't
shipped for production embedding. The production embedding setup configures
Talisman with narrow `frame-ancestors` rather than disabling it, and that path
is unchanged by this PR. The trade-off here is "one local toggle vs. a more
complex narrow-CSP recipe in a dev-only config", and we've taken the simpler
option.
Happy to revisit if there's a downstream consumer of this lightweight config
in a non-dev context I'm missing.
--
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]
