-
Notifications
You must be signed in to change notification settings - Fork 14.4k
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
Upgrade the dataproc package to 3.0.0 and migrate from v1beta2 to v1 api #18879
Conversation
8ee6791
to
ab252b4
Compare
setup.py
Outdated
@@ -302,7 +302,7 @@ def write_version(filename: str = os.path.join(*[my_dir, "airflow", "git_version | |||
'google-cloud-build>=3.0.0,<4.0.0', | |||
'google-cloud-container>=0.1.1,<2.0.0', | |||
'google-cloud-datacatalog>=3.0.0,<4.0.0', | |||
'google-cloud-dataproc>=2.2.0,<2.6.0', | |||
'google-cloud-dataproc>=3.0.0,<4.0.0', |
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.
It looks like a breaking chhange. Can you add a note about it in the changelog for the provider?
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.
@mik-laj 👍
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.
'google-cloud-dataproc>=3.0.0,<4.0.0', | |
'google-cloud-dataproc>=2.2.0,<4.0.0', |
We don't need to change the lower bound. New API is also available in 2.* series. In this way, we will avoid breaking changes.
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.
👍
a2cc3dc
to
ca916ec
Compare
|
||
Breaking changes | ||
~~~~~~~~~~~~~~~~ | ||
* ``Migrate Google Cloud Dataproc to the version 3.0.0 (#18879)`` |
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.
This summary should include a brief summary of what a breaking change is so that you do not need to read the full PR. It should also explain why we decided to introduce it to encourage migration. If there are other documents that describe this change, we should attach the link. See relates notes for v2.0.0 that have a similar case.
Not every library update is a breaking change, but it is here, so it needs some clarification.
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.
I don't know if we need this note at all if we can make this change more user-friendly. See: https://github.com/apache/airflow/pull/18879/files#r726122588
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.
@mik-laj I moved this change to the Feature
section in the CHANGELOG and I added better description. Now I think will be more clear for user what was changed.
ca916ec
to
b48d3cc
Compare
The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease. |
d7ab5f8
to
3ef2b98
Compare
3ef2b98
to
b7a0ef4
Compare
b7a0ef4
to
b0b0116
Compare
7.0.0 | ||
..... | ||
|
||
Features |
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.
In the long discussion above, we established that a note is not needed.
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.
@mik-laj I removed this change
@lwyszomi Let's wait until the CI is finished, I will check if we can merge this change, despite the fact that not all tests were successful. |
b0b0116
to
370bfff
Compare
Related -> #18485
^ 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.