-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
obsservice: ui handler serves db console assets #82741
obsservice: ui handler serves db console assets #82741
Conversation
|
df37331
to
56b6439
Compare
b188bd4
to
ac94fae
Compare
ac94fae
to
4e90a98
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @dhartunian and @sjbarag)
pkg/obsservice/obslib/lib.go
line 101 at r1 (raw file):
func (c *noOIDCConfigured) GetOIDCConf() ui.OIDCUIConf { return ui.OIDCUIConf{
nit: brief comments on this function
Code quote:
func (c *noOIDCConfigured) GetOIDCConf() ui.OIDCUIConf {
return ui.OIDCUIConf{
Enabled: false,
}
}
pkg/obsservice/obslib/lib.go
line 128 at r1 (raw file):
nodeID := &base.NodeIDContainer{} nodeID.Set(ctx, 123456)
nit: can we use a constant here instead so we can accompany it with some comments?
Code quote:
123456
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @abarganier, @dhartunian, and @sjbarag)
a discussion (no related file):
Good stuff @dhartunian ! Sorry for being slow on the review here 😓
pkg/cmd/dev/build.go
line 92 at r1 (raw file):
"geos": geosTarget, "libgeos": geosTarget, "obsservice": "//pkg/obsservice/cmd/obsservice",
Do we have a make
target set up for this as well? I can't remember if we need to also support make
builds
Code quote:
if target.fullName == buildTargetMapping["cockroach"] ||
target.fullName == buildTargetMapping["cockroach-oss"] ||
pkg/obsservice/obslib/lib.go
line 137 at r1 (raw file):
TODO(davidh): Previously, the
/
handler was the proxy to CRDB. This is now broken because we've started serving the UI from the same endpoint. We should find some way to serve the UI and proxy to CRDB at the same time. Unclear how this should work at the moment.
I assume it's the ambiguity of paths (is it a file, an SPA fallback to index.html, or a proxyable path?) that's the issue right? I wouldn't mind having /api/**
routes or something to separate UI paths from data-only paths 🤔
4e90a98
to
ff97794
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @abarganier, @dhartunian, and @sjbarag)
pkg/cmd/dev/build.go
line 92 at r1 (raw file):
Previously, sjbarag (Sean Barag) wrote…
Do we have a
make
target set up for this as well? I can't remember if we need to also supportmake
builds
We do not have a Makefile target. I'd prefer not to since we will never backport this code but I'll check with #dev-inf.
pkg/obsservice/obslib/lib.go
line 101 at r1 (raw file):
Previously, abarganier (Alex Barganier) wrote…
nit: brief comments on this function
Done.
pkg/obsservice/obslib/lib.go
line 128 at r1 (raw file):
Previously, abarganier (Alex Barganier) wrote…
nit: can we use a constant here instead so we can accompany it with some comments?
Done. Ended up just adding logic to be able to omit the NodeID since it makes no sense in this context.
pkg/obsservice/obslib/lib.go
line 137 at r1 (raw file):
Previously, sjbarag (Sean Barag) wrote…
TODO(davidh): Previously, the
/
handler was the proxy to CRDB. This is now broken because we've started serving the UI from the same endpoint. We should find some way to serve the UI and proxy to CRDB at the same time. Unclear how this should work at the moment.I assume it's the ambiguity of paths (is it a file, an SPA fallback to index.html, or a proxyable path?) that's the issue right? I wouldn't mind having
/api/**
routes or something to separate UI paths from data-only paths 🤔
This comment is actually out of date as I've added proxying below this. I'll extract the list of CRDB paths for clarity.
ff97794
to
dd70bf9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @abarganier, @dhartunian, and @sjbarag)
pkg/cmd/dev/build.go
line 92 at r1 (raw file):
dev-inf
I have confirmation that we do not need to support the Makefile
This change enables the Observabilty Service to serve the DB Console UI itself. Endpoints that CRDB can handle are proxied (`/_admin/`, `/_status/`, `/ts/`) but the base URL will return a blank HTML page with JS assets that load the DB Console. In order to build with the ui code, the `--config=with_ui` flag must be passed to bazel. This commit also adds a shortcut to the `dev` command to build `obsservice` which includes the `--config=with_ui` flag just as we do by default in `cockroach` builds. Release note: None
dd70bf9
to
b8faa51
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @abarganier, @dhartunian, and @sjbarag)
pkg/obsservice/obslib/lib.go
line 137 at r1 (raw file):
Previously, dhartunian (David Hartunian) wrote…
This comment is actually out of date as I've added proxying below this. I'll extract the list of CRDB paths for clarity.
Perfect, thank you!
TFTRs! bors r=sjbarag,abarganier |
Build succeeded: |
This change enables the Observabilty Service to serve the DB Console UI
itself. Endpoints that CRDB can handle are proxied (
/_admin/
,/_status/
,/ts/
) but the base URL will return a blank HTML page withJS assets that load the DB Console.
In order to build with the ui code, the
--config=with_ui
flag must bepassed to bazel.
This commit also adds a shortcut to the
dev
command to buildobsservice
which includes the--config=with_ui
flag just as we do bydefault in
cockroach
builds.Release note: None