Page MenuHomeElementl

fix: assert compute log manager json schema matches configurable class
ClosedPublic

Authored by rexledesma on Jun 18 2021, 3:21 PM.

Diff Detail

Repository
R1 dagster
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

helm/dagster/schema/schema_tests/test_instance.py
102

So if we were to ever add another compute log manager, we'd have to remember to go add it here?

Are there other configurable classes we should do this for as well?

helm/dagster/schema/tox.ini
8

I think I understand why aws, azure, and gcp are getting added (although I don't love what it says about our dependency tree that our general dagster helm chart depends on our specific cloud provider packages). Why are pandas and postgres getting added? Because they're dependencies of one of these other packages?

helm/dagster/schema/schema_tests/test_instance.py
102

Yes.. do you think that's a reasonable request? I'm trying to get the best of both worlds where the config is typed but also ensure that there's no drift between the OSS configuration and the Helm configuration. If this is too much of a headache, we can go back to the original plan of just removing the config typing

helm/dagster/schema/tox.ini
8

Otherwise, the command below pip list --exclude-editable | grep -e dagster -e dagit' would fail since the pypi versions of these packages would be installed. Yeah, definitely unideal

dgibson added inline comments.
helm/dagster/schema/schema_tests/test_instance.py
102

no strong opinion here, I guess the main thing this helps with is preventing typos in the class name? ultimately i think my gut would be to make it permissive for things that users are allowed to change, but I don't feel strongly

This revision is now accepted and ready to land.Jun 21 2021, 5:31 PM
This revision was landed with ongoing or failed builds.Jun 21 2021, 9:48 PM
This revision was automatically updated to reflect the committed changes.