You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2022/08/26 12:57:11 UTC
[GitHub] [airflow] gustavom2998 opened a new pull request, #25982: Main
gustavom2998 opened a new pull request, #25982:
URL: https://github.com/apache/airflow/pull/25982
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](https://github.com/apache/airflow/pull/25782)
-->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
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](https://github.com/apache/airflow/tree/main/newsfragments).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #25982: bugfix: tweak support for project_id argument in BigQueryGetDataOperator
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #25982:
URL: https://github.com/apache/airflow/pull/25982#issuecomment-1237411412
> > 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).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #25982: bugfix: tweak support for project_id argument in BigQueryGetDataOperator
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #25982:
URL: https://github.com/apache/airflow/pull/25982#issuecomment-1255943776
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 is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] gustavom2998 commented on pull request #25982: bugfix: tweak support for project_id argument in BigQueryGetDataOperator
Posted by GitBox <gi...@apache.org>.
gustavom2998 commented on PR #25982:
URL: https://github.com/apache/airflow/pull/25982#issuecomment-1236978648
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] gustavom2998 commented on pull request #25982: bugfix: tweak support for project_id argument in BigQueryGetDataOperator
Posted by GitBox <gi...@apache.org>.
gustavom2998 commented on PR #25982:
URL: https://github.com/apache/airflow/pull/25982#issuecomment-1255815562
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] boring-cyborg[bot] commented on pull request #25982: Main
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on PR #25982:
URL: https://github.com/apache/airflow/pull/25982#issuecomment-1228455278
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]( https://github.com/apache/airflow/blob/main/STATIC_CODE_CHECKS.rst#prerequisites-for-pre-commit-hooks) 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](https://github.com/apache/airflow/blob/main/docs/apache-airflow/howto/custom-operator.rst) Consider adding an example DAG that shows how users should use it.
- Consider using [Breeze environment](https://github.com/apache/airflow/blob/main/BREEZE.rst) 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](https://www.apache.org/foundation/policies/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]( https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#coding-style-and-best-practices).
Apache Airflow is a community-driven project and together we are making it better 🚀.
In case of doubts contact the developers at:
Mailing List: dev@airflow.apache.org
Slack: https://s.apache.org/airflow-slack
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] gustavom2998 commented on pull request #25982: bugfix: tweak support for project_id argument in BigQueryGetDataOperator
Posted by GitBox <gi...@apache.org>.
gustavom2998 commented on PR #25982:
URL: https://github.com/apache/airflow/pull/25982#issuecomment-1254502244
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] gustavom2998 commented on pull request #25982: bugfix: tweak support for project_id argument in BigQueryGetDataOperator
Posted by GitBox <gi...@apache.org>.
gustavom2998 commented on PR #25982:
URL: https://github.com/apache/airflow/pull/25982#issuecomment-1248798274
@potiuk any chance you could take a look now! Took a while but I think we're almost there :D
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #25982: bugfix: tweak support for project_id argument in BigQueryGetDataOperator
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #25982:
URL: https://github.com/apache/airflow/pull/25982#issuecomment-1306403893
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #25982: bugfix: tweak support for project_id argument in BigQueryGetDataOperator
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #25982:
URL: https://github.com/apache/airflow/pull/25982#issuecomment-1228942993
Can you please fix static check and add/update a unit test to avoid regressions in the future ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #25982: bugfix: tweak support for project_id argument in BigQueryGetDataOperator
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #25982:
URL: https://github.com/apache/airflow/pull/25982#issuecomment-1250654380
Looks good, but you need to fix statich checks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #25982: bugfix: tweak support for project_id argument in BigQueryGetDataOperator
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #25982:
URL: https://github.com/apache/airflow/pull/25982#issuecomment-1255090962
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] gustavom2998 commented on pull request #25982: bugfix: tweak support for project_id argument in BigQueryGetDataOperator
Posted by GitBox <gi...@apache.org>.
gustavom2998 commented on PR #25982:
URL: https://github.com/apache/airflow/pull/25982#issuecomment-1229015796
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #25982: bugfix: tweak support for project_id argument in BigQueryGetDataOperator
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #25982:
URL: https://github.com/apache/airflow/pull/25982#issuecomment-1236730418
Any updates @gustavom2998 :)?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] closed pull request #25982: bugfix: tweak support for project_id argument in BigQueryGetDataOperator
Posted by GitBox <gi...@apache.org>.
github-actions[bot] closed pull request #25982: bugfix: tweak support for project_id argument in BigQueryGetDataOperator
URL: https://github.com/apache/airflow/pull/25982
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #25982: bugfix: tweak support for project_id argument in BigQueryGetDataOperator
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #25982:
URL: https://github.com/apache/airflow/pull/25982#issuecomment-1255941168
> 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-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.
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 change (which is 100 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 .
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org