Details
- Reviewers
• catherinewu - Commits
- R1:648d1a596a4d: Split user deployments into Helm subchart
integration
unit
Diff Detail
- Repository
- R1 dagster
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| 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? | |
| 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
amazing work!! two small notes, then good to go 💃
but also this
| 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 | |