Page MenuHomePhabricator

force run view under pipeline in url namespace

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


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

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)

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

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

Event Timeline

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

force new execution to open run view to namespaced pipeline url

prha edited the summary of this revision. (Show Details)Aug 29 2019, 7:50 PM
prha added a reviewer: Restricted Project.
prha planned changes to this revision.Aug 29 2019, 8:26 PM
prha requested review of this revision.Aug 29 2019, 8:33 PM
sashank accepted this revision.Sep 3 2019, 2:08 PM
sashank added a subscriber: sashank.
sashank added inline comments.

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


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.


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.Sep 3 2019, 2:08 PM
bengotow accepted this revision.Sep 3 2019, 5:30 PM

Just a few inline comments—nothing blocking!


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:


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


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.Sep 3 2019, 10:02 PM

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

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

fix merge conflict bug

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