Skip to content

Commit

Permalink
make k8s container context volumes and volume mounts use snake case, …
Browse files Browse the repository at this point in the history
…not camel case (#7437)

Summary:
I realized when writing the docs that this field sticks out like a sore thumb in the typeahead since it's the only thing that expects snake case. We already have logic that is set up to coerce both snake case and camel case to be the right form, but this makes the default config that container_context uses use snake case instead of camel case.

Test Plan: BK
  • Loading branch information
gibsondan committed Apr 13, 2022
1 parent a5c2854 commit 7aef716
Show file tree
Hide file tree
Showing 3 changed files with 80 additions and 33 deletions.
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
from typing import TYPE_CHECKING, Any, Dict, List, NamedTuple, Optional

import kubernetes

from dagster import check
from dagster.config.validate import process_config
from dagster.core.errors import DagsterInvalidConfigError
Expand All @@ -10,6 +12,7 @@
from . import K8sRunLauncher

from .job import DagsterK8sJobConfig
from .models import k8s_snake_case_dict


def _dedupe_list(values):
Expand Down Expand Up @@ -59,8 +62,14 @@ def __new__(
env_config_maps=check.opt_list_param(env_config_maps, "env_config_maps"),
env_secrets=check.opt_list_param(env_secrets, "env_secrets"),
env_vars=check.opt_list_param(env_vars, "env_vars"),
volume_mounts=check.opt_list_param(volume_mounts, "volume_mounts"),
volumes=check.opt_list_param(volumes, "volumes"),
volume_mounts=[
k8s_snake_case_dict(kubernetes.client.V1VolumeMount, mount)
for mount in check.opt_list_param(volume_mounts, "volume_mounts")
],
volumes=[
k8s_snake_case_dict(kubernetes.client.V1Volume, volume)
for volume in check.opt_list_param(volumes, "volumes")
],
labels=check.opt_dict_param(labels, "labels"),
namespace=check.opt_str_param(namespace, "namespace"),
)
Expand Down
14 changes: 8 additions & 6 deletions python_modules/libraries/dagster-k8s/dagster_k8s/job.py
Original file line number Diff line number Diff line change
Expand Up @@ -418,14 +418,16 @@ def config_type_container(cls):
),
"volume_mounts": Field(
Array(
Shape(
# Can supply either snake_case or camelCase, but in typeaheads based on the
# schema we assume snake_case
Permissive(
{
"name": StringSource,
"mountPath": StringSource,
"mountPropagation": Field(StringSource, is_required=False),
"readOnly": Field(BoolSource, is_required=False),
"subPath": Field(StringSource, is_required=False),
"subPathExpr": Field(StringSource, is_required=False),
"mount_path": Field(StringSource, is_required=False),
"mount_propagation": Field(StringSource, is_required=False),
"read_only": Field(BoolSource, is_required=False),
"sub_path": Field(StringSource, is_required=False),
"sub_path_expr": Field(StringSource, is_required=False),
}
)
),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,14 @@ def container_context_config():
"env_vars": ["MY_ENV_VAR"],
"volume_mounts": [
{
"mountPath": "my_mount_path",
"mountPropagation": "my_mount_propagation",
"mount_path": "my_mount_path",
"mount_propagation": "my_mount_propagation",
"name": "a_volume_mount_one",
"readOnly": False,
"subPath": "path/",
"read_only": False,
"sub_path": "path/",
}
],
"volumes": [{"name": "foo", "configMap": {"name": "settings-cm"}}],
"volumes": [{"name": "foo", "config_map": {"name": "settings-cm"}}],
"labels": {"foo_label": "bar_value"},
"namespace": "my_namespace",
}
Expand All @@ -45,20 +45,46 @@ def other_container_context_config():
"env_vars": ["YOUR_ENV_VAR"],
"volume_mounts": [
{
"mountPath": "your_mount_path",
"mountPropagation": "your_mount_propagation",
"mount_path": "your_mount_path",
"mount_propagation": "your_mount_propagation",
"name": "b_volume_mount_one",
"readOnly": True,
"subPath": "your_path/",
"read_only": True,
"sub_path": "your_path/",
}
],
"volumes": [{"name": "bar", "configMap": {"name": "your-settings-cm"}}],
"volumes": [{"name": "bar", "config_map": {"name": "your-settings-cm"}}],
"labels": {"bar_label": "baz_value"},
"namespace": "your_namespace",
}
}


@pytest.fixture
def container_context_config_camel_case_volumes():
return {
"k8s": {
"image_pull_policy": "Always",
"image_pull_secrets": [{"name": "my_secret"}],
"service_account_name": "my_service_account",
"env_config_maps": ["my_config_map"],
"env_secrets": ["my_secret"],
"env_vars": ["MY_ENV_VAR"],
"volume_mounts": [
{
"mountPath": "my_mount_path",
"mountPropagation": "my_mount_propagation",
"name": "a_volume_mount_one",
"readOnly": False,
"subPath": "path/",
}
],
"volumes": [{"name": "foo", "configMap": {"name": "settings-cm"}}],
"labels": {"foo_label": "bar_value"},
"namespace": "my_namespace",
}
}


@pytest.fixture(name="empty_container_context")
def empty_container_context_fixture():
return K8sContainerContext()
Expand All @@ -74,6 +100,11 @@ def other_container_context_fixture(other_container_context_config):
return K8sContainerContext.create_from_config(other_container_context_config)


@pytest.fixture(name="container_context_camel_case_volumes")
def container_context_camel_case_volumes_fixture(container_context_config_camel_case_volumes):
return K8sContainerContext.create_from_config(container_context_config_camel_case_volumes)


def test_empty_container_context(empty_container_context):
assert empty_container_context.image_pull_policy == None
assert empty_container_context.image_pull_secrets == []
Expand Down Expand Up @@ -102,6 +133,11 @@ def _check_same_sorted(list1, list2):
) == sorted([make_readonly_value(val) for val in list2], key=lambda val: val.__hash__())


def test_camel_case_volumes(container_context_camel_case_volumes, container_context):
assert container_context.volume_mounts == container_context_camel_case_volumes.volume_mounts
assert container_context.volumes == container_context_camel_case_volumes.volumes


def test_merge(empty_container_context, container_context, other_container_context):
assert container_context.image_pull_policy == "Always"
assert container_context.image_pull_secrets == [{"name": "my_secret"}]
Expand All @@ -111,14 +147,14 @@ def test_merge(empty_container_context, container_context, other_container_conte
assert container_context.env_vars == ["MY_ENV_VAR"]
assert container_context.volume_mounts == [
{
"mountPath": "my_mount_path",
"mountPropagation": "my_mount_propagation",
"mount_path": "my_mount_path",
"mount_propagation": "my_mount_propagation",
"name": "a_volume_mount_one",
"readOnly": False,
"subPath": "path/",
"read_only": False,
"sub_path": "path/",
}
]
assert container_context.volumes == [{"name": "foo", "configMap": {"name": "settings-cm"}}]
assert container_context.volumes == [{"name": "foo", "config_map": {"name": "settings-cm"}}]
assert container_context.labels == {"foo_label": "bar_value"}
assert container_context.namespace == "my_namespace"

Expand Down Expand Up @@ -158,26 +194,26 @@ def test_merge(empty_container_context, container_context, other_container_conte
merged.volume_mounts,
[
{
"mountPath": "your_mount_path",
"mountPropagation": "your_mount_propagation",
"mount_path": "your_mount_path",
"mount_propagation": "your_mount_propagation",
"name": "b_volume_mount_one",
"readOnly": True,
"subPath": "your_path/",
"read_only": True,
"sub_path": "your_path/",
},
{
"mountPath": "my_mount_path",
"mountPropagation": "my_mount_propagation",
"mount_path": "my_mount_path",
"mount_propagation": "my_mount_propagation",
"name": "a_volume_mount_one",
"readOnly": False,
"subPath": "path/",
"read_only": False,
"sub_path": "path/",
},
],
)
_check_same_sorted(
merged.volumes,
[
{"name": "bar", "configMap": {"name": "your-settings-cm"}},
{"name": "foo", "configMap": {"name": "settings-cm"}},
{"name": "bar", "config_map": {"name": "your-settings-cm"}},
{"name": "foo", "config_map": {"name": "settings-cm"}},
],
)
assert merged.labels == {"foo_label": "bar_value", "bar_label": "baz_value"}
Expand Down

0 comments on commit 7aef716

Please sign in to comment.