-
Notifications
You must be signed in to change notification settings - Fork 16.3k
Deferred KubernetesPodOperator pushes XCom on successful execution. #58488
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Deferred KubernetesPodOperator pushes XCom on successful execution. #58488
Conversation
| if self.do_xcom_push: | ||
| return xcom_sidecar_output |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be illegal syntax in a future Python version. We'll need to take that return out of finally.
https://peps.python.org/pep-0765/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AutomationDev85 the more I revisit the core the more I think this patch was not good. Actually you wanted to push XCom but with this you are masking the exception raise.
Can you re-work? I assume you to manually xcom_push() and then raise in case of error, else return XCom in this way only on success but not in finally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@johnslavik thanks for identifying this issue!! I created a PR to fix this issue: #58998
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume cause is that the ruff rule https://docs.astral.sh/ruff/rules/return-in-try-except-finally/ (SIM107) is not enabled in the repo, that should have catched this. Will put this on my bucket list to enable... or if somebody else wants to make a PR I assume this is an important check we are missing atm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will put this on my bucket list to enable... or if somebody else wants to make a PR I assume this is an important check we are missing atm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool. Thanks @johnslavik !
Overview
After upgrading to kubernetes provider 10.10.0, XCom was no longer pushed on successful execution of KubernetesPodOperator in deferrable mode. This PR restores XCom propagation for successful deferred runs.
Change Summary
trigger_reentryreturns XCom value whendo_xcom_pushis True and execution succeeds.Feedback is welcome.