-
Notifications
You must be signed in to change notification settings - Fork 14.1k
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
Standardise dataproc location param to region #16034
Standardise dataproc location param to region #16034
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/master/CONTRIBUTING.rst)
|
It looks like a breaking change. Can we avoid it and keep backward compatibility with warning? This change also requires an entry in changelog to facilitate migration. See: https://github.com/apache/airflow/blob/master/airflow/providers/google/CHANGELOG.rst |
Thank you so much for doing this change that I haven't had time to do! +1 to @mik-laj 's comment about it being a breaking change. |
454a2b3
to
0349a97
Compare
@mik-laj Would this now be alright? Please let me know |
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. |
I removed Changelog entry. I am going to describe it soon but changelog entries (unless we introduce bigger description for breaking changes) should be managed by release manager (semi-automatically), not the author. |
@Daniel-Han-Yang can you fix the Pylint test? |
and rebase to latest |
@leahecole should we make similar change in other operators? AFAIK we assumed to use location everywhere to make the interface consistent. |
69b4acf
to
fe3a2c5
Compare
Good question. I would hope it would be the same, but am not 100% sure. I'll double check other operators and comment once I've looked. |
I just reviewed a bunch of the other GCP operators and it seems like most of the APIs use |
Standardises DataProc hook & operators `location` parameter to `region` in line with underlying google DataProc Python client library.
fe3a2c5
to
c88f173
Compare
Awesome work, congrats on your first merged pull request! |
Standardises DataProc hook & operators
location
parameter toregion
in linewith underlying google DataProc Python client library.
closes: #15622
^ 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 UPDATING.md.