Page MenuHomeElementl

Split user deployments into Helm subchart
ClosedPublic

Authored by rexledesma on Feb 16 2021, 6:47 AM.

Details

Summary

Depends on D6881 and D6883.

As the title. Renames userDeployments -> dagster-user-deployments in accordance with the subchart name.

Test Plan

integration
unit

Diff Detail

Repository
R1 dagster
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Harbormaster returned this revision to the author for changes because remote builds failed.Mar 11 2021, 5:05 PM
Harbormaster failed remote builds in B27193: Diff 33249!
helm/dagster/charts/dagster-user-deployments/Chart.yaml
5

update the description?

helm/dagster/charts/dagster-user-deployments/templates/configmap-env-user.yaml
13

is this intentionally removed?

helm/dagster/charts/dagster-user-deployments/values.yaml
17

how does this interact with dagster-user-deployments.enabled in the dagster/charts/values.yaml

when should users set this to false?

helm/dagster/values.yaml
167โ€“168

whats the reason we're breaking from the camelCase standard?

integration_tests/python_modules/dagster-k8s-test-infra/dagster_k8s_test_infra/helm.py
356

its kind of confusing if we expect users who have dagster-user-deployments.enabled = False to set dagster-user-deployments.enableSubchart to True?

751

should we have some tests that enable the user deployments but don't enable the subchart and make sure it still works?

add tests to check if chart renders

helm/dagster/values.yaml
167โ€“168

Chart names should not have uppercase letters. https://helm.sh/docs/chart_best_practices/conventions/#chart-names

integration_tests/python_modules/dagster-k8s-test-infra/dagster_k8s_test_infra/helm.py
356

we expect them to set dagster-user-deployments.enableSubchart to be False as well. The chart will fail to load otherwise - added a test for this case

751

added a template test for this case.

i think we need an end-to-end integration test for "dagster-user-deployments": {"enabled": True, "enableSubchart": False} to be sure that launching runs w/o using subcharts continues to work

add integration test for subchart disabled

install both umbrella chart and subchart

use install name as release name

port forward to postgres in namespace

disable nslookup if subchart is disabled

install subchart before parent chart

amazing work!! two small notes, then good to go ๐Ÿ’ƒ

Macro pooh_eating_honey:

but also this

Macro burn-it-down:

helm/dagster/templates/configmap-workspace.yaml
4

should this check be somewhere else, even if its _helpers.tpl

also, could we simplify the error message to "dagster-user-deployments subchart cannot be enabled if userDeployments is not enabled."?

integration_tests/python_modules/dagster-k8s-test-infra/dagster_k8s_test_infra/helm.py
240โ€“253

can we have this check be true by default and special case the exception on L320

i think if people use a different chart_name we should still run these "default" pod checks

This revision is now accepted and ready to land.Mar 18 2021, 4:36 AM
This revision was automatically updated to reflect the committed changes.