potiuk commented on PR #68786: URL: https://github.com/apache/airflow/pull/68786#issuecomment-5154210082
The tests are welcome, but the one-line change in this PR is doing more than the title and description suggest, and I think it deserves to be the headline. ```python pod_cp = copy.deepcopy(pod) pod_cp.spec.volumes = pod.spec.volumes or [] # reads the original, not the copy pod_cp.spec.volumes.insert(0, PodDefaults.VOLUME) ``` Rebinding to the *original's* list defeats the deepcopy for that attribute, so the `insert` mutates the caller's pod: ``` before: caller pod after add_xcom_sidecar() -> ['XCOM_VOLUME', 'user-volume'] after: caller pod after add_xcom_sidecar() -> ['user-volume'] ``` `add_xcom_sidecar` deepcopies specifically so it does not touch its input, and that contract was silently broken for any pod that already had volumes. It stays invisible when there are none, because `or []` hands back a fresh list. Could you retitle and describe this as the bug fix it is, with the tests as supporting work? The description becomes the squash commit message, and as it stands someone bisecting a pod-mutation problem would never find this commit. Two practical notes, neither about the content. This branch has drifted a long way behind `main` — 1047 commits — and is now conflicting, so it needs a rebase before it can go anywhere. And this and #68788 both edit `xcom_sidecar.py` and `test_xcom_sidecar.py`, so whichever merges first will leave the other needing a second rebase. My suggestion is to land this one first: it is small and independently correct, and #68788's larger `PodDefaults` restructuring then rebases onto it rather than the other way round. I have left a note there too. --- Drafted-by: Claude Code (Opus 5); reviewed by @potiuk 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]
