Add pod_override setting for KubernetesExecutor - #10756
Conversation
a6421f9 to
7871dc6
Compare
|
@potiuk @kaxil @turbaszek can I get a review on this? :) |
7871dc6 to
38e90af
Compare
There was a problem hiding this comment.
| k8s_object = obj.get("podOverride", None) | |
| k8s_object = obj.get("pod_override", None) |
I think using snake_case might be better, wdyt?
There was a problem hiding this comment.
@kaxil sure, I was trying to keep in line with the original key "KubernetesExecutor", but I'm fine with that to be more consistent with pythonic best practices.
|
Can you add more docs about this feature? Code cleaning is also a good time to fill gaps in the documentation. ;-) |
There was a problem hiding this comment.
Do we really need a separate file here? We just want to deprecate but still support KubernetesExecutor config. The code you delete in airflow/kubernetes/pod_generator.py can just go in a separate function or a static method (or just in that if block) in the same file, just prefix it with _
There was a problem hiding this comment.
@kaxil We need a separate file because in the simplify_kubernetes_executor PR we change a lot more stuff in the pod_generator file and it is safest to just keep it separated. This PR is mostly about simplifying that PR.
There was a problem hiding this comment.
aah should I review that first? Can you tell me the dependencies in the PR.
There was a problem hiding this comment.
Or we should separate the "new module" bit in a different PR because more reviewers might face the same question
There was a problem hiding this comment.
concepts.rst should have only the core concepts. Additional information specific to a specific topic should be included in the topic article. For example, queues are unique to CeleryExecutor, so they are described in CeleryExecutor.
There was a problem hiding this comment.
Can you use :class:: role when you refer to class for the first time?
There was a problem hiding this comment.
There was a problem hiding this comment.
Can you use an literalicnlude or exampleinclude directive to include code snippets? This makes the code easier to maintain as all automatic checks can verify it. Ideally this example is covered by integration/system tests to be sure it is working properly. All guides for Google services are made this way, so you can see it in practice.
38dea80 to
892c17b
Compare
Users of the KubernetesExecutor will now have a "podOverride" option in the executor_config. This option will allow users to modify the pod launched by the KubernetesExecutor using a `kubernetes.client.models.V1Pod` class. This is the first step in deprecating the tradition executor_config.
892c17b to
0c30ef2
Compare
Users of the KubernetesExecutor will now have a "podOverride"
option in the executor_config. This option will allow users to
modify the pod launched by the KubernetesExecutor using a
kubernetes.client.models.V1Podclass. This is the first stepin deprecating the tradition executor_config.
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.