-
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
Modify BigQueryCreateExternalTableOperator to use updated hook function #24363
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)
|
@malthe @eladkal @alexInhert I see that issue #24416 is already merged, can we proceed with this PR? Thanks. |
Im away from laptop so cant check but if I rember correctly #24416 changed the name of the function that you import so you need to rebase and apply the needed changes here. |
I feel like I did something wrong here... not exactly familiar with the workflow to rebase. I first rebased my commit to the latest apache remote branch and then pulled changes from my remote branch, lastly made mods to the PR. Not sure why my code does not pass the up-to-date checker, can anyone advise what to do next? Many thanks. |
Rebasing is easiest with drop-down next to "Update branch". button. See the instructions on rebase in our CONTRIBUTING docs https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#id14 they have all details @cswpy . The worst case - take your changes and re-apply them manually on top of main if you are somehow lost with your branch. you can start a new PR and close this one - no problem with that. |
@potiuk I think it's ready to merge now :) Thanks for the tips. |
The test is failing I am afraid |
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. |
Still docs and tests failing. Please fix and rebase it :) |
18a4aab
to
0b08736
Compare
358ba5e
to
17aa7bf
Compare
Now the PR is failing some checks... I believe this is due to the new commits that has already merged into main, where I rebased my commit onto. Can you advise me on how to proceed? Thanks |
Just continue rebasing. It's unlikely broken tests will stay broken for a long time they are usually fixed quickly. You were just unlucky when you hit them. |
22558eb
to
94f54fe
Compare
Hi @potiuk , all checks passed 😃 |
ea0c13f
to
a032170
Compare
Awesome work, congrats on your first merged pull request! |
Fixes: #24160
Modified the unit tests to verify that
create_empty_table
is called at least once.^ 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.