add test

alright the staging area change works - this isn't needed

lets take some time to think this one through since it appears doing it in a backwards compat safe and not confusing way may be tricky

Buildkite also to run bin/clean.py on every merge to master?

them empty errors view is a little janky too but not blocking

in max we trust

TEST PLAN
na

I would much rather see an appropriate where the module/fn name of the function that defines the solid definition to be put under test is serialized with an appropriate pipeline, repository, etc reconstructed on the other sid, and the intermediates store is used for the input values.

everything old is new again https://dagster.phacility.com/D1671 time is a flat circle

this stuff still scares me but i feel like developing trust in these migrations is pretty key for us

oh shit migrations - want @max in here for that

4th?

rename + enum would be nice but isn't blocking - file away a starter issue if you punt on it

3rd times the charm'

handle empty

handle empty

to you q for question

this diff currently contains all the commits for the diff below as well

I think the repository.yaml constraint is reasonable, but maybe we can relax the requirement for the entrypoint being set? I think in our other images we haven't set this, that way the end user can choose to run dagster, dagster-graphql, dagit, etc. on the same image depending on use case

composite

i think we want to have just the value here since its now guaranteed to be string

woah good call almost whiffed on testing composite mappings which need much more care

leverage BuiltinScalarDagsterType for early errors

@nate since i was bugging you about this with regards to the k8s stuff the other day - what do we think the rules should be for a "dagster image"

make sure to test make prettier too - I think that might need to get updated to just yarn run prettier.

requesting changes to fix lint

code looks reasonable

accepting assuming good error happens

you get a good error when you try to install without these values set?

seems fine to land this as is though - unclear from discussion if there are follow ups but those can be other diff

seems legit to me

definitely not tied to using serdes

Why don't we special case default checks for the scalar types? I think that would snag an entire class of errors.

my initial reaction is negative - ill think about it some more

use native default values?

hmm this isnt too crazy - interested what other folks think

How do we feel about bundling this in with a crack at https://github.com/dagster-io/dagster/issues/1087

handle more cases add more tests

ya normal PresetDefinition construction can take this in with the rest of a manually constructed dict, the from_files case is a little tougher and might justify a with so i can add that in a separate diff

is any of this going to be a breaking change for users?

builder / deployment stuff looks v reasonable - tricky part is definitely where / how these knobs should be turned since having a lot of this stuff in the environment dict feels spooky - at least to me

this works for both the us rolling out pg case and the user supplied one i assume? we put the user provided password in a secret?

up

up

up

ExecutionTargetHandle is such a mess already - im skeptical that we want to do this image handling inside that. Especially since what we are loading out of the image is actually going to be a RepositorySnapshot which has a set of PipelineSnapshots and not a real RepositoryDefinition. I have a hunch putting a layer of indirection on top of ExecutionTargetHandle may be a better path.

up