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

Fix handling some None parameters in kubernetes 23 libs #21905

Merged
merged 1 commit into from
Mar 1, 2022

Conversation

potiuk
Copy link
Member

@potiuk potiuk commented Mar 1, 2022

Kubernetes 23.* is more picky when it comes to values passed to
Pod Generator - it requires:

  • imagePullPolicy
  • dnsPolicy
  • restartPolicy

to be not None.

We are fixing it in the way, that we simply skip setting those
if they are None.


^ 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.

@boring-cyborg boring-cyborg bot added the provider:cncf-kubernetes Kubernetes provider related issues label Mar 1, 2022
@potiuk potiuk changed the title Add missing parameters for Kubernetes 23 library tests Add missing parameters for Kubernetes 23.3. library tests Mar 1, 2022
@potiuk potiuk requested a review from kaxil March 1, 2022 17:25
@potiuk
Copy link
Member Author

potiuk commented Mar 1, 2022

This is the last one that failed our main. I think I implemented in a mostly backwards-compatible way. The only doubt I have to the "IfNotPresent" - because default this parameter might be different if "latest" image is used. Not sure if it is worth complicating as this is only for deprecated usage/PodGenerator. @jedcunningham @kaxil @dimberman - let me know what you think.

airflow/kubernetes/pod_generator_deprecated.py Outdated Show resolved Hide resolved
airflow/kubernetes/pod_generator_deprecated.py Outdated Show resolved Hide resolved
@potiuk potiuk force-pushed the more-picky-kuberenetes-library branch 2 times, most recently from ea1fc41 to 41ef3b8 Compare March 1, 2022 17:42
@potiuk potiuk requested a review from jedcunningham March 1, 2022 17:42
@potiuk
Copy link
Member Author

potiuk commented Mar 1, 2022

All should be fixed @jedcunningham

@potiuk potiuk force-pushed the more-picky-kuberenetes-library branch from 41ef3b8 to 27b9929 Compare March 1, 2022 18:13
@potiuk
Copy link
Member Author

potiuk commented Mar 1, 2022

Fixed. The tests are monstrous now :)

@potiuk potiuk requested a review from jedcunningham March 1, 2022 18:14
@potiuk potiuk force-pushed the more-picky-kuberenetes-library branch 2 times, most recently from 58ff7e7 to c75d04a Compare March 1, 2022 18:36
Copy link
Member

@jedcunningham jedcunningham left a comment

Choose a reason for hiding this comment

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

Probably worth updating the title and description here.

@potiuk
Copy link
Member Author

potiuk commented Mar 1, 2022

Sorry for that @jedcunningham. I think I need to go to sleep. :)

Kubernetes 23.* is more picky when it comes to values passed to
Pod Generator - it requires:

* imagePullPolicy
* dnsPolicy
* restartPolicy

to be not None.

We are fixing it in the way, that we simply skip setting those
if they are None.
@jedcunningham
Copy link
Member

All good 🍺

@github-actions github-actions bot added the full tests needed We need to run full set of tests for this PR to merge label Mar 1, 2022
@github-actions
Copy link

github-actions bot commented Mar 1, 2022

The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.

@potiuk potiuk force-pushed the more-picky-kuberenetes-library branch from c75d04a to 4557b34 Compare March 1, 2022 18:38
@potiuk potiuk changed the title Add missing parameters for Kubernetes 23.3. library tests Fix handling some None parameters in kubernetes 23 libs Mar 1, 2022
@potiuk
Copy link
Member Author

potiuk commented Mar 1, 2022

Probably worth updating the title and description here.

Done

@jedcunningham jedcunningham merged commit e856aba into apache:main Mar 1, 2022
@jedcunningham jedcunningham added this to the Airflow 2.2.5 milestone Mar 24, 2022
@jedcunningham jedcunningham added the type:bug-fix Changelog: Bug Fixes label Mar 24, 2022
potiuk added a commit that referenced this pull request Mar 26, 2022
Kubernetes 23.* is more picky when it comes to values passed to
Pod Generator - it requires:

* imagePullPolicy
* dnsPolicy
* restartPolicy

to be not None.

We are fixing it in the way, that we simply skip setting those
if they are None.

(cherry picked from commit e856aba)
ephraimbuddy pushed a commit that referenced this pull request Mar 26, 2022
Kubernetes 23.* is more picky when it comes to values passed to
Pod Generator - it requires:

* imagePullPolicy
* dnsPolicy
* restartPolicy

to be not None.

We are fixing it in the way, that we simply skip setting those
if they are None.

(cherry picked from commit e856aba)
ephraimbuddy pushed a commit that referenced this pull request Mar 26, 2022
Kubernetes 23.* is more picky when it comes to values passed to
Pod Generator - it requires:

* imagePullPolicy
* dnsPolicy
* restartPolicy

to be not None.

We are fixing it in the way, that we simply skip setting those
if they are None.

(cherry picked from commit e856aba)
potiuk added a commit that referenced this pull request Mar 28, 2022
@ephraimbuddy ephraimbuddy removed this from the Airflow 2.2.5 milestone Mar 28, 2022
ephraimbuddy pushed a commit that referenced this pull request Mar 28, 2022
ephraimbuddy pushed a commit that referenced this pull request Mar 28, 2022
@potiuk potiuk deleted the more-picky-kuberenetes-library branch July 29, 2022 20:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
full tests needed We need to run full set of tests for this PR to merge provider:cncf-kubernetes Kubernetes provider related issues type:bug-fix Changelog: Bug Fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants