Build UI as part of the trino-main module#22785
Conversation
wendigo
requested review from
electrum,
losipiuk,
nineinchnick and
raunaqmorarka
wendigo
changed the title
wendigo
force-pushed
the
serafin/exclude-webapp-src
branch
from
5f05570 to
ecb252d
Compare
.github/workflows/ci.yml
Outdated
There was a problem hiding this comment.
maybe we need to change this to instead verify if the build causes the lock files to change - which suggests something needs checking in.
There was a problem hiding this comment.
I'm not super convinced it's necessary.
|
This is a result of #22765 and our investigation on how to improve this overall. |
There was a problem hiding this comment.
Code changes all look good to me. In Trino Gateway we had some issues with a similar approach where the ordering of the plugin executions caused the UI to be missing on a build from a clean checkout. Please verify that this is not the case here.
Also .. how do we test the UI since there are a bunch of dependency updates woven into this PR now.
Lastly .. I think it is best practice to check in the lock files as you did .. can you confirm? And also verify it wont cause issues?
|
@mosabua good feedback. I've added explicit phases to the executions! |
wendigo
force-pushed
the
serafin/exclude-webapp-src
branch
from
b605916 to
e879ba0
Compare
|
@mosabua the dependency updates are around tooling, not the react itself that is used in the UI. I've tested it manually and it seems to work :) |
Well.. at least React was updated. Do you know if the dist folder shadowed the declared and transitive dependencies or reverse? In either case .. the fact that you tested it a bit is good to know. Not sure how much more testing we should do .. maybe just merge shortly after the next release and see what comes up in development usage ;-) Also not sure if we want to add explicit declaration of other dependencies such as vis |
|
@mosabua it was but to a patch version to address a CVE |
|
@mosabua after 453 it is :) |
wendigo
force-pushed
the
serafin/exclude-webapp-src
branch
from
e879ba0 to
0184075
Compare
|
@mosabua merged |
5 participants
This removes checked-in dist/ directory.
Instead of keeping commited dist/ directory, it's built from sources using frontend plugin. I've checked that builds in production mode are reproducible. As a consequence I've dropped check_webui.sh as now the tests are executed as part of the trino-main.