-
Notifications
You must be signed in to change notification settings - Fork 14.5k
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
AirbyteHook
add cancel job option
#24593
Conversation
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst)
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also please fix static check errors.
AirbyteHook
add cancel job option
@uranusjr Thank you for reviewing the changes and feedback. Could you review the latest commit ? |
The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest main or amend the last commit of the PR, and push it with --force-with-lease. |
@uranusjr is there anything pending from my side ? |
No; we need a second reviewer’s approval here to merge (project policy), so all we can do for now is to wait. |
Actually @uranusjr - the policy is only for core changes - not providers. Providers are cool to merge with one committer. I often do so - anyhow there is always additional step of verification at release candidate time when I ask people to test their changes so we have this extra "gate" before release. @sivankumar86 - you actually need to fix things. static checks and test are failing and it seems relevant to your changes. Install pre-commit and it will fix the static checks for you mostly. |
@potiuk Thank you for your feedback and update. Could you review and approve it ? |
@uranusjr Could you please approve the workflow ? |
Timeout on sdist building has been fixed in main already. Merging. |
Awesome work, congrats on your first merged pull request! |
closes: #24574
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code change, 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 newsfragement file, named
{pr_number}.significant.rst
, in newsfragments.