Improve provider verification pre-commit #33640
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Note! This PR will fail and it will nicely show all the kinds of problems detected by the script now - I am submitting separate PRs to fix problems found here and when we merge them, this one should become green. |
potiuk
added a commit
to potiuk/airflow
that referenced
this pull request
Aug 23, 2023
The Slack Notification listed in provider.yaml for slack has been still using the deprecated version of it. Since the provider verification of ours did not check neither notifications nor warnings, it was not found before. Found in the attempt of adding new verification in apache#33608 then also detected by apache#33640 that improved existing verification.
potiuk
added a commit
to potiuk/airflow
that referenced
this pull request
Aug 23, 2023
Found when implementing apache#33640
potiuk
force-pushed
the
improve-provider-verification-pre-commit
branch
from
ff72e9a to
eecb68b
Compare
|
Those are example errors printed now when the script fails: 
|
potiuk
force-pushed
the
improve-provider-verification-pre-commit
branch
from
eecb68b to
6fc8a09
Compare
potiuk
added a commit
that referenced
this pull request
Aug 23, 2023
…33643) The Slack Notification listed in provider.yaml for slack has been still using the deprecated version of it. Since the provider verification of ours did not check neither notifications nor warnings, it was not found before. Found in the attempt of adding new verification in #33608 then also detected by #33640 that improved existing verification.
7 tasks
potiuk
added a commit
to potiuk/airflow
that referenced
this pull request
Aug 23, 2023
The Dataproc Links have been deprecated, but they were deprecated wrongly - the Deprecation Warnings were always raised when the dataproc module has been deprecated, but only few classes there were deprecated. This was because the warnings were added as class-level warnings rather than as warnings in constructors. Also the deprecated classes have still been used in operators, raising the warnings even if the deprecation warnings have been moved from class to it's constructor. This PR fixes that: * moves deprecation warnings from classes to constructors * replaces usage of deprecated links with those links that replaced the deprecated ones Found during implementing of apache#33640
potiuk
force-pushed
the
improve-provider-verification-pre-commit
branch
from
6fc8a09 to
1c03797
Compare
There were a numer of problems with the provider verification pre-commit scripts: * It missed verification of "notifications" * It did not check if the classes or modules specified in provider yaml raised deprecation warnings * The messages produced by the script when some discrepancies were found were pretty cryptic and it was difficult to guess what kind of action should be taken to fix the problem This PR fixes all those problems: * verification of notification is performed * when importing all the classes and modules, check for the AirflowProviderDeprecationWarnings is done and treated as error * The messages produced provide clear actionable instructions on what to do and explain what are the discrepancies of expected vs. current list in a clear way
potiuk
force-pushed
the
improve-provider-verification-pre-commit
branch
from
1c03797 to
891c701
Compare
|
I decided to give up on "undeprecating DataProcLink" for now (too risky) - just added a feature of "Known deprecated classes" instead. |
|
Google team will take care about the DataprocLink some time soon-ish cc: @VladaZakharova :) |
|
flaky test only, Merging. |
potiuk
added a commit
to potiuk/airflow
that referenced
this pull request
Aug 23, 2023
Continuation of apache#33640 - there were one more wrong check in the provider, that missed "hook-class-names" check
potiuk
added a commit
that referenced
this pull request
Aug 23, 2023
Continuation of #33640 - there were one more wrong check in the provider, that missed "hook-class-names" check
potiuk
added a commit
to potiuk/airflow
that referenced
this pull request
Aug 24, 2023
This one completes review and check of provider.yaml verifications. There was one more check - for duplicates - that did not work as expected. It was hiding errors detected. This PR fixes it and also adds a bit more diagnostics messagese on what is actually being checked to give a bit more clue if some check is not doing what it is supposed to be doing. Follow up after apache#33662 and apache#33640
potiuk
added a commit
that referenced
this pull request
Aug 24, 2023
This one completes review and check of provider.yaml verifications. There was one more check - for duplicates - that did not work as expected. It was hiding errors detected. This PR fixes it and also adds a bit more diagnostics messagese on what is actually being checked to give a bit more clue if some check is not doing what it is supposed to be doing. Follow up after #33662 and #33640
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There were a numer of problems with the provider verification pre-commit scripts:
This PR fixes all those problems:
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named
{pr_number}.significant.rstor{issue_number}.significant.rst, in newsfragments.