You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "eladkal (via GitHub)" <gi...@apache.org> on 2023/05/27 07:55:35 UTC
[GitHub] [airflow] eladkal opened a new pull request, #31576: Add discoverability for notifications and triggers in provider.yaml
eladkal opened a new pull request, #31576:
URL: https://github.com/apache/airflow/pull/31576
This PR adds/simplify the usage of notifications & triggers in provider.yaml by making their defintion the same as operators/hooks
related: https://github.com/apache/airflow/issues/31443
related: https://github.com/apache/airflow/issues/29918
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] eladkal merged pull request #31576: Add discoverability for triggers in provider.yaml
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal merged PR #31576:
URL: https://github.com/apache/airflow/pull/31576
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] eladkal commented on a diff in pull request #31576: Add discoverability for notifications and triggers in provider.yaml
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on code in PR #31576:
URL: https://github.com/apache/airflow/pull/31576#discussion_r1207767945
##########
scripts/in_container/run_provider_yaml_files_check.py:
##########
@@ -306,22 +311,6 @@ def check_hook_classes(yaml_files: dict[str, dict]):
)
-def check_trigger_classes(yaml_files: dict[str, dict]):
- print("Checking triggers classes belong to package, exist and are classes")
- resource_type = "triggers"
- for yaml_file_path, provider_data in yaml_files.items():
- provider_package = pathlib.Path(yaml_file_path).parent.as_posix().replace("/", ".")
- trigger_classes = {
- name
- for trigger_class in provider_data.get(resource_type, {})
- for name in trigger_class["class-names"]
- }
- if trigger_classes:
- check_if_objects_exist_and_belong_to_package(
- trigger_classes, provider_package, yaml_file_path, resource_type, ObjectType.CLASS
- )
Review Comment:
This 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] eladkal commented on pull request #31576: Add discoverability for notifications and triggers in provider.yaml
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on PR #31576:
URL: https://github.com/apache/airflow/pull/31576#issuecomment-1571417317
@potiuk I can limit this PR to modify only the triggers and recheck later on the notifications.
would that work for you?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] eladkal commented on pull request #31576: Add discoverability for triggers in provider.yaml
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on PR #31576:
URL: https://github.com/apache/airflow/pull/31576#issuecomment-1572523830
@potiuk i think we are set here
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #31576: Add discoverability for notifications and triggers in provider.yaml
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #31576:
URL: https://github.com/apache/airflow/pull/31576#issuecomment-1565702149
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: https://github.com/apache/airflow/pull/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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] eladkal commented on a diff in pull request #31576: Add discoverability for notifications and triggers in provider.yaml
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on code in PR #31576:
URL: https://github.com/apache/airflow/pull/31576#discussion_r1207767945
##########
scripts/in_container/run_provider_yaml_files_check.py:
##########
@@ -306,22 +311,6 @@ def check_hook_classes(yaml_files: dict[str, dict]):
)
-def check_trigger_classes(yaml_files: dict[str, dict]):
- print("Checking triggers classes belong to package, exist and are classes")
- resource_type = "triggers"
- for yaml_file_path, provider_data in yaml_files.items():
- provider_package = pathlib.Path(yaml_file_path).parent.as_posix().replace("/", ".")
- trigger_classes = {
- name
- for trigger_class in provider_data.get(resource_type, {})
- for name in trigger_class["class-names"]
- }
- if trigger_classes:
- check_if_objects_exist_and_belong_to_package(
- trigger_classes, provider_package, yaml_file_path, resource_type, ObjectType.CLASS
- )
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #31576: Add discoverability for notifications and triggers in provider.yaml
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #31576:
URL: https://github.com/apache/airflow/pull/31576#issuecomment-1571458566
Yep.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] eladkal commented on pull request #31576: Add discoverability for triggers in provider.yaml
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on PR #31576:
URL: https://github.com/apache/airflow/pull/31576#issuecomment-1571618031
Work as expected. It found entries merged yesterday that were missing:
```
Found 1 errors in providers
Error: Incorrect content of key 'triggers/python-modules' in file: airflow/providers/amazon/provider.yaml
-- Items in the left set but not the right:
'airflow.providers.amazon.aws.triggers.glue'
'airflow.providers.amazon.aws.triggers.glue_crawler'
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a diff in pull request #31576: Add discoverability for triggers in provider.yaml
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #31576:
URL: https://github.com/apache/airflow/pull/31576#discussion_r1212761754
##########
airflow/provider.yaml.schema.json:
##########
@@ -208,8 +208,8 @@
"type": "string",
"description": "Integration name. It must have a matching item in the 'integration' section of any provider."
},
- "class-names": {
- "description": "List of the trigger class name that implements trigger.",
+ "python-modules": {
+ "description": "List of python modules containing the triggers.",
Review Comment:
```suggestion
"description": "List of Python modules containing the triggers.",
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #31576: Add discoverability for notifications and triggers in provider.yaml
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #31576:
URL: https://github.com/apache/airflow/pull/31576#issuecomment-1565660983
Can you please close this PR @eladkal and push your branch (and create new PR) from `apache` repository instead of your own repo? I believe the reason for the problem is that validation schema used in this PR is the old one. I am not sure why exactly, but if you can push it to the`apache` repo, it should go a bit different path and it would validate my suspicion.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org