amoghrajesh commented on code in PR #53477:
URL: https://github.com/apache/airflow/pull/53477#discussion_r2217563692
##########
providers/cncf/kubernetes/src/airflow/providers/cncf/kubernetes/operators/pod.py:
##########
@@ -1132,7 +1132,10 @@ def _build_find_pod_label_selector(self, context:
Context | None = None, *, excl
**self.labels,
**self._get_ti_pod_labels(context, include_try_number=False),
}
- label_strings = [f"{label_id}={label}" for label_id, label in
sorted(labels.items())]
+ label_strings = [
+ f"{label_id}={str(label) if label is not None else ''}"
+ for label_id, label in sorted(labels.items())
+ ]
Review Comment:
Empty labels are valid, but it doesn't make sense to have labels like:
`key=None`, that's what the old code did.
```
def old(labels):
label_strings = [f"{label_id}={label}" for label_id, label in
sorted(labels.items())]
return label_strings
old(l1)
Out[10]: ['empty_str=', 'foo=bar', 'hello=airflow', 'none_str=None']
```
New code fixes that:
```
def new(labels):
label_strings = [
f"{label_id}={str(label) if label is not None else ''}"
for label_id, label in sorted(labels.items())
]
return label_strings
l1 = {"foo": "bar", "hello": "airflow", "empty_str": "", "none_str": None}
new(l1)
Out[8]: ['empty_str=', 'foo=bar', 'hello=airflow', 'none_str=']
```
But the question is, why even allow: `"none_str": None` while initialising
the KPO? No objections against doing this:
```
k = KubernetesPodOperator(
labels={"foo": "bar", "hello": "airflow", "empty_str": "",
"none_str": ""},
```
But
```
k = KubernetesPodOperator(
labels={"foo": "bar", "hello": "airflow", "empty_str": "",
"none_str": None},
```
makes little to no sense to me.
--
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]
