Page MenuHomePhabricator

Add actionable instruction when surfacing dagster home error
ClosedPublic

Authored by rexledesma on Sep 22 2020, 6:08 PM.

Details

Summary

Rather than leak references to internal functions, we give an error message to the user explaining what dagster home is and how to set it.

Test Plan

Unit

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

Nice!!

Maybe after "...in your filesystem" we can add "that contains your dagster instance configuration file (dagster.yaml)"

I think we should also add an informative error if the user sets $DAGSTER_HOME correctly but there is no dagster.yaml present.

python_modules/dagster/dagster/core/instance/config.py
26

I think that by the time we enter this code path, we do require that the file exists so ""If nothing is specified..." section would be a bit misleading.

Was thinking we should do something similar to check.invariant(os.path.isdir(base_dir), "base_dir should be a directory") which raises on failure

python_modules/dagster/dagster/core/instance/config.py
26

ah my bad, i thinkkkk its actually valid for the path not to exist

This revision is now accepted and ready to land.Sep 23 2020, 9:04 PM
dgibson added inline comments.
python_modules/dagster/dagster_tests/core_tests/instance_tests/test_instance.py
96–101

i think this one fails when you run it locally and do have DAGSTER_HOME set. No biggie, fixed in https://dagster.phacility.com/D4531