Skip to content

Conversation

@dstandish
Copy link
Contributor

@dstandish dstandish commented Nov 19, 2021

Here we add a few params to KubernetesHook

        self.cluster_context = cluster_context
        self.config_file = config_file
        self.in_cluster = in_cluster

We also make it optional to use an airflow connection. In order to use k8s hook with k8s pod op, we need connection to be optional because up to now k8s pod operator does not use airflow connection.

For the new hook params, I parameterized the relevant tests and added a lot of cases in order to verify search path (hook param beats conn extra)

The principal motivator here is enabling the use of KubernetesHook with KubernetesPodOperator while maintaining backward compatibility.

@boring-cyborg boring-cyborg bot added provider:cncf-kubernetes Kubernetes (k8s) provider related issues area:providers labels Nov 19, 2021
@dstandish dstandish changed the title Add params to k8s hook Add config and context params to KubernetesHook Nov 19, 2021
@dstandish dstandish requested a review from turbaszek November 19, 2021 00:43
@dstandish dstandish force-pushed the add-params-to-k8s-hook branch from 13d72de to 5c79084 Compare November 19, 2021 02:38
Copy link
Contributor Author

Choose a reason for hiding this comment

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

it seems that merger and loader were backwards in some of these tests

@dstandish dstandish force-pushed the add-params-to-k8s-hook branch from f6ddbd0 to 4898392 Compare November 19, 2021 16:04
This is mainly needed in order to enable use KubernetesHook with KubernetesPodOperator while maintaining backw
ard compatibility.
@dstandish dstandish force-pushed the add-params-to-k8s-hook branch from 4898392 to 0c9da50 Compare November 19, 2021 16:14
@potiuk
Copy link
Member

potiuk commented Nov 24, 2021

Do you stll need feedback on it @dstandish ?

@dstandish
Copy link
Contributor Author

Do you stll need feedback on it @dstandish ?

Yes please, it's ready for review if you have the time. It's a stepping stone towards adding k8s hook to k8s pod op

@potiuk
Copy link
Member

potiuk commented Nov 24, 2021

Tomorrow morning :)

@jedcunningham
Copy link
Member

We also make it optional to use an airflow connection.

Do any other hooks have this behavior? I wonder if KPO should handle backwards compat instead?

@dstandish
Copy link
Contributor Author

dstandish commented Dec 1, 2021

We also make it optional to use an airflow connection.

Do any other hooks have this behavior? I wonder if KPO should handle backwards compat instead?

Well, SubprocessHook doesn't even take a connection id, and postgres and mysql will allow you to not supply a connection id but supply an actual Connection object instead.

To me it seems pretty reasonable because it's a situation where it would be very normal to not need a connection (cus you're in the cluster e.g.) but want to use the hook methods etc. It's not really about backwards compat in my view but making it flexible to use the hook without having defined a connection.

Co-authored-by: Jed Cunningham <66968678+jedcunningham@users.noreply.github.com>
dstandish and others added 5 commits December 1, 2021 14:55
Co-authored-by: Jed Cunningham <66968678+jedcunningham@users.noreply.github.com>
Co-authored-by: Jed Cunningham <66968678+jedcunningham@users.noreply.github.com>
Co-authored-by: Jed Cunningham <66968678+jedcunningham@users.noreply.github.com>
Co-authored-by: Jed Cunningham <66968678+jedcunningham@users.noreply.github.com>
@github-actions
Copy link

github-actions bot commented Dec 2, 2021

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 main or amend the last commit of the PR, and push it with --force-with-lease.

@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 2, 2021
@dstandish dstandish merged commit f9eab1c into apache:main Dec 6, 2021
dstandish added a commit to dstandish/airflow that referenced this pull request Dec 18, 2021
This refactor has the following goals:
* a simpler, easier-to-follow execute method; less logic and more readable method calls
* remove reliance on mutation of pod and namespace instance attributes
    - the `self.pod` attribute is no longer referenced or mutated anywhere outside of `execute`
    - the only reason we need the `pod` attribute at all is for `on_kill`
* reduce code duplication
* improve method names for greater transparency

allow usage of kubernetes hook (waiting on Add config and context params to KubernetesHook apache#19695 before implementing)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:providers okay to merge It's ok to merge this PR as it does not require more tests provider:cncf-kubernetes Kubernetes (k8s) provider related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants