Skip to content

Commit

Permalink
Fix back-compat for new helm chart param for container context (#7476)
Browse files Browse the repository at this point in the history
Summary:
I incorrectly thought that the default values.yaml of False would be applied for this param, but since it's in a list of deployments, the values in values.yaml do not matter. Fix that and add tests that still use the old format where includeConfigInLaunchedRuns is not set at all.

Test Plan: BK
  • Loading branch information
gibsondan committed Apr 15, 2022
1 parent d1a1e44 commit 2c54844
Show file tree
Hide file tree
Showing 6 changed files with 10 additions and 12 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -51,12 +51,14 @@ spec:
secretKeyRef:
name: {{ include "dagsterUserDeployments.postgresql.secretName" $ }}
key: postgresql-password
{{- if $deployment.includeConfigInLaunchedRuns }}
{{- if $deployment.includeConfigInLaunchedRuns.enabled }}
# uses the auto_envvar_prefix of the dagster cli to set the --container-context arg
# on 'dagster api grpc'
- name: DAGSTER_CLI_API_GRPC_CONTAINER_CONTEXT
value: {{ include "dagsterUserDeployments.k8sContainerContext" (list $ $deployment) | fromYaml | toJson | quote }}
{{- end }}
{{- end }}
envFrom:
- configMapRef:
name: {{ include "dagster.fullname" $ }}-user-deployments-shared-env
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ class UserDeployment(BaseModel):
name: str
image: kubernetes.Image
dagsterApiGrpcArgs: List[str]
includeConfigInLaunchedRuns: UserDeploymentIncludeConfigInLaunchedRuns
includeConfigInLaunchedRuns: Optional[UserDeploymentIncludeConfigInLaunchedRuns]
port: int
env: Optional[Dict[str, str]]
envConfigMaps: Optional[List[kubernetes.ConfigMapEnvSource]]
Expand Down
10 changes: 7 additions & 3 deletions helm/dagster/schema/schema_tests/utils.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
from typing import Optional

from schema.charts.dagster_user_deployments.subschema.user_deployments import (
UserDeployment,
UserDeploymentIncludeConfigInLaunchedRuns,
Expand All @@ -6,15 +8,17 @@


def create_simple_user_deployment(
name: str, include_config_in_launched_runs: bool = False
name: str, include_config_in_launched_runs: Optional[bool] = False
) -> UserDeployment:
return UserDeployment(
name=name,
image=kubernetes.Image(repository=f"repo/{name}", tag="tag1", pullPolicy="Always"),
dagsterApiGrpcArgs=["-m", name],
port=3030,
includeConfigInLaunchedRuns=UserDeploymentIncludeConfigInLaunchedRuns(
enabled=include_config_in_launched_runs
includeConfigInLaunchedRuns=(
UserDeploymentIncludeConfigInLaunchedRuns(enabled=include_config_in_launched_runs)
if include_config_in_launched_runs != None
else None
),
)

Expand Down
1 change: 0 additions & 1 deletion helm/dagster/values.schema.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -665,9 +665,6 @@ def helm_chart_for_user_deployments_subchart_disabled(namespace, docker_image, s
"define_demo_execution_repo",
],
"port": 3030,
"includeConfigInLaunchedRuns": {
"enabled": False,
},
"env": (
{"BUILDKITE": os.getenv("BUILDKITE")} if os.getenv("BUILDKITE") else {}
),
Expand Down Expand Up @@ -723,9 +720,6 @@ def helm_chart_for_user_deployments_subchart(namespace, docker_image, should_cle
"define_demo_execution_repo",
],
"port": 3030,
"includeConfigInLaunchedRuns": {
"enabled": False,
},
}
],
}
Expand Down

0 comments on commit 2c54844

Please sign in to comment.