As the title.
Details
Details
bk, helm
Diff Detail
Diff Detail
- Repository
- R1 dagster
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| helm/dagster/templates/configmap-instance.yaml | ||
|---|---|---|
| 85 | do we need this with statement? would be good to reduce statement nesting if possible, especially for the helm chart since it's already a bit difficult to read | |
| helm/dagster/values.yaml | ||
| 271 | I think we should describe this as using the K8sRunLauncher as opposed to the default CeleryK8sRunLauncher; I think describing this as the "celery-less run launcher" is less precise since there are actually a few run launchers that are "celery-less". Also, we should enable this run launcher here instead of within the celery config and then assert that if the k8s run launcher is enabled, that celery is not enabled. This is much more discoverable for a user who is reading the high level sections and wants to know how to enable this run launcher or who is reading the file and wants to know whether the config specified in this section is being used | |
| 273 | would prefer to use camel case since most of the helm chart is in camel case. opening a GH issue (https://github.com/dagster-io/dagster/issues/3129) to standardize the casing | |
| 285 | tilde? would prefer this value to be the same format as elsewhere kubeconfigFile: "" | |
| helm/dagster/values.yaml | ||
|---|---|---|
| 271 |
I'll put in this assertion for now, and fail if they're both enabled. Obviously, not an ideal solution, but restructuring the helm file via this proposal will make it easier to maintain this in the future. | |
| 285 | The default value for kubeconfig_file in the launcher is actually None, which is why i preferred ~ over "". But looks like they're interpreted the same way via the configurable class so it doesn't really matter | |
| helm/dagster/templates/configmap-instance.yaml | ||
|---|---|---|
| 61 | i think this should just check whether .Values.k8sRunLauncher.enabled is enabled and the "not (.Values.celery.enabled and .Values.k8sRunLauncher.enabled)" should be checked elsewhere | |
| helm/dagster/values.yaml | ||
| 201–202 | "Celery run launcher" -> "CeleryK8sRunLauncher" | |
| 202 | I would perhaps word this as "When enabling the K8sRunLauncher, make sure to set celery.enabled to be false" | |
| python_modules/libraries/dagster-k8s/dagster_k8s/launcher.py | ||
| 61 | hmm, this part isn't true anymore. We should remove: This image will be run with the command dagster-graphql -p startPipelineExecution -v {executionParams}`. It uses 'dagster api execute_run_with_structured_logs' instead | |