Copilot commented on code in PR #42495: URL: https://github.com/apache/superset/pull/42495#discussion_r3667904403
########## docs/admin_docs/configuration/alerts-reports.mdx: ########## @@ -277,7 +277,7 @@ WEBDRIVER_OPTION_ARGS = [ ] # This is for internal use, you can keep http -WEBDRIVER_BASEURL = "http://superset:8088"; # When running using docker compose use "http://superset_app:8088' +WEBDRIVER_BASEURL = "http://superset:8088"; # This is also the default for Docker Compose, where the app service is named "superset" Review Comment: This example omits `SUPERSET_APP_ROOT`, but the dev Docker default is now explicitly `http://superset:8088{SUPERSET_APP_ROOT}/`. If `SUPERSET_APP_ROOT` is configured to a non-root path, readers following this snippet may still end up with a working hostname but an incorrect path for reports/screenshots. Suggest updating the comment (or the example) to mention including `SUPERSET_APP_ROOT` when set, and that the value should end with a trailing slash for predictable `urljoin` behavior. ########## docker/pythonpath_dev/superset_config.py: ########## @@ -114,7 +114,11 @@ class CeleryConfig: } EXTENSIONS_PATH = "/app/docker/extensions" ALERT_REPORTS_NOTIFICATION_DRY_RUN = True -WEBDRIVER_BASEURL = f"http://superset_app{os.environ.get('SUPERSET_APP_ROOT', '/')}/" # When using docker compose baseurl should be http://superset_nginx{ENV{BASEPATH}}/ # noqa: E501 +# The Docker Compose app service is named "superset" and listens on 8088. Only the +# scheme/host/port matter here (paths are joined with urljoin). For screenshots in +# the dev stack (unbuilt static assets) point this at the nginx service instead: +# http://nginx{SUPERSET_APP_ROOT}/ +WEBDRIVER_BASEURL = f"http://superset:8088{os.environ.get('SUPERSET_APP_ROOT', '/')}/" Review Comment: The comment says only scheme/host/port matter, but the configured value also includes `SUPERSET_APP_ROOT` (and in practice, the base path and trailing slash do affect `urljoin` results). Consider rewording the comment to reflect that scheme/host/port must be correct and the base path should match `SUPERSET_APP_ROOT` (with a trailing slash), otherwise URL joining can produce unexpected paths. ########## docker/pythonpath_dev/superset_config.py: ########## @@ -114,7 +114,11 @@ class CeleryConfig: } EXTENSIONS_PATH = "/app/docker/extensions" ALERT_REPORTS_NOTIFICATION_DRY_RUN = True -WEBDRIVER_BASEURL = f"http://superset_app{os.environ.get('SUPERSET_APP_ROOT', '/')}/" # When using docker compose baseurl should be http://superset_nginx{ENV{BASEPATH}}/ # noqa: E501 +# The Docker Compose app service is named "superset" and listens on 8088. Only the +# scheme/host/port matter here (paths are joined with urljoin). For screenshots in +# the dev stack (unbuilt static assets) point this at the nginx service instead: +# http://nginx{SUPERSET_APP_ROOT}/ +WEBDRIVER_BASEURL = f"http://superset:8088{os.environ.get('SUPERSET_APP_ROOT', '/')}/" Review Comment: This can produce double (or triple) slashes when `SUPERSET_APP_ROOT` is `/` (or already has leading/trailing slashes), e.g. `http://superset:8088//`. While browsers often tolerate this, it can change `urljoin` semantics and lead to surprising path resolution. Consider normalizing `SUPERSET_APP_ROOT` once (e.g., ensure exactly one leading slash and no trailing slash) before building `WEBDRIVER_BASEURL`, then append a single trailing slash. -- 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]
