-
Notifications
You must be signed in to change notification settings - Fork 595
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
Allow serving web ui at arbitrary root #348
Conversation
ec2bb92
to
4b10bf3
Compare
internal/driver/webui.go
Outdated
|
||
mux := http.NewServeMux() | ||
mux.Handle("/ui/", http.StripPrefix("/ui", handler)) | ||
mux.Handle("/", http.RedirectHandler("/ui/", http.StatusTemporaryRedirect)) |
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.
I'd add a comment on why the redirect is done.
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.
Done.
@tschottdorf Added a small comment. I agree #208 is overdue. |
Previously, the web UI was hard-coded to be served at `/` but applications which are *embedding* the ui are unlikely to host it there. This change makes the links between the various ui pages relative, addressing this problem. To make sure this doesn't rot, the internal http server now serves the ui at /ui/ (redirecting there from the root). I think we can all agree that the ui code and JavaScript is kind of a haphazard affair. I performed a great deal of clicking around to make sure the semantics didn't change and interestingly spent some time trying to fix semantics I later realized weren't actually there: I thought that when navigating to another page from a focused search (say `flamegraph?f=x`) the focus would be carried over. This doesn't seem to be the case, however. The point being, there's no test coverage and setting it up is way beyond what I can do here. I'm happy to manually verify whatever sequence of clicks is suggested, though. Touches cockroachdb/cockroach#24145.
Thanks for the review, added the comment. |
Codecov Report
@@ Coverage Diff @@
## master #348 +/- ##
==========================================
- Coverage 66.55% 66.52% -0.04%
==========================================
Files 36 36
Lines 7439 7441 +2
==========================================
- Hits 4951 4950 -1
- Misses 2084 2087 +3
Partials 404 404
Continue to review full report at Codecov.
|
Previously, the web UI was hard-coded to be served at `/` but applications which are *embedding* the ui are unlikely to host it there. This change makes the links between the various ui pages relative, addressing this problem. To make sure this doesn't rot, the internal http server now serves the ui at /ui/ (redirecting there from the root). I think we can all agree that the ui code and JavaScript is kind of a haphazard affair. I performed a great deal of clicking around to make sure the semantics didn't change and interestingly spent some time trying to fix semantics I later realized weren't actually there: I thought that when navigating to another page from a focused search (say `flamegraph?f=x`) the focus would be carried over. This doesn't seem to be the case, however. The point being, there's no test coverage and setting it up is way beyond what I can do here. I'm happy to manually verify whatever sequence of clicks is suggested, though. Touches cockroachdb/cockroach#24145.
Previously, the web UI was hard-coded to be served at
/
butapplications which are embedding the ui are unlikely to host it there.
This change makes the links between the various ui pages relative,
addressing this problem.
To make sure this doesn't rot, the internal http server now serves the
ui at /ui/ (redirecting there from the root).
I think we can all agree that the ui code and JavaScript is kind of a
haphazard affair. I performed a great deal of clicking around to make
sure the semantics didn't change and interestingly spent some time
trying to fix semantics I later realized weren't actually there: I
thought that when navigating to another page from a focused search (say
flamegraph?f=x
) the focus would be carried over. This doesn't seem tobe the case, however.
The point being, there's no test coverage and setting it up is way
beyond what I can do here. I'm happy to manually verify whatever
sequence of clicks is suggested, though.
Touches cockroachdb/cockroach#24145.