Skip to content
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

Bring back broken "connection-type/hook-class-name" check #33662

Merged
merged 1 commit into from
Aug 23, 2023

Conversation

potiuk
Copy link
Member

@potiuk potiuk commented Aug 23, 2023

Continuation of #33640 - there were one more wrong check in the provider, that missed "hook-class-names" check


^ 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.rst or {issue_number}.significant.rst, in newsfragments.

@potiuk
Copy link
Member Author

potiuk commented Aug 23, 2023

One more fix to the provider.yaml pre-commit. It also missed the "hook-class-names" verification. This is why PinotHook has not been detected - this PR will only succeed after #33601 is merged. (But I wanted to add it now to have a proof that it would have detected it).

@eladkal
Copy link
Contributor

eladkal commented Aug 23, 2023

Why?
related: #24702

@potiuk
Copy link
Member Author

potiuk commented Aug 23, 2023

Why? related: #24702

In #24702 you removed code for "hook-class-names" check @eladkal :) . This PR fixes support for "connection-types" / "hook-class-name" : ).

The "hook-class-names" in PR title was just a mental shortcut :)

Continuation of apache#33640 - there were one more wrong check in
the provider, that missed "hook-class-names" check
@potiuk potiuk force-pushed the fix-wrong-hook-class-names-check branch from c7fc828 to b66a3eb Compare August 23, 2023 18:40
@potiuk potiuk changed the title Bring back wrong "hook-class-names" check Bring back wrong "connection-type/hook-class-name" check Aug 23, 2023
@potiuk potiuk changed the title Bring back wrong "connection-type/hook-class-name" check Bring back broken "connection-type/hook-class-name" check Aug 23, 2023
@potiuk
Copy link
Member Author

potiuk commented Aug 23, 2023

@eladkal

Bring back broken "connection-type/hook-class-name" check

This is a much better title to not confuse it with the removed "hook-class-names" entry.

@potiuk potiuk merged commit 4037d79 into apache:main Aug 23, 2023
@potiuk potiuk deleted the fix-wrong-hook-class-names-check branch August 23, 2023 19:15
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants