Page MenuHomePhabricator

force run view under pipeline in url namespace
ClosedPublic

Authored by prha on Thu, Aug 29, 6:59 PM.

Details

Reviewers
bengotow
sashank
Group Reviewers
Restricted Project
Commits
R1:3101183904e0: force run view under pipeline in url namespace
Summary

The Run history refactor split out the runs view to be independent of a given pipeline. The TopNav component assumes that there is a selected pipeline, which gives the context for the Explore and Execute tabs.

This diff changes a single run view to still be scoped under a specific pipeline, changing the URL to include the loaded pipeline name.

Any loads of a single run view (/run/:runId) will redirect to a URL containing the pipeline name (/:pipelineName/run/:runId)

https://github.com/dagster-io/dagster/issues/1701

Test Plan

clicked to runs view, clicked through to single run view, saw appropriate url with pipeline name, and top nav dropdown populated

Diff Detail

Repository
R1 dagster
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

prha created this revision.Thu, Aug 29, 6:59 PM
prha updated this revision to Diff 4116.Thu, Aug 29, 7:24 PM

force new execution to open run view to namespaced pipeline url

prha edited the summary of this revision. (Show Details)Thu, Aug 29, 7:50 PM
prha added a reviewer: Restricted Project.
prha planned changes to this revision.Thu, Aug 29, 8:26 PM
prha requested review of this revision.Thu, Aug 29, 8:33 PM
sashank accepted this revision.Tue, Sep 3, 2:08 PM
sashank added a subscriber: sashank.
sashank added inline comments.
js_modules/dagit/src/runs/RunHistory.tsx
321–325

Should these links be updated too (to avoid the redirect)?

js_modules/dagit/src/runs/RunRoot.tsx
13–14

This might be a good place to use useQuery hook or extract the logic to a helper render function, so that we can avoid the three level ternary here.

18–20

Also pass along the query string in this Redirect. We currently use q=type:materialization and q=type:expectation to filter the logs.

This revision is now accepted and ready to land.Tue, Sep 3, 2:08 PM
bengotow accepted this revision.Tue, Sep 3, 5:30 PM

Just a few inline comments—nothing blocking!

js_modules/dagit/src/App.tsx
32

Hmm, I recently renamed :pipelineName/explore and the others to explore/:pipelineName (always prefixed with a known path component) because the routing was getting complicated and route-order dependent with the pipeline wildcard coming first.

I think this diff looks fine for now, but I wonder if we should do something like the following to make them all consistent in the future:

/runs
/runs/:id
/p/:pipelineName/runs/:id
/p/:pipelineName/explore
/p/:pipelineName/execute

(Using a shared prefix would eliminate the TopNav switch too)

js_modules/dagit/src/runs/RunRoot.tsx
18–20

Yeah this is a bit of pain, but I think you can say <Redirect to={{pathname: /...}} /> and it'll leave other parts of the location intact (the search string).

prha updated this revision to Diff 4181.Tue, Sep 3, 10:02 PM

update paths to use /p/ namespace for pipeline scoped views

prha updated this revision to Diff 4185.Tue, Sep 3, 10:10 PM

fix merge conflict bug

prha updated this revision to Diff 4187.Tue, Sep 3, 10:27 PM
  • fix snapshot tests
This revision was automatically updated to reflect the committed changes.