Skip to content

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Jun 2, 2023
Merged

Add discoverability for triggers in provider.yaml #31576

merged 5 commits into from
Jun 2, 2023

Conversation

eladkal
Copy link
Contributor

@eladkal eladkal commented May 27, 2023 •
edited
Loading

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

@boring-cyborg boring-cyborg bot added area:dev-tools provider:cncf-kubernetes Kubernetes (k8s) provider related issues area:providers provider:Apache provider:google Google (including GCP) related issues labels May 27, 2023
@eladkal eladkal mentioned this pull request May 27, 2023
Closed
7 tasks
eladkal
eladkal commented May 27, 2023
View reviewed changes
scripts/in_container/run_provider_yaml_files_check.py
Comment on lines -309 to -322
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
)
Copy link
Contributor Author

@eladkal eladkal May 27, 2023 •
edited
Loading

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.

@eladkal eladkal marked this pull request as ready for review May 27, 2023 07:59
potiuk
potiuk approved these changes May 27, 2023
View reviewed changes
Copy link
Member

@potiuk potiuk left a 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

@potiuk
Copy link
Member

potiuk commented May 27, 2023 •
edited
Loading

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 in this PR 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 theapache repo, it should go a bit different path and it would validate my suspicion.

@potiuk
Copy link
Member

potiuk commented May 27, 2023

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.

@eladkal
Copy link
Contributor Author

eladkal commented Jun 1, 2023

@potiuk I can limit this PR to modify only the triggers and recheck later on the notifications.
would that work for you?

@potiuk
Copy link
Member

potiuk commented Jun 1, 2023

Yep.

@eladkal eladkal changed the title Add discoverability for notifications and triggers in provider.yaml Add discoverability for triggers in provider.yaml Jun 1, 2023
uranusjr
uranusjr reviewed Jun 1, 2023
View reviewed changes
@eladkal
Copy link
Contributor Author

eladkal commented Jun 1, 2023

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'

@eladkal eladkal requested a review from potiuk June 1, 2023 17:47
@eladkal
Copy link
Contributor Author

eladkal commented Jun 1, 2023

@potiuk i think we are set here

@eladkal eladkal merged commit dc5bf3f into apache:main Jun 2, 2023
@eladkal eladkal deleted the ci branch June 2, 2023 11:20
@eladkal eladkal added this to the Airflow 2.6.2 milestone Jun 8, 2023
@eladkal eladkal added changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) and removed changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) labels Jun 8, 2023
@eladkal eladkal removed this from the Airflow 2.6.2 milestone Jun 8, 2023
@eladkal eladkal mentioned this pull request Jun 20, 2023
Closed
86 tasks
@ephraimbuddy ephraimbuddy added the changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) label Jul 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@uranusjr uranusjr uranusjr approved these changes

@pankajkoti pankajkoti pankajkoti approved these changes

@ashb ashb Awaiting requested review from ashb ashb is a code owner

@o-nikolas o-nikolas Awaiting requested review from o-nikolas

@josh-fell josh-fell Awaiting requested review from josh-fell

@jedcunningham jedcunningham Awaiting requested review from jedcunningham

@potiuk potiuk Awaiting requested review from potiuk potiuk is a code owner

Assignees
No one assigned
Labels
area:dev-tools area:providers changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) provider:cncf-kubernetes Kubernetes (k8s) provider related issues provider:google Google (including GCP) related issues
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@eladkal @potiuk @uranusjr @pankajkoti @ephraimbuddy