-
Notifications
You must be signed in to change notification settings - Fork 16.4k
allow changing of config file from default for the KubernetesResourceBaseOperator #34819
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
Conversation
|
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst)
|
|
The failed tests look unrelated to this change, let me check. |
|
I close an reopen it to execute all the tests |
| @cached_property | ||
| def hook(self) -> KubernetesHook: | ||
| hook = KubernetesHook(conn_id=self.kubernetes_conn_id) | ||
| hook = KubernetesHook(conn_id=self.kubernetes_conn_id, config_file=self.config_file) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dont you think we should have a None check before using the config_file here? If not passed, it will go as None to the hook.
Also can you add some test coverage for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if it's null the kubernetes library fills in the default, the KubernetesPodOperator works the same way
https://github.com/apache/airflow/blob/main/airflow/providers/cncf/kubernetes/operators/pod.py#L301
https://github.com/apache/airflow/blob/main/airflow/providers/cncf/kubernetes/operators/pod.py#L518
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the existing tests for the modified operators in the last commit
|
tests are failing |
amoghrajesh
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chapm250 changes look good, you need to rebase and find out why tests are failing
Currently the KubernetesResourceBaseOperator doesn't allow changing from the default K8s config_file location (unlike the KubernetesPodOperator), this change allows changing the config_file location so airflow deployments with a non-default K8s config_file path can use the subclasses of the KubernetesResourceBaseOperator