-
Notifications
You must be signed in to change notification settings - Fork 16.4k
Add discoverability for triggers in provider.yaml #31576
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
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 think this check is not needed. It was aim to do the same as with check_hook_classes function but this function is to tests the connections of the hook. This is why we have it only for hooks.
I removed this function and renamed check_hook_classes to check_hook_connection_classes to avoid future confusion.
potiuk
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.
Yes. That should be enough
|
Can you please close this PR @eladkal and push your branch (and create new PR) from |
|
Ah. No. the explanation is simpler. You need to also update provider_info.schema.json @eladkal and it unfortunately means that it cannot be implemented this way without breaking compatibility. The "notification" object type has been added here: #29191 and relased in 2.6.0. The "provider_info" schema (unlike the provider.yaml" one ) is public-facing. Airflow expects the `provider_info" exposed by get_provider_info endpoint to conform to this schema. And it means that Airflow 2.6.* will not validate the provider_info properly. So this change will have to be done differently if we decide to do it. For example you could change the field from "notifications" to "airflow-notifications" and deprecate the "notifications" one. But it would be quite complex to implement. I am not sure if it is worth it. |
|
@potiuk I can limit this PR to modify only the triggers and recheck later on the notifications. |
|
Yep. |
|
Work as expected. It found entries merged yesterday that were missing: |
|
@potiuk i think we are set here |
This PR adds/simplify the usage of notifications & triggers in provider.yaml by making their definition the same as operators/hooks
related: #31443
related: #29918