airflow
airflow copied to clipboard
bugfix: tweak support for project_id argument in BigQueryGetDataOperator
This PR is a small bugfix/tweak related to BigQueryGetDataOperator project_id implementation. After attempting to use the provided code, the operator wouldn't work because the get_schema method wasn't using the project_id parameter.
Also added the project_id to the log while attempting to debug this. Ended up being a nice to have in the operator.
Fix #25782
Edit: Ended up not creating a new branch in fork since I was excited to get my first PR in. If there is any changes that need to be made or I forgot anything, list them below and I will make them. Also didn't make any changes to the tests since I assumed this fix would be covered by the previous tests.
^ 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.
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) Here are some useful points:
- Pay attention to the quality of your code (flake8, mypy and type annotations). Our pre-commits will help you with that.
- In case of a new feature add useful documentation (in docstrings or in
docs/directory). Adding a new operator? Check this short guide Consider adding an example DAG that shows how users should use it. - Consider using Breeze environment for testing locally, it's a heavy docker but it ships with a working Airflow and a lot of integrations.
- Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
- Please follow ASF Code of Conduct for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
- Be sure to read the Airflow Coding style. Apache Airflow is a community-driven project and together we are making it better 🚀. In case of doubts contact the developers at: Mailing List: [email protected] Slack: https://s.apache.org/airflow-slack
Can you please fix static check and add/update a unit test to avoid regressions in the future ?
Can you please fix static check and add/update a unit test to avoid regressions in the future ?
Yep - I've adressed the static check locally. I'm looking in how to write the test since this is my first contribution. I'll post an update tomorrow.
Edit (29/08/2022) : Attempting to get this done this week.
Any updates @gustavom2998 :)?
Any updates @gustavom2998 :)?
Hey @potiuk - sorry about the delay. I had some troubles getting breeze working locally. Managed to get it working over the weekend. But I still have to learn how to properly write the test. I've been taking a look at the other tests and will try to run them locally. Didn't expect to run into this many problems with the local dev environment - Sorry.
Will try to get this in this week.
Any updates @gustavom2998 :)?
Hey @potiuk - sorry about the delay. I had some troubles getting breeze working locally. Managed to get it working over the weekend. But I still have to learn how to properly write the test. I've been taking a look at the other tests and will try to run them locally. Didn't expect to run into this many problems with the local dev environment - Sorry.
Will try to get this in this week.
No worries - I was just checking. This is OSS, and anyone does contributions when they can :). It was just more-or-less regular ping to see if things are not forgotten (seem they are not :D).
@potiuk any chance you could take a look now? Took a while but I think we're almost there :D
Looks good, but you need to fix statich checks
There was a merge conflict exactly on the operator I worked on - but hopefully I've solved the problem. I've ran the pre-commits locally on the affected files locally and didn't get any errors.
Still some error.
TIP: If you did not have pre-commit installed when made your commits, I recommend to squash all your changes into single commit, rebase it on top of the "main" and run breeze static-checks --last-commit - iti will then run all the necessary changes for all files modified in all commits squashed.
Hey @potiuk - thanks for the tip. I attempted to perform the squash, but since the have been many commits merged from the main repository to the fork - I had some problems performing the squash via rebase.
About the checks, I've been using the pre-commit run --all-files command to run the checks, not breeze - and I reran them and got the error. Not sure what happened yesterday - I may have forgotten to save the file before running the test. I've fixed it and I performed the pre-commit checks for all files again, everything seems to have passed.
Sorry about the amount of commits and the problems - but It's been quite a learning process. If this PR is too polluted, I can close it and open a new PR.
ository to the fork - I had some problems performing the squash via rebase.
No worries - it takes time to master, but I am afraid you will have to rebase anyway if yoy want to get it merged. I think right now your commit needs to have conflicts resolved and we won't be able to merge it before you resolve the conflicts.
About the checks, I've been using the
pre-commit run --all-filescommand to run the checks, not breeze - and I reran them and got the error. Not sure what happened yesterday - I may have forgotten to save the file before running the test. I've fixed it and I performed the pre-commit checks for all files again, everything seems to have passed.
The pre-comit run --all-files is fine, but it will take a lot of time (10 minutes on my 16-core PC). It's much faster to run it locally on only changed files. You should run pre-commit install and then it will run all the checks only for files changed in your commit - automatically every time when you do commit (thii is 100s times faster or so usually). You can also run it on sequence of commits pre-commit run --from-ref HASH_COMMIT1 --to-ref HASH_COMMIT2 which will run pre-commit for all changed files between those two commits and breeze is merely adding few extra shortcuts so --last-commit is just shortcut to --from-ref HEAD^ --to-ref HEAD to only last commit.
You can read more about it here: https://github.com/apache/airflow/blob/main/STATIC_CODE_CHECKS.rst#using-pre-commit
Following some of that, saves a lot of time.
Sorry about the amount of commits and the problems - but It's been quite a learning process. If this PR is too polluted, I can close it and open a new PR.
No problem at all :). No need to do it but you will need to rebase between merge.
BTW. If you have problem with conflict resolution but your change is small, sometimes indeed it is easier to apply your changes again to latest main. And it this case it is enough to recrete branch with the same name and --force-push it and the new branch will reaplace code in this PR, no need to open a new PR for that.
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.