Uh oh!
There was an error while loading. Please reload this page.
Move container-related functions from PodManager to a separate file - #56700
Conversation
65b55e1 to
3729539Compare
jscheffl
left a comment
There was a problem hiding this comment.
Thanks for the refactoring. Looking good to me. Just one nit as comment.
I leave this PR open for some days and hope another pair of eyes makes another pass before merge. But I assume this is a non critical change.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
…pache#56700) * Move container-related functions from PodManager to a separate file * Moved unit tests * Optimize redundant container status checks --------- Co-authored-by: AutomationDev85 <AutomationDev85>
| terminal_states = {FAILED, SUCCEEDED} | ||
| class PodOperatorHookProtocol(Protocol): |
There was a problem hiding this comment.
is this considered part of the public API?
There was a problem hiding this comment.
The question is if it's part of our public API or not.
I don't know if this can be used in some user custom code (operator, executor)?
There was a problem hiding this comment.
I think this question is not the one that should be asked - hence I answered the righ one. I don't think we ever specified what is and what is not public API.
But generally it's pretty much the same - Breaking = Public API change in a breaking way. I have not found it to be used elsewhere in our code except cncf.kubernetes - so I assume the intention was to not be used outside of it -> hence not breaking -> hence no public API.
BTW, Semver is not about whether something is a public API or not (because often it is not clearly specified) - but about whether intention of it was to be used as such.
So actually the proper question (if we want to be picky and ask really good question) to ask is "was that intended to be used outside of the provider and changes it in a breaking way?" , And my answer is "I don't think so".
There was a problem hiding this comment.
Sorry was distracted all day.
Technically it was not decleared private but I agree to Jarek that we did not define the "public API" on providers. Had the same question if this is breaking as well and did not find any other reference in "our" codebose.
The Protocol is not documented elsewhere and is not needed if you implement operators/Dags or so. So a "normal user" should not see this being removed. I assume every "Advanced user" that potentially builds around the package and extends the operators and needs this would know how to adjust but would be a very nice use case/extension. As GKE was also not using it I think it is safe to remove.
Overview
We are preparing an update to the KubernetesPodTriggerer workflow to align its startup behavior with that of the synchronous workflow. As part of this effort, we are introducing this preparation PR.
This PR moves certain container-related functions from PodManager to a new container.py module. This refactoring helps resolve circular import issues, as both the Hook and PodManager can now import these shared functions from container.py. In upcoming PRs, we plan to unify the code paths so that both the Triggerer (async) and synchronous workflows use the same logic to start pods. This change also lays the groundwork for introducing an AsyncPodManager that will utilize functions from the AsyncKubernetesHook.
We welcome your feedback on this change
Details of change: