Add pre-commit hook to validate provider spec

[uranusjr]

Inspired by #33601. This adds a pre-commit hook to ensure the provider.yaml specifications are valid when its surrounding provider changes. Two errors are found and fixed.

Since this obviously also needs to change the Pinot provider, this will unfortunately cause merge conflicts when either this or the other PR is merged. We’ll just need to resolve it when it happens.

[potiuk]

Hmm ... we already have pre_commit_check_provider_yaml_files.py which does a lot more check on various provider classes and even verifies whether all the classes and packages specified are properly named, imports them etc. etc.

Basically what it does is that it runs

python3 /opt/airflow/scripts/in_container/run_provider_yaml_files_check.py inside breeze and it runs a series of checks there:

    check_integration_duplicates(all_parsed_yaml_files)
    check_duplicates_in_integrations_names_of_hooks_sensors_operators(all_parsed_yaml_files)
    check_completeness_of_list_of_transfers(all_parsed_yaml_files)
    check_duplicates_in_list_of_transfers(all_parsed_yaml_files)
    check_hook_connection_classes(all_parsed_yaml_files)
    check_plugin_classes(all_parsed_yaml_files)
    check_extra_link_classes(all_parsed_yaml_files)
    check_correctness_of_list_of_sensors_operators_hook_modules(all_parsed_yaml_files)
    check_unique_provider_name(all_parsed_yaml_files)
    check_providers_have_all_documentation_files(all_parsed_yaml_files)

So I'd say either the check should be added there, or maybe some of the existing check should be improved there.

[potiuk]

Or if you prefer to reimplement the checks in a simpler way - maybe - then it's worth to replace the other checks with new ones.

BTW. The thing why it uses breeze is that it provides the consistent set of libraries and system level packages and imports that are always the same for everyone - so you can consistently run all kinds of imports and they are guaranteed to work as breeze has all the dependencies installed already.

[uranusjr]

I wonder if that existing hook already covers the same topics but is not currently triggered correctly.

[potiuk]

I wonder if that existing hook already covers the same topics but is not currently triggered correctly.

It should be triggred - especially that we are running it with --all-files in main. So I guess this must be either missing or buggy check there.

[uranusjr]

But we don’t run it with --all-files in CI, which causes the CI to be incorrectly green in PRs changing relevant files, and stay green in main after those changes are merged.

[potiuk]

But we don’t run it with --all-files in CI, which causes the CI to be incorrectly green in PRs changing relevant files, and stay green in main after those changes are merged.

We do. All the runs in CI are alwas with --all-files for all PRs.

https://github.com/apache/airflow/blob/main/.github/workflows/ci.yml#L537

[potiuk]

The problem is that we do not check deprecation warnigs there. Both

• airflow.providers.apache.pinot.hooks.pinot.PinotHook
• airflow.providers.slack.notifications.slack_notifier.SlackNotifier

were importable with deprecation warning - for backwards compatibility - but in the checks we have we have not checkded if deprecation warnings are raised for them.

[potiuk]

And we missed notifier checks in there.

[potiuk]

Improve provider verification script here: #33640

There were quite a few other errors detected by checking Deprecated warnings.

[potiuk]

Closing in favour of series of fixes:

• Exclude deprecated "operators.kubernetes_pod" module from provider.yaml #33641
• Fix Google Provider Link Deprecations #33642
• Replace deprecated slack notification in provider.yaml with new one #33643

Finally to be completed with improved verrification in

• Improve provider verification pre-commit #33640