Page MenuHomePhabricator

link schedule page
ClosedPublic

Authored by prha on Mon, Dec 2, 10:48 PM.

Details

Summary

D1479 created the standalone schedule page. This diff links it from the schedules list view, and formats the attempts table a little bit to take into account pipeline run state.

Test Plan

Viewed schedule page, saw both successful and unsuccessful attempts

Diff Detail

Repository
R1 dagster
Branch
prha/schedule_page
Lint
Lint OK
Unit
No Unit Test Coverage

Event Timeline

prha created this revision.Mon, Dec 2, 10:48 PM
prha edited the summary of this revision. (Show Details)Mon, Dec 2, 10:52 PM
prha updated this revision to Diff 7057.Mon, Dec 2, 11:46 PM

snapshot update

sashank accepted this revision.Tue, Dec 3, 12:23 AM

This is great!

js_modules/dagit/src/schedules/ScheduleRow.tsx
87–93

Could you explain what match is doing here?

This revision is now accepted and ready to land.Tue, Dec 3, 12:23 AM
sashank requested changes to this revision.Tue, Dec 3, 12:27 AM

From Slack: Would be nice for the 5th column in the top bar to show attempts instead of runs, so that the colors match up with the attempts list

This revision now requires changes to proceed.Tue, Dec 3, 12:27 AM
prha added inline comments.Tue, Dec 3, 12:29 AM
js_modules/dagit/src/schedules/ScheduleRow.tsx
87–93

yeah, match is non-null if it matches the /schedule/:scheduleName route.

We check it so that we don't show a link if we're already on the page that we're linking to.

prha updated this revision to Diff 7062.Tue, Dec 3, 1:33 AM

updated to display attempts rather than runs

prha edited the summary of this revision. (Show Details)Tue, Dec 3, 1:35 AM
prha updated this revision to Diff 7063.Tue, Dec 3, 3:00 AM

regenerate types

sashank accepted this revision.Tue, Dec 3, 9:04 PM

Looks great! Just two minor suggestions

js_modules/dagit/src/schedules/ScheduleRow.tsx
179

What do you think about wrapping this in a Tooltip too that reads "Click to show error", just for consistency

<Tooltip
  position={"top"}
  content={"Click to show error"}
  wrapperTagName="div"
  targetTagName="div"
>
  <AttemptStatus status={attempt.status} />
</Tooltip>
python_modules/dagster-graphql/dagster_graphql/schema/run_schedule.py
154 ↗(On Diff #7063)

[1]

198 ↗(On Diff #7063)

Maybe we should extract this and [1] to a function so that paths don't get out of sync

This revision is now accepted and ready to land.Tue, Dec 3, 9:04 PM
prha updated this revision to Diff 7088.Tue, Dec 3, 11:08 PM

sashank

This revision was automatically updated to reflect the committed changes.