Changes based on PR comments

metaflow/plugins/kubernetes/kubernetes_cli.py

@@ -186,9 +186,8 @@ def echo(msg, stream="stderr", job_id=None, **kwargs):
 
     # `labels` is a tuple of strings or a tuple with a single comma separated string
     # convert it to a dict
-    labels = KubernetesDecorator.validate_kube_labels(
-        KubernetesDecorator.parse_kube_keyvalue_list(labels, False)
-    )
+    labels = KubernetesDecorator.parse_kube_keyvalue_list(labels, False)
+    KubernetesDecorator.validate_kube_labels(labels)
 
     def _sync_metadata():
         if ctx.obj.metadata.TYPE == "local":

metaflow/plugins/kubernetes/kubernetes_decorator.py

@@ -112,7 +112,7 @@ def __init__(self, attributes=None, statically_defined=False):
             self.attributes["labels"] = self.parse_kube_keyvalue_list(
                 self.attributes["labels"].split(","), False
             )
-        self.attributes["labels"] = self.validate_kube_labels(self.attributes["labels"])
+        self.validate_kube_labels(self.attributes["labels"])
 
         if isinstance(self.attributes["node_selector"], str):
             self.attributes["node_selector"] = self.parse_kube_keyvalue_list(
@@ -441,21 +441,20 @@ def parse_kube_keyvalue_list(items: List[str], requires_both: bool = True):
     @staticmethod
     def validate_kube_labels(
         labels: Optional[Dict[str, Optional[str]]],
-        regex_match: str = r"^(([A-Za-z0-9][-A-Za-z0-9_.]{0,61})?[A-Za-z0-9])?$",
-    ):
+    ) -> bool:
         def validate_label(s: Optional[str]):
+            regex_match: str = r"^(([A-Za-z0-9][-A-Za-z0-9_.]{0,61})?[A-Za-z0-9])?$"
             if not s:
                 # allow empty label
-                return s
+                return True
             if not re.search(regex_match, s):
                 # this is the same message kubernetes itself returns
-                raise Exception(
+                raise KubernetesException(
                     f'Invalid value: "{s}": a valid label must be an empty string or '
-                    "consist of alphanumeric characters, '-', '_' or '.', and must "
+                    "consist of alphanumeric characters, '-', '_' or '.', must "
                     "start and end with an alphanumeric character (e.g. 'MyValue', "
-                    "or 'my_value', or '12345', regex used for validation is "
-                    "'^(([A-Za-z0-9][-A-Za-z0-9_.]{0,61})?[A-Za-z0-9])?$'"
+                    "or 'my_value', or '12345'), and be 63 characters or less"
                 )
-            return s
+            return True
 
-        return {k: validate_label(v) for k, v in labels.items()} if labels else labels
+        return all([validate_label(v) for v in labels.values()]) if labels else True

test/unit/test_kubernetes_decorator.py

@@ -1,5 +1,6 @@
 import pytest
 
+from metaflow.plugins.kubernetes.kubernetes import KubernetesException
 from metaflow.plugins.kubernetes.kubernetes_decorator import KubernetesDecorator
 
 
@@ -37,8 +38,7 @@
     ],
 )
 def test_kubernetes_decorator_validate_kube_labels(labels):
-    cleaned_labels = KubernetesDecorator.validate_kube_labels(labels)
-    assert cleaned_labels == labels
+    assert KubernetesDecorator.validate_kube_labels(labels)
 
 
 @pytest.mark.parametrize(
@@ -64,5 +64,5 @@ def test_kubernetes_decorator_validate_kube_labels(labels):
 )
 def test_kubernetes_decorator_validate_kube_labels_fail(labels):
     """Fail if label contains invalid characters or is too long"""
-    with pytest.raises(Exception):
+    with pytest.raises(KubernetesException):
         KubernetesDecorator.validate_kube_labels(labels)