Page MenuHomeElementl

(config-scaffold 2/n) Add scaffold config button to Dagit playground
ClosedPublic

Authored by sashank on Oct 18 2020, 7:12 AM.

Details

Summary

This diff adds a "scaffold missing config" button to Dagit, which is inspired by the scaffold CLI command. It does the scaffolding client side based on the config schema.

There are a few choices here that are debatable. I've highlighted them in the comments. Open to feedback about how to handle them.

Resolves: https://github.com/dagster-io/dagster/issues/3118

Test Plan

Created solid and resource with every possible config type. Checked to make sure scaffolded config was correct.

Video:

Diff Detail

Repository
R1 dagster
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

dish requested changes to this revision.Oct 19 2020, 7:54 PM

Direction looks great to me. Some nits inline, mostly stylistic/legibility things.

Main thing is that I think it would be good to use a solid third-party merge-deep library so that we don't have to deal with testing it ourselves. Sending back to your queue for that, feel free to push back if you have strong convictions about keeping this merge implementation.

js_modules/dagit/src/DomUtils.tsx
62 โ†—(On Diff #23968)

It might be worth using a third-party npm module for this, e.g. https://github.com/TehShrike/deepmerge. (No idea if it's a good library.)

Otherwise:

  • I'd just put this in its own file, e.g. src/utils/mergeDeep.
  • Definitely recommend a Jest test.
  • The mutativity is a little worrying. It could be pretty easy to use this wrong and end up mutating an object you didn't mean to.
js_modules/dagit/src/execute/ExecutionSessionContainer.tsx
201โ€“204

Looks like it could be defined outside of onScaffoldMissingConfig, and independently testable.

206โ€“249

Possibly a bit more legible as a switch. ๐Ÿคทโ€โ™‚๏ธ

270

Could just make runConfigData a const below, doesn't need to be a let here.

275

Remove?

284

Shouldn't need this.

476

Should be fine to do onScaffoldMissingConfig={this.onScaffoldMissingConfig}.

js_modules/dagit/src/execute/RunPreview.tsx
140โ€“165

A bit more legible with the function defined outside the React block.

const onClick = async () => {
  ...
};

// etc
<Button small onClick={onClick}>Scaffold Missing Config</Button>
160

Why the extra space?

This revision now requires changes to proceed.Oct 19 2020, 7:54 PM
sashank edited the test plan for this revision. (Show Details)
  • Extract scaffoldType to separate function and put under test
  • Use swtich statement in scaffoldType
  • Pull in deepmerge dependency and remove previous merge function
  • Address nits
  • Delete dead code
  • Add missing files

@dish this should be ready for another pass! Thanks for the detailed review and great suggestions

defer to @dish

js_modules/dagit/src/execute/ExecutionSessionContainer.tsx
212โ€“213

spooky todo - plan to resolve this before commit?

dish added inline comments.
js_modules/dagit/src/execute/ExecutionSessionContainer.tsx
213

This is no longer a problem now that you're using deepmerge, right?

js_modules/dagit/src/execute/scaffoldType.ts
38

Just return these directly rather than creating x, y, z?

This revision is now accepted and ready to land.Oct 26 2020, 6:29 PM
js_modules/dagit/src/execute/scaffoldType.ts
38

Ah thanks for catching this โ€“ I was doing this to generate test cases.

This revision now requires review to proceed.Nov 13 2020, 5:59 PM
This revision was not accepted when it landed; it landed in state Needs Review.Nov 25 2020, 9:09 PM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.