shahar1 commented on code in PR #70342:
URL: https://github.com/apache/airflow/pull/70342#discussion_r3649646534
##########
providers/docker/src/airflow/providers/docker/operators/docker.py:
##########
@@ -489,7 +486,11 @@ def _copy_from_docker(self, container_id, src):
lib = getattr(self, "pickling_library", pickle)
return lib.load(file)
+ def _normalize_mounts(self) -> None:
+ self.mounts = [m if isinstance(m, Mount) else Mount(**m) for m in
self.mounts]
Review Comment:
`mounts` is a template field, and the SDK templater flattens **any** `dict`
subclass — `Mount` included — into a plain `dict`
(`task-sdk/src/airflow/sdk/definitions/_internal/templater.py`, the
`isinstance(value, dict)` branch). So a user-supplied `Mount` arrives here as
`{'Target': ..., 'Source': ..., 'Type': ..., 'ReadOnly': False}`. That is no
longer a `Mount`, so this takes the `else` branch and calls `Mount(**m)` with
API-cased keys:
```text
TypeError: Mount.__init__() got an unexpected keyword argument 'Target'. Did
you mean 'target'?
```
A rendered `Mount` is already in the shape docker-py wants, so it only needs
to be left alone:
```suggestion
# Rendering flattens a Mount into a plain dict with API-cased keys —
leave those alone.
self.mounts = [m if isinstance(m, Mount) or "Target" in m else
Mount(**m) for m in self.mounts]
```
I checked that this makes a rendered-`Mount` round-trip pass with the
existing docker operator tests still green. `DockerSwarmOperator` inherits the
method, so it is covered by the same change.
---
Drafted-by: Claude Code (Opus 5); reviewed by @shahar1 before posting
##########
providers/docker/tests/unit/docker/operators/test_docker.py:
##########
@@ -874,12 +874,16 @@ def
test_dict_mounts_are_normalized_to_mount_objects(self):
Mount(target="/logs", source="logs", type="volume"),
],
)
- assert all(isinstance(m, Mount) for m in op.mounts)
- assert op.mounts[0]["Target"] == "/data"
- assert op.mounts[0]["Source"] == "workspace"
- assert op.mounts[0]["Type"] == "volume"
- assert op.mounts[0]["ReadOnly"] is False
- assert op.mounts[1]["Target"] == "/logs"
+ assert not isinstance(op.mounts[0], Mount)
+
+ op.execute(None)
Review Comment:
This calls `execute()` without going through `render_templates()` first, so
the `Mount` in the fixture above is still a `Mount` when `_normalize_mounts`
runs and the `isinstance` guard short-circuits. The behaviour this PR changes
only shows up *after* rendering, so this test cannot catch a regression there —
which is why CI stays green on a path that raises `TypeError` at runtime.
Worth adding a case that renders first, then executes:
```python
@pytest.mark.db_test
def test_mount_objects_survive_render_then_execute(self,
create_task_instance_of_operator):
ti = create_task_instance_of_operator(
operator_class=DockerOperator,
dag_id="test", task_id="test", image="test", mount_tmp_dir=False,
mounts=[Mount(source="workspace",
target="/{{task_instance.run_id}}", type="volume")],
)
task = ti.render_templates()
task.execute(None)
passed = self.client_mock.create_host_config.call_args.kwargs["mounts"]
assert passed[0]["Target"] == f"/{ti.run_id}"
```
---
Drafted-by: Claude Code (Opus 5); reviewed by @shahar1 before posting
--
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]
