Skip to content
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

[AIRFLOW-5873] KubernetesPodOperator fixes and test #6524

Merged
merged 1 commit into from Nov 12, 2019

Conversation

ddelange
Copy link
Contributor

@ddelange ddelange commented Nov 8, 2019

Make sure you have checked all steps below.

Jira

Description

This PR implements the same as #6523 but is adapted to master branch:

Tests

  • My PR adds the following unit tests OR does not need testing for this extremely good reason:
    contrib/operators/test_kubernetes_pod_operator.py

@potiuk
Copy link
Member

potiuk commented Nov 12, 2019

Now just waiting for the master one to pass :)

- `security_context` was missing from docs of `KubernetesPodOperator`
- `KubernetesPodOperator` kwarg `in_cluster` erroneously defaults to
False in comparison to `default_args.py`, also default `do_xcom_push`
 was overwritten to False in contradiction to `BaseOperator`
- `KubernetesPodOperator` kwarg `resources` is erroneously passed to
 `base_operator`, instead should only go to `PodGenerator`. The two
 have different syntax. (both on `master` and `v1-10-test` branches)
- `kubernetes/pod.py`: classes do not have `__slots__`
 so they would accept arbitrary values in `setattr`
- Reduce amount of times the pod object is copied before execution
@codecov-io
Copy link

codecov-io commented Nov 12, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@13c539d). Click here to learn what that means.
The diff coverage is 94.11%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #6524   +/-   ##
=========================================
  Coverage          ?   83.62%           
=========================================
  Files             ?      645           
  Lines             ?    37291           
  Branches          ?        0           
=========================================
  Hits              ?    31186           
  Misses            ?     6105           
  Partials          ?        0
Impacted Files Coverage Δ
airflow/kubernetes/pod_generator.py 94.69% <ø> (ø)
airflow/kubernetes/pod.py 92.1% <100%> (ø)
airflow/utils/helpers.py 77.97% <100%> (ø)
...rflow/contrib/operators/kubernetes_pod_operator.py 98.57% <92.85%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 13c539d...8171cb2. Read the comment docs.

Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really nice set of fixes for KubernetesPodOperator

@@ -40,6 +40,8 @@ class Resources(K8SModel):
:param limit_gpu: Limits for GPU used
:type limit_gpu: int
"""
__slots__ = ('request_memory', 'request_cpu', 'limit_memory', 'limit_cpu', 'limit_gpu')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@potiuk potiuk merged commit cf38ddc into apache:master Nov 12, 2019
GnunuX pushed a commit to GnunuX/airflow that referenced this pull request Nov 13, 2019
- `security_context` was missing from docs of `KubernetesPodOperator`
- `KubernetesPodOperator` kwarg `in_cluster` erroneously defaults to
False in comparison to `default_args.py`, also default `do_xcom_push`
 was overwritten to False in contradiction to `BaseOperator`
- `KubernetesPodOperator` kwarg `resources` is erroneously passed to
 `base_operator`, instead should only go to `PodGenerator`. The two
 have different syntax. (both on `master` and `v1-10-test` branches)
- `kubernetes/pod.py`: classes do not have `__slots__`
 so they would accept arbitrary values in `setattr`
- Reduce amount of times the pod object is copied before execution
dimberman pushed a commit that referenced this pull request Jun 4, 2020
- `security_context` was missing from docs of `KubernetesPodOperator`
- `KubernetesPodOperator` kwarg `in_cluster` erroneously defaults to
False in comparison to `default_args.py`, also default `do_xcom_push`
 was overwritten to False in contradiction to `BaseOperator`
- `KubernetesPodOperator` kwarg `resources` is erroneously passed to
 `base_operator`, instead should only go to `PodGenerator`. The two
 have different syntax. (both on `master` and `v1-10-test` branches)
- `kubernetes/pod.py`: classes do not have `__slots__`
 so they would accept arbitrary values in `setattr`
- Reduce amount of times the pod object is copied before execution

(cherry picked from commit cf38ddc)
dimberman pushed a commit that referenced this pull request Jun 4, 2020
- `security_context` was missing from docs of `KubernetesPodOperator`
- `KubernetesPodOperator` kwarg `in_cluster` erroneously defaults to
False in comparison to `default_args.py`, also default `do_xcom_push`
was overwritten to False in contradiction to `BaseOperator`
- `KubernetesPodOperator` kwarg `resources` is erroneously passed to
`base_operator`, instead should only go to `PodGenerator`. The two
have different syntax. (both on `master` and `v1-10-test` branches)
- `kubernetes/pod.py`: classes do not have `__slots__`
so they would accept arbitrary values in `setattr`
- Reduce amount of times the pod object is copied before execution

(cherry picked from commit cf38ddc)
kaxil pushed a commit that referenced this pull request Jul 1, 2020
- `security_context` was missing from docs of `KubernetesPodOperator`
- `KubernetesPodOperator` kwarg `in_cluster` erroneously defaults to
False in comparison to `default_args.py`, also default `do_xcom_push`
was overwritten to False in contradiction to `BaseOperator`
- `KubernetesPodOperator` kwarg `resources` is erroneously passed to
`base_operator`, instead should only go to `PodGenerator`. The two
have different syntax. (both on `master` and `v1-10-test` branches)
- `kubernetes/pod.py`: classes do not have `__slots__`
so they would accept arbitrary values in `setattr`
- Reduce amount of times the pod object is copied before execution

(cherry picked from commit cf38ddc)
cfei18 pushed a commit to cfei18/incubator-airflow that referenced this pull request Mar 5, 2021
- `security_context` was missing from docs of `KubernetesPodOperator`
- `KubernetesPodOperator` kwarg `in_cluster` erroneously defaults to
False in comparison to `default_args.py`, also default `do_xcom_push`
was overwritten to False in contradiction to `BaseOperator`
- `KubernetesPodOperator` kwarg `resources` is erroneously passed to
`base_operator`, instead should only go to `PodGenerator`. The two
have different syntax. (both on `master` and `v1-10-test` branches)
- `kubernetes/pod.py`: classes do not have `__slots__`
so they would accept arbitrary values in `setattr`
- Reduce amount of times the pod object is copied before execution

(cherry picked from commit cf38ddc)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants