Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Exclude query_view_spa.html from build #24596

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

unidevel
Copy link
Contributor

@unidevel unidevel commented Feb 19, 2025

Description

For issue #24593

Changes:

  1. Exclude query_view_spa.html from build
  2. Add command yarn build:spa, yarn serve:spa, yarn watch:spa for development and build locally
  3. Fix the url '/ui/dev` not open the default index.html (fix in the java file)
  4. Fix/Adjust UI for query_view_spa.html (works as offline page) as screenshot below ( removed the API call, and fixed the header, show as Offline under UPTIME)
    image

Motivation and Context

The query_view_spa.html can not be open in the presto server by using URL like http://localhost:8080/ui/dev/query_view_spa.html
The cause is that from 0.291, the CSP is added in presto http server.
So the urls of external javascript/css files in query_view_spa.html will be blocked.
That is why we exlcude this page from build.
To open the query, user can open url like http://localhost:8080/ui/dev/ (which will open the index.html)
The query_view_spa.html page can be kept for offline use. User has to build it locally.

Impact

Presto UI, URL mapping for /ui/dev/, /ui/dev

Test Plan

Local test

Contributor checklist

  • Please make sure your submission complies with our contributing guide, in particular code style and commit standards.
  • PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
  • Documented new properties (with its default value), SQL syntax, functions, or other functionality.
  • If release notes are required, they follow the release notes guidelines.
  • Adequate tests were added if applicable.
  • CI passed.

Release Notes

== NO RELEASE NOTE ==

@prestodb-ci prestodb-ci added the from:IBM PR from IBM label Feb 19, 2025
@unidevel unidevel marked this pull request as ready for review February 19, 2025 20:05
@unidevel unidevel requested review from yhwang and a team as code owners February 19, 2025 20:05
@unidevel unidevel requested a review from jaystarshot February 19, 2025 20:05
@unidevel unidevel changed the title Separate query_view_spa.html from build Exclude query_view_spa.html from build Feb 19, 2025
yhwang
yhwang previously approved these changes Feb 19, 2025
Copy link
Member

@yhwang yhwang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the changes look good to me. However, still need to fix the flow errors.
Thanks for the fix.

@unidevel
Copy link
Contributor Author

the changes look good to me. However, still need to fix the flow errors. Thanks for the fix.

Thanks @yhwang , I forget to run the check before commit will keep in mind next time.

BTW, I removed the yarn run package from README.md, seems it is not working anymore.

httpServerBinder(binder).bindResource("/tableau", "webapp/tableau")
.withExtraHeader(HttpHeaders.X_CONTENT_TYPE_OPTIONS, "nosniff")
.withExtraHeader(HttpHeaders.CONTENT_SECURITY_POLICY, "default-src 'self'; style-src 'self' 'unsafe-inline' https://fonts.googleapis.com; font-src 'self' https://fonts.gstatic.com; frame-ancestors 'self'; img-src http: https: data:");
webUIBinder(binder, "/ui/dev", "webapp/dev").withWelcomeFile("index.html");
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add this to let user can use URL like http://localhost/ui/dev/ or http://localhost/ui/dev to access the query viewer page.
Before this fix, user have to use URL like http://localhost/ui/dev/index.html to access the query viewer page, also http://localhost/ui/dev/ and http://localhost/ui/dev will show a blank page with no errors.

Comment on lines +44 to +46
"build:spa": "webpack --config webpack.config.js --env production --env config=spa",
"serve:spa": "webpack serve --config webpack.config.js --env config=spa",
"watch:spa": "webpack --config webpack.config.js --env config=spa --watch",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New build/serve/watch command for page query_viewer_spa.html, will not generate this during presto build

Copy link
Contributor

@steveburnett steveburnett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! (docs)

"View file" in GitHub, everything looks good. Thanks for the edit!

@unidevel
Copy link
Contributor Author

@jaystarshot Can you help to review? Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
from:IBM PR from IBM
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants