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

Pass image_pull_policy in KubernetesPodOperator correctly #13289

Merged
merged 3 commits into from
Dec 24, 2020

Conversation

KevYuen
Copy link
Contributor

@KevYuen KevYuen commented Dec 23, 2020

image_pull_policy is not being passed into the V1Container in KubernetesPodOperator

This causes pods to not re-pull images even if the value is set.

image_pull_policy is not being passed into the V1Container in
KubernetesPodOperator.
@boring-cyborg boring-cyborg bot added the provider:cncf-kubernetes Kubernetes provider related issues label Dec 23, 2020
@KevYuen KevYuen changed the title pass image_pull_policy to V1Container pass image_pull_policy to V1Container in KubernetesPodOperator Dec 23, 2020
@KevYuen KevYuen changed the title pass image_pull_policy to V1Container in KubernetesPodOperator image_pull_policy in KubernetesPodOperator Dec 23, 2020
@github-actions github-actions bot added the okay to merge It's ok to merge this PR as it does not require more tests label Dec 23, 2020
@github-actions
Copy link

The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest master or amend the last commit of the PR, and push it with --force-with-lease.

@mock.patch("airflow.kubernetes.pod_launcher.PodLauncher.start_pod")
@mock.patch("airflow.kubernetes.pod_launcher.PodLauncher.monitor_pod")
@mock.patch("airflow.kubernetes.kube_client.get_kube_client")
def test_image_pull_policy_correctly_set(self, mock_client, monitor_mock, start_mock):
Copy link
Member

Choose a reason for hiding this comment

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

Shall we have one more test case, where we don't explicitly specify image_pull_policy then assert it against default value IfNotPresent ?

@github-actions github-actions bot removed the okay to merge It's ok to merge this PR as it does not require more tests label Dec 23, 2020
image_pull_policy should be IfNotPresent by default if
it's not set. The test ensure the correct value is passed
to the V1Container object.
@KevYuen KevYuen requested a review from XD-DENG December 24, 2020 16:21
Copy link
Member

@XD-DENG XD-DENG left a comment

Choose a reason for hiding this comment

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

Thanks @KevYuen !

We can merge this after the CI succeeds. I will take care of it.

Thanks for the contribution!

@github-actions github-actions bot added the okay to merge It's ok to merge this PR as it does not require more tests label Dec 24, 2020
@github-actions
Copy link

The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest master or amend the last commit of the PR, and push it with --force-with-lease.

@XD-DENG XD-DENG added this to the Airflow 2.0.1 milestone Dec 24, 2020
@XD-DENG XD-DENG added the type:bug-fix Changelog: Bug Fixes label Dec 24, 2020
@XD-DENG XD-DENG changed the title image_pull_policy in KubernetesPodOperator Pass image_pull_policy in KubernetesPodOperator correctly Dec 24, 2020
@XD-DENG XD-DENG merged commit 7a560ab into apache:master Dec 24, 2020
@KevYuen
Copy link
Contributor Author

KevYuen commented Dec 24, 2020

Thanks @XD-DENG! Much appreciated

@KevYuen KevYuen deleted the image-pull-policy-k8s-pod branch December 24, 2020 17:06
@XD-DENG
Copy link
Member

XD-DENG commented Dec 24, 2020

No problem ;-) Happy holidays ahead btw!

potiuk added a commit to PolideaInternal/airflow that referenced this pull request Dec 26, 2020
The apache#13289 fixed imagePullPolicy behaviour but broke master K8S
tests. This PR attempts to fix it.
potiuk added a commit that referenced this pull request Dec 26, 2020
The #13289 fixed imagePullPolicy behaviour but broke master K8S
tests. This PR attempts to fix it.
kaxil pushed a commit that referenced this pull request Jan 21, 2021
The #13289 fixed imagePullPolicy behaviour but broke master K8S
tests. This PR attempts to fix it.

(cherry picked from commit 6f246b0)
kaxil pushed a commit that referenced this pull request Jan 29, 2021
* pass image_pull_policy to V1Container

image_pull_policy is not being passed into the V1Container in
KubernetesPodOperator. This commit fixes this.

* add test for image_pull_policy not set

image_pull_policy should be IfNotPresent by default if
it's not set. The test ensure the correct value is passed
to the V1Container object.

(cherry picked from commit 7a560ab)
kaxil pushed a commit that referenced this pull request Feb 4, 2021
* pass image_pull_policy to V1Container

image_pull_policy is not being passed into the V1Container in
KubernetesPodOperator. This commit fixes this.

* add test for image_pull_policy not set

image_pull_policy should be IfNotPresent by default if
it's not set. The test ensure the correct value is passed
to the V1Container object.

(cherry picked from commit 7a560ab)
kaxil pushed a commit to astronomer/airflow that referenced this pull request Apr 23, 2021
The apache#13289 fixed imagePullPolicy behaviour but broke master K8S
tests. This PR attempts to fix it.

(cherry picked from commit 6f246b0)
(cherry picked from commit fd0fddc)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
okay to merge It's ok to merge this PR as it does not require more tests 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