-
Notifications
You must be signed in to change notification settings - Fork 13.8k
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
Coalesce extra
params to None in KubernetesHook
#19694
Coalesce extra
params to None in KubernetesHook
#19694
Conversation
When using UI form widgets FAB provides a empty string by default for every param. This turns out to make a difference sometimes. E.g. in this hook, we decide what to do depending on whether the param `is not None` -- and if you've created the connection in the UI, even though you didn't supply a value for this param, it will be `not None` In this case, I do not think this is a breaking change because e.g. if `kubeconfig_path` is empty string then loading it should fail. This should just allow better functioning of the hook.
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 feel like we should have a test for this too. I can see reversing this being an "optimization" later on.
reasonable |
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. |
When using UI form widgets FAB provides a empty string by default for every param. This turns out to make a difference sometimes. E.g. in this hook, we decide what to do depending on whether the param
is not None
-- and if you've created the connection in the UI, even though you didn't supply a value for this param, it will benot None
In this case, I do not think this is a breaking change because e.g. if
kubeconfig_path
is empty string then loading it should fail. This should just allow better functioning of the hook.