ashb commented on code in PR #54041:
URL: https://github.com/apache/airflow/pull/54041#discussion_r2256360411
##########
providers/cncf/kubernetes/tests/unit/cncf/kubernetes/decorators/test_kubernetes.py:
##########
@@ -128,3 +130,31 @@ def f(arg1, arg2, kwarg1=None, kwarg2=None):
# Second container is xcom image
assert containers[1].image == XCOM_IMAGE
assert containers[1].volume_mounts[0].mount_path == "/airflow/xcom"
+
+ def test_template_fields_order(self):
+ """Test that template_fields includes deterministic order."""
+ with self.dag:
+
+ @task.kubernetes(image="python:3.10-slim-buster")
+ def f():
+ return "test"
+
+ task_instance = f()
+
+ # Get the actual operator instance
+ operator = task_instance.operator
Review Comment:
That wasn't an task_instance (those only exist at runtime) -- it was
actually an XComArg object, but regardless:
```suggestion
operator = f().operator
```
##########
providers/cncf/kubernetes/src/airflow/providers/cncf/kubernetes/decorators/kubernetes.py:
##########
@@ -62,7 +62,7 @@ class _KubernetesDecoratedOperator(DecoratedOperator,
KubernetesPodOperator):
# `cmds` and `arguments` are used internally by the operator
template_fields: Sequence[str] = tuple(
- {"op_args", "op_kwargs", *KubernetesPodOperator.template_fields} -
{"cmds", "arguments"}
+ sorted({"op_args", "op_kwargs",
*KubernetesPodOperator.template_fields} - {"cmds", "arguments"})
Review Comment:
Instead f sorting here and in the KubernetesJobOperator -- shouldn't we
actually be sorting this on KubernetesPodOperator -- otherwise I think using
KPO directly would still result in hash changes.
##########
providers/cncf/kubernetes/tests/unit/cncf/kubernetes/operators/test_job.py:
##########
@@ -47,6 +47,7 @@
JOB_NAME = "test-job"
JOB_NAMESPACE = "test-namespace"
JOB_POLL_INTERVAL = 20.0
+JOB_TEMPLATE_FILE = "job_template.yaml"
Review Comment:
```suggestion
```
##########
providers/cncf/kubernetes/tests/unit/cncf/kubernetes/operators/test_job.py:
##########
@@ -106,6 +107,16 @@ def create_context(task, persist_to_db=False,
map_index=None):
@pytest.mark.db_test
@pytest.mark.execution_timeout(300)
class TestKubernetesJobOperator:
+ def test_template_fields_order(self):
+ """Test that template_fields includes deterministic order."""
+ job = KubernetesJobOperator(job_template_file=JOB_TEMPLATE_FILE,
task_id="task")
+
+ # Verify that job_template_file is in template_fields
+ assert "job_template_file" in job.template_fields
+
+ # Verify that all fields are ordered
+ assert list(job.template_fields) == sorted(job.template_fields)
+
Review Comment:
Just put `assert list(job.template_fields) == sorted(job.template_fields)`
in the existing `test_templates` test (if it's even needed -- see my previous
comment)
Also: as a convention, tests should be after the class level fixtures.
--
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]
