amoghrajesh commented on code in PR #53477:
URL: https://github.com/apache/airflow/pull/53477#discussion_r2381196865
##########
providers/cncf/kubernetes/src/airflow/providers/cncf/kubernetes/operators/pod.py:
##########
@@ -1193,6 +1193,7 @@ def _build_find_pod_label_selector(self, context: Context
| None = None, *, excl
**self.labels,
**self._get_ti_pod_labels(context, include_try_number=False),
}
+ labels = normalize_labels_dict(labels)
Review Comment:
```suggestion
labels = _normalize_labels_dict(labels)
```
##########
providers/cncf/kubernetes/src/airflow/providers/cncf/kubernetes/operators/pod.py:
##########
@@ -1421,3 +1422,9 @@ def __exit__(self, exctype, excinst, exctb) -> bool:
logger = logging.getLogger(__name__)
logger.exception(excinst)
return True
+
+
+# --- Helper functions ---
Review Comment:
```suggestion
```
##########
providers/cncf/kubernetes/tests/unit/cncf/kubernetes/operators/test_pod.py:
##########
@@ -1639,6 +1639,32 @@ def test_task_id_as_name_dag_id_is_ignored(self):
pod = k.build_pod_request_obj({})
assert re.match(r"a-very-reasonable-task-name-[a-z0-9-]+",
pod.metadata.name) is not None
+ def test_normalize_labels_dict_empty(self):
+ """normalize_labels_dict should return empty dict unchanged"""
+ from airflow.providers.cncf.kubernetes.operators.pod import
normalize_labels_dict
+
+ labels = {}
+ assert normalize_labels_dict(labels) == {}
+
+ def test_normalize_labels_dict_with_none(self):
+ """normalize_labels_dict should convert None values to empty strings"""
+ from airflow.providers.cncf.kubernetes.operators.pod import
normalize_labels_dict
+
+ labels = {"a": None, "b": "value", "c": None}
+ normalized = normalize_labels_dict(labels)
+ assert normalized == {"a": "", "b": "value", "c": ""}
+
+ def test_normalize_labels_dict_preserve_other_values(self):
+ """normalize_labels_dict should preserve falsy but non-None values"""
+ from airflow.providers.cncf.kubernetes.operators.pod import
normalize_labels_dict
+
+ labels = {"empty_str": "", "zero": 0, "false": False, "none": None}
+ normalized = normalize_labels_dict(labels)
+ assert normalized["empty_str"] == ""
+ assert normalized["zero"] == 0
+ assert normalized["false"] is False
+ assert normalized["none"] == ""
Review Comment:
Can we just add this as a single test with `pytest.parameterize`
(https://docs.pytest.org/en/stable/example/parametrize.html#parametrizing-tests)?
Just have a label and expected as two params
##########
providers/cncf/kubernetes/src/airflow/providers/cncf/kubernetes/operators/pod.py:
##########
@@ -1421,3 +1422,9 @@ def __exit__(self, exctype, excinst, exctb) -> bool:
logger = logging.getLogger(__name__)
logger.exception(excinst)
return True
+
+
+# --- Helper functions ---
+def normalize_labels_dict(labels: dict) -> dict:
Review Comment:
```suggestion
def _normalize_labels_dict(labels: dict) -> dict:
```
##########
providers/cncf/kubernetes/src/airflow/providers/cncf/kubernetes/operators/pod.py:
##########
Review Comment:
I would expect some test coverage for this particular function though. We
could replace the below tests to test the behaviour of this function instead
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
