PTAL @dimberman

@feng-tao what date this should be ready so it can land in 1.10.3 ? I'm asking so I could know what priority this PR should be for me.

@ashb may know better for the date.

@feng-tao @ashb @pgagnon this PR is ready for review, I've incorporated most of the requested changes here. Tests are finally passing, I've added documentation.

PS. I don't know how to setup unit testing locally with K8S, on my macOS, so it took me a while debugging with TravisCI. It was kinda annoying but not sure how can I fix that for future so I can run all tests locally on macOS...

@ashb I've applied Your feedback, and made unittest mocked rather than calling k8s and checking values back

@galuszkak sorry - that example wasn't so great - we now aren't actually testing the behaviour of the pod factory functions.

Somewhere in the tests I'd like to see something like

self.assertEqual(
     SOMETHING['spec']['containers'][0]['envFrom'],
     { 'secretRef': { 'name': 'secret-name' } },
)

or similar.

Maybe we can do this like this:

    @mock.patch("airflow.contrib.kubernetes.pod_launcher.PodLauncher._monitor_pod")
    @mock.patch("airflow.contrib.kubernetes.kube_client.get_kube_client")
    def test_envs_from_secrets(self, client_mock, _):
        # GIVEN
        from airflow.utils.state import State
        secrets = [Secret('env', None, "secret_name")]
        mock_resp = MagicMock()
        mock_resp.status.start_time = datetime.now()
        client_mock.create_namespaced_pod.return_value = mock_resp

        # WHEN
        k = KubernetesPodOperator(
            namespace='default',
            image="ubuntu:16.04",
            cmds=["bash", "-cx"],
            arguments=["echo 10"],
            secrets=secrets,
            labels={"foo": "bar"},
            name="test",
            task_id="task",
        )
        k.execute(None)
        # THEN

        req = client_mock.create_namespaced_pod.call_args[0][1]['req']

       self.assertEqual(
           req['spec']['containers'][0]['envFrom'],
            { 'secretRef': { 'name': 'secret-name' } },
       )

Note: this is untested, and we may need to mock some extra methods

@ashb can I understand reasoning behind that, why we want to test that now? None of the tests were testing output of KubernetesRequestFactory before, only classes like Pod/PodLauncher to check if fields there are correct.

If we want to start testing that, I believe there should be separate ticket to make sure we cover most of the cases of outputs of KubernetesRequestFactory that is actually producing what we want in every case. Because for now there isn't single assertion on product of that class, only abstraction on top of it.

@dimberman Final thoughts? I'm okay leaving my requested test change to #4952.

@ashb @dimberman hope now it's good to go! I've applied requested changes.

@ashb I've rebase Your changes.

Thanks - if I was paying more attention I could have merged it (and squashed it) on Github. The reason Why I changed it from __ to _ is that __ is generally reserved for Python internals.

For me __ always meant private (because it cannot be overwritten), and _ protected (because it can be overwritten). Mostly because of this:
https://docs.python.org/3.7/tutorial/classes.html#private-variables

But I get Your point, I think this is just part of preference, how to think about API design.

Oh I didn't know that __foo actually did something specific. Still: I don't think anything should be private. If an advanced user want's to subclass and change behaviour, let the,

@ashb I agree, I just take literally what @dimberman wanted as he would like this as private helper, which I took very literally ;).

Thanks for hard work, and a lot of those code review. It was a really good experience for me, although a little longer than expected ;). Again thank You!

Thanks for merging it! Big thanks for support and help for @FlyrInc !