[AIRFLOW-3251] KubernetesPodOperator does not use 'image_pull_secrets…

[victornoel]

…' argument

Make sure you have checked all steps below.

Jira

• My PR addresses the following Airflow Jira issues and references them in the PR title. For example, "[AIRFLOW-XXX] My Airflow PR"
  • https://issues.apache.org/jira/browse/AIRFLOW-3251

Description

• Here are some details about my PR, including screenshots of any UI changes:

I have chosen to not modify how image_pull_secrets were handled by KubernetesRequestFactory.extract_image_pull_secrets and so the pod operator takes a str and not a list str.

[ashb]

Would you be able to add some unit tests to ensure that the values are passed down appropriately?

[victornoel]

@ashb yes, but I'm not familiar with the tests of airflow. Where should they reside? In test_kubernetes_pod_operator.py? This looks like integration tests so I'm not sure.

[victornoel]

@ashb or I can test that a failure happen when an incorrect image_pull_secrets is used to create a pod (as with service_account_name).

[ashb]

Something like https://github.com/apache/incubator-airflow/blob/master/tests/contrib/minikube/test_kubernetes_pod_operator.py#L63-L85 (the mocking) looks like it would do.

[victornoel]

@ashb for now I've implemented a test that checks that a failure happen if there is an incorrect image_pull_secret, because I have trouble finding what should be mocked exactly to test this.

Sorry I'm not very familiar with python unit testing framework :)

[codecov-io]

Codecov Report

Merging #4188 into master will increase coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff            @@
##           master   #4188      +/-   ##
=========================================
+ Coverage   77.69%   77.7%   +<.01%     
=========================================
  Files         199     199              
  Lines       16309   16309              
=========================================
+ Hits        12672   12673       +1     
+ Misses       3637    3636       -1

Impacted Files	Coverage Δ
airflow/models.py	92.37% <0%> (+0.04%)	⬆️

---

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 8668ef8...b7bd02d. Read the comment docs.

[ashb]

This test alone seems a little bit hard-to-reason about.

Could you do something like assertRaisesRegexp(ApiException, r'invalid pull secrets') (or what ever string indicates the pull secrets are the problem - try to make it not too specific.) And perhaps a comment too so we know what this test was trying to test if it fails in the future.

[victornoel]

Thanks, anyway this doesn't work apparently, if it is not needed, the pull secret is not used it seems, I will think of another way to test this

[victornoel]

@ashb I have finally added a test using the mock to validate that the pod operator properly set the parameter on the pod.