ui: add static images to asset build step #121380
Conversation
|
This change is |
dhartunian
requested review from
laurenbarker,
a team and
abarganier
and removed request for
a team
dhartunian
added
backport-23.1.x
There was a problem hiding this comment.
LGTM, but I'll let Ricky and Lauren give the approval.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @dhartunian, @laurenbarker, and @rickystewart)
pkg/ui/workspaces/db-console/BUILD.bazel line 62 at r1 (raw file):
outs = [ "db-console-ccl/assets/bundle.js", "db-console-ccl/assets/favicon.ico",
Just curious, is there a reason we can't use wildcard patterns here for the assets dir?
There was a problem hiding this comment.
Reviewable status:
pkg/ui/workspaces/db-console/BUILD.bazel line 62 at r1 (raw file):
Previously, abarganier (Alex Barganier) wrote…
Just curious, is there a reason we can't use wildcard patterns here for the assets dir?
your question is the core of why UI builds with bazel are hard: bazel requires explicit definition of all outputs. hence you can't use wildcards, and you can't have webpack tell bazel what the names of the outputs are at runtime (if their names are hashes for instance).
|
@rickystewart friendly ping, any concerns about me adding some static assets to the UI build? I assume it wasn't previously done because it's problematic for maintenance reasons but I think the rate of change for these files is quite low. |
|
TFTR! bors r=laurenbarker |
|
Build failed (retrying...): |
|
Build failed (retrying...): |
|
CI seems to be red. bors r- |
|
Canceled. |
|
thanks @yuzefovich I see the problem. my build config is not OSS-compatible. I will fix. |
dhartunian
force-pushed
the
fix-missing-favicon
branch
from
5f07491 to
de7ad28
Compare
During the `genassets` build + embed step, we were taking just the output of the `db-console-ccl` or `db-console-oss` step which is just a build.js file. This commit adds references to the image assets we want bundled as well. This includes favicon.ico and everything in `./ assets` relative to the db-console build directory. We disable content hashing in webpack in order to keep the filenames static, which bazel requires. The impact should be minimal as we rarely change these images so if they're cached forever, it's fine. This change restores the favicon to the CRDB build and the nice image that shows up in the background of the email signup bar. The size of the final zipped bundle only differs by around 1MB and is already 10MB in size. Fixes: cockroachdb#117876 Epic: None Release note (ui change): the favicon now renders properly for DB Console along with other image files.
dhartunian
force-pushed
the
fix-missing-favicon
branch
from
de7ad28 to
5245691
Compare
|
bors r=laurenbarker |
|
This PR was included in a batch that was canceled, it will be automatically retried |
|
bors retry |
|
Build failed (retrying...): |
During the
genassetsbuild + embed step, we were taking just the output of thedb-console-cclordb-console-ossstep which is just a build.js file. This commit adds references to the image assets we want bundled as well. This includes favicon.ico and everything in./ assetsrelative to the db-console build directory.We disable content hashing in webpack in order to keep the filenames static, which bazel requires. The impact should be minimal as we rarely change these images so if they're cached forever, it's fine.
This change restores the favicon to the CRDB build and the nice image that shows up in the background of the email signup bar.
The size of the final zipped bundle only differs by around 1MB and is already 10MB in size.
Fixes: #117876
Epic: None
Release note (ui change): the favicon now renders properly for DB Console along with other image files.