-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Add an automatic GCP-BOM dependency upgrader #30262
Conversation
scripts/tools/gcpbomupgrader.py
Outdated
*('-q dependencies --configuration implementation --console=plain' | ||
.split()) | ||
], | ||
cwd=self.BUILD_DIR, |
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.
nits: the format looks strange here
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.
That was the result that I have run yapf on the script.
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.
That's a great tool to facilitate libraries-bom version upgrade. Thanks Yi!
scripts/tools/gcpbomupgrader.py
Outdated
There are few reasons we need to declare the version numbers: | ||
1. Sync the dependency that not included in GCP-BOM with those included with BOM | ||
For example, "com.google.cloud:google-cloud-spanner" does while "com.google.cloud:google-cloud-spanner:():test" doesn't | ||
2. There are Beam artifacts not depending on GCP-BOM but used dependency managed |
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.
Could you also document what we should change BeamModulePlugin if we have a new dep that we want to pin and we want to leverage this tool later (or at least don't break the standardization)?
There are two cases here:
- adding a new dep from a project that is already tracked in BeamModulePlugin, and
- adding a new dep from a new project.
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.
Good point. Definitely will clean up this script when marked as ready for review. Currently it just serves as an "unoffical" tool.
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.
Added comments about the tags that the script relies on: https://github.com/apache/beam/pull/30262/files#diff-0435a83a413ec063bf7e682cadcd56776cd18fc878f197cc99a65fc231ef2047R595
bdc2f06
to
bd3b12b
Compare
// Try to keep grpc_version consistent with gRPC version in google_cloud_platform_libraries_bom | ||
def grpc_version = "1.60.0" | ||
// [bomupgrader] determined by: io.grpc:grpc-netty, consistent with: google_cloud_platform_libraries_bom | ||
def grpc_version = "1.61.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.
we again have the grpc version mismatch after #30181, my bad, fixed by this script
R: @shunping ready for review now |
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control |
The code looks good. I am also wondering if we can use your script to add a presubmit test, so if the version is not right, the update cannot be submitted. This may need to change your script to support two modes: in-place updating (which is already implemented) and reporting only. Then a new test can be added to call this script in reporting mode and check if there is any version mismatch. WDYT? |
This sounds good, similar to .github/workflows/update_python_dependencies.yml . We can setup a "test" to generate a PR / or fail like the referred workflow. However in practice we already have many tests and there are infra related workflow no one cares and red for months. So the actual benefit I am not sure. As of the scope of this PR I am not intend to setup a test. |
LGTM! |
Command:
python scripts/tools/gcpbomupgrader.py 26.30.0
If run
python scripts/tools/gcpbomupgrader.py 26.31.0
based on this PR, it would give:Please add a meaningful description for your change here
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
addresses #123
), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, commentfixes #<ISSUE NUMBER>
instead.CHANGES.md
with noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.
To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI or the workflows README to see a list of phrases to trigger workflows.