Page MenuHomePhabricator

[Helm 2/N] More Helm fixes
ClosedPublic

Authored by nate on Tue, Feb 11, 6:24 PM.

Details

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

nate created this revision.Tue, Feb 11, 6:24 PM
nate updated this revision to Diff 9539.Tue, Feb 11, 9:44 PM

rebase

alangenfeld added inline comments.
python_modules/libraries/dagster-k8s/helm/dagster/templates/configmap-instance.yaml
15โ€“16

cc @sashank to verify this bit - seems unfortunate that we have to manually set this

python_modules/libraries/dagster-k8s/helm/dagster/templates/configmap-job-env.yaml
12

๐Ÿ˜†

python_modules/libraries/dagster-k8s/helm/dagster/templates/deployment.yaml
40โ€“48

should "scheduler on" be gated by a helm value?

python_modules/libraries/dagster-k8s/helm/dagster/values.yaml
14โ€“23

๐Ÿ‘ ya it think this is the right way to go about it

sashank added inline comments.Tue, Feb 11, 10:42 PM
python_modules/libraries/dagster-k8s/helm/dagster/templates/configmap-instance.yaml
15โ€“16

Not sure if there's a easy way to automatically set this.

python_modules/libraries/dagster-k8s/helm/dagster/templates/deployment.yaml
46

We don't need to do restart anymore - schedule up should automatically take care of it. So we can delete this line.

nate added inline comments.Tue, Feb 11, 10:46 PM
python_modules/libraries/dagster-k8s/helm/dagster/templates/deployment.yaml
46

cool, done

sashank added inline comments.Tue, Feb 11, 11:06 PM
python_modules/libraries/dagster-k8s/helm/dagster/templates/configmap-instance.yaml
15โ€“16

I'd say include it for now while we figure out a way to automatically provide a default for this.

python_modules/libraries/dagster-k8s/helm/dagster/templates/deployment.yaml
40โ€“48

If we're by default configuring the cron scheduler, then we probably don't need to?

sashank accepted this revision.Tue, Feb 11, 11:08 PM
This revision is now accepted and ready to land.Tue, Feb 11, 11:08 PM
This revision was automatically updated to reflect the committed changes.
alangenfeld added inline comments.Wed, Feb 12, 7:04 PM
python_modules/libraries/dagster-k8s/helm/dagster/values.yaml
20โ€“21

oops - job_image.image is unfortunate - should probably be job_runner or something. Worth changing? README.md fell out of date with this change as well

nate added inline comments.Wed, Feb 12, 7:06 PM
python_modules/libraries/dagster-k8s/helm/dagster/values.yaml
20โ€“21

yeah agree, will update both

alangenfeld added inline comments.Wed, Feb 12, 7:11 PM
python_modules/libraries/dagster-k8s/helm/dagster/templates/deployment.yaml
40โ€“48

the issue is if you don't install cron in your docker image - you now get this crash

cron: unrecognized service

and i cant just disable it with a helm flag, i now need to rebuild my images and this becomes a hard requirement for anyone using the helm chart