Page MenuHomePhabricator
Differential D5891

Update helm defaults for user code deployments, run launcher, and rabbitmq
ClosedPublic
Actions

Authored by rexledesma on Jan 9 2021, 12:43 AM.

Details

Reviewers
catherinewu
nate
bob
alangenfeld
Commits
R1:c41d5be09a98: Update helm defaults for user code deployments, run launcher, and rabbitmq
Summary

Depends on D5890.

We change the following defaults in the helm chart:

  1. User code deployments are now enabled by default.
  2. The default run launcher is now the K8sRunLauncher.

Thus, we now disable`rabbitmq`, a message broker used
in celery, by default.

Test Plan

helm lint
integration

Diff Detail

Repository
R1 dagster
Branch
rl-change-helm-defaults
Lint
Lint OK
Unit
No Unit Test Coverage

Event Timeline

rexledesma created this revision.Jan 9 2021, 12:43 AM
rexledesma edited the summary of this revision. (Show Details)Jan 9 2021, 12:44 AM
rexledesma added a parent revision: D5890: Expose custom run launcher option in helm values.
Harbormaster returned this revision to the author for changes because remote builds failed.Jan 9 2021, 12:58 AM
Harbormaster failed remote builds in B23997: Diff 29180!
rexledesma requested review of this revision.Jan 9 2021, 1:03 AM
rexledesma added inline comments.Jan 9 2021, 1:08 AM
helm/dagster/values.yaml
149

Wondering if we can sneak this in as well

rexledesma updated this revision to Diff 29191.Jan 9 2021, 1:46 AM

rebase

Harbormaster completed remote builds in B24005: Diff 29191.Jan 9 2021, 2:04 AM
alangenfeld added inline comments.Jan 11 2021, 4:51 PM
helm/dagster/values.yaml
149

ya i think we should move towards this being the default, only bummer is filling out dagsterApiGrpcArgs. We should at least make sure thats documented well if we don't have other means of improving

168–170

do we have any clever ideas for if this can be improved? this is not very user friendly ergonomics wise, though i dont know how to make it better without a lot of complexity

rexledesma added inline comments.Jan 11 2021, 5:01 PM
helm/dagster/values.yaml
168–170

We could surface grpc flags as fields to construct the dagster api grpc command, but again that's tying the helm chart to implementation. I'll put up some RFC later on this?

alangenfeld resigned from this revision.Jan 11 2021, 10:06 PM

defer to @catherinewu for final accept to make sure docs are aligned

catherinewu accepted this revision.Jan 11 2021, 10:22 PM

LGTM!

helm/dagster/values.yaml
422

i didnt quite follow why we want to disable rabbitmq by default?

This revision is now accepted and ready to land.Jan 11 2021, 10:22 PM
catherinewu added a comment.Jan 11 2021, 10:23 PM

oh, we should run integration tests on this

rexledesma edited the test plan for this revision. (Show Details)Jan 14 2021, 4:05 PM
rexledesma updated this revision to Diff 29690.Jan 14 2021, 4:05 PM

rebase

rexledesma updated this revision to Diff 29696.Jan 14 2021, 4:17 PM

update integration tests

rexledesma updated this revision to Diff 29704.Jan 14 2021, 5:01 PM

up

Harbormaster failed remote builds in B24395: Diff 29690!Jan 14 2021, 5:12 PM
Harbormaster failed remote builds in B24400: Diff 29696!
Harbormaster completed remote builds in B24404: Diff 29704.Jan 14 2021, 5:22 PM
rexledesma updated this revision to Diff 29706.Jan 14 2021, 5:22 PM

update changes and migration instructions

Harbormaster completed remote builds in B24406: Diff 29706.Jan 14 2021, 5:41 PM
Closed by commit R1:c41d5be09a98: Update helm defaults for user code deployments, run launcher, and rabbitmq (authored by rexledesma). · Explain WhyJan 14 2021, 5:55 PM
This revision was automatically updated to reflect the committed changes.
rexledesma added a commit: R1:c41d5be09a98: Update helm defaults for user code deployments, run launcher, and rabbitmq.

Revision Contents
Changeset List

PathSize
helm/
dagster/
60 lines

Diff 29690

View Options

helm/dagster/values.yaml

Loading...