Page MenuHomePhabricator

Allow Dagit to be mounted at a path via new `—path` CLI param
ClosedPublic

Authored by bengotow on Jul 4 2020, 2:12 AM.

Details

Summary

This diff adds a new --path param that hosts dagit and the /graphql endpoint at a subpath. No requests assume the root path, and hitting the root redirects to the prefixed path (just in case the user has not mounted something else at the root of the domain). The path you choose is injected into the the React app via a meta tag in the HTML, and is used to do two things:

  • We give React Router the path prefix. This makes most of the app "just work" which is actually fairly surprising - <Link to="/bla"> goes to "/path/bla" and <Route path="/bla" /> handles "/path/bla".
  • We store the path prefix and manually append it when we have to build URLs from scratch, eg: for opening new windows / tabs. To make this less of a pain point, I consolidated code that handles the launch/relaunch responses (it was nearly identical) so there is only one place in the app we manually assemble a run URL.

Sidenote: create-react-app is designed to support building apps to run on a path (and automatically prefixes the paths to assets, javascript, css, etc.) but we can't use their out-of-the-box support because we need the path specified at runtime. My solution of using find/replace to "fix" the paths in the index.html file isn't super great but gets the job done... :-/

Dev workflow: Right now there's not a great way to test this when you're running the React app via make. To test this, you need to build the React app (yarn build-for-python) and then serve it with dagit. I would like to change this and use a subpath all the time during development because it'd enforce good hygiene / prevent us from breaking support for paths in the future by accident, but I haven't found a good way to do it yet.

Test Plan

Run dagit at a path!

Diff Detail

Repository
R1 dagster
Branch
dbg/running-at-pat
Lint
Lint OK
Unit
No Unit Test Coverage

Event Timeline

bengotow created this revision.Jul 4 2020, 2:12 AM
bengotow edited the summary of this revision. (Show Details)Jul 4 2020, 2:12 AM
bengotow edited the summary of this revision. (Show Details)Jul 4 2020, 2:18 AM
Harbormaster returned this revision to the author for changes because remote builds failed.Jul 4 2020, 2:26 AM
Harbormaster failed remote builds in B14666: Diff 17997!
bengotow updated this revision to Diff 17998.Jul 4 2020, 3:22 AM
  • Rebase, fix tests
  • Use <meta> tag instead of <base> which has implications we don't want
  • Fix urls to custom fonts
Harbormaster returned this revision to the author for changes because remote builds failed.Jul 4 2020, 3:37 AM
Harbormaster failed remote builds in B14667: Diff 17998!
bengotow updated this revision to Diff 18037.Jul 6 2020, 2:53 PM

Ignore missing index.html file when running from tests

Harbormaster returned this revision to the author for changes because remote builds failed.Jul 6 2020, 3:04 PM
Harbormaster failed remote builds in B14698: Diff 18037!
Harbormaster returned this revision to the author for changes because remote builds failed.Jul 6 2020, 5:08 PM
Harbormaster failed remote builds in B14707: Diff 18048!
bengotow updated this revision to Diff 18077.Jul 6 2020, 8:45 PM

Black again for some reason?

bengotow requested review of this revision.Jul 6 2020, 8:58 PM

should be relatively worth it and easy to add some dagit cli tests around acceptable path values

CHANGES.md
26

move to a 0.8.7 (Upcoming) section

python_modules/dagit/dagit/app.py
172–182

scaredbaby

185

this seems like a mistake

python_modules/dagit/dagit/cli.py
65–69

pathPrefix or something a little more specific maybe

145–147

do we ensure that the path passed starts with / ?

alangenfeld requested changes to this revision.Jul 6 2020, 9:43 PM
This revision now requires changes to proceed.Jul 6 2020, 9:43 PM
bengotow planned changes to this revision.Jul 7 2020, 12:29 AM
bengotow added inline comments.
python_modules/dagit/dagit/app.py
185

Yeah I will revert this to the way it used to work. I had to add this line to workaround the fact that the tests launch dagit without the react app compiled and I moved this from a runtime error to a launch-time error.

python_modules/dagit/dagit/cli.py
65–69

That sounds good to me!

145–147

Hmm we do, I will add some validation

bengotow updated this revision to Diff 18104.Jul 7 2020, 12:33 AM

Apply diff feedback

schrockn resigned from this revision.Jul 7 2020, 1:56 PM

Good stuff. Will let @alangenfeld approve. The index string replacement is definitely unfortunate but we can deal for now

bengotow updated this revision to Diff 18115.Jul 7 2020, 2:10 PM

Need to tie VSCode to Black

alangenfeld requested changes to this revision.Jul 7 2020, 3:12 PM
alangenfeld added inline comments.
python_modules/dagit/dagit_tests/test_app.py
158–198

can we add some tests here for valid / invalid values of path prefix?

This revision now requires changes to proceed.Jul 7 2020, 3:12 PM
bengotow planned changes to this revision.Jul 7 2020, 3:34 PM
bengotow added inline comments.
python_modules/dagit/dagit_tests/test_app.py
158–198

Oh definitely!

bengotow updated this revision to Diff 18130.Jul 7 2020, 4:05 PM

Add tests for Dagit path prefix feature

This revision is now accepted and ready to land.Jul 7 2020, 4:45 PM