-
Notifications
You must be signed in to change notification settings - Fork 24
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
feat: add dependency versions script #741
Conversation
Adding yaml for versions script.
Compares the latest version of google-cloud-shared-config to the parent of an added dependency
Permission denied error when running without sudo
Checking for functionality of versions script.
check
yaml
… to v0.118.1-alpha
… specific client libraries and the other checks all client libraries
@chingor13 Completed updates on scripts per our conversation on Monday. Interested to hear your thoughts! |
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
@salehsquared -- please sigh cla since you are external contributor now. Thanks! |
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
@googlebot I signed it! |
dependency-convergence/src/test/java/RecentCommitCheckTest.java
Outdated
Show resolved
Hide resolved
pushd "${scriptDir}/../dependency-convergence" | ||
mvn clean install -DskipTests | ||
# Single quotes around double quotes so that the branch name is counted as only one argument | ||
mvn exec:java -Dexec.args="'${runRelease}'" |
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 would suggest we break these into separate checks (rather than re-using the same check). We can run these from GitHub actions and skip if the commit does not match what we expect.
.generateArtifactData(latestSharedDependencies); | ||
String latestSharedDependenciesVersion = sharedDependenciesData.getLatestVersion(); | ||
if (latestSharedDependenciesVersion == null || latestSharedDependenciesVersion.isEmpty()) { | ||
return 2; |
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.
These magic integers should either be enums or perhaps thrown exceptions
return 1; | ||
} | ||
// A release PR was found | ||
Arguments arguments = Arguments.readCommandLine("-f ../pom.xml"); |
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 is very odd usage of command line parsing. Instead, you should be able to instantiate the Bom
instance yourself. It's also odd to hard code this value.
System.out.println("Total dependencies checked: " + clientLibraries.size()); | ||
} | ||
|
||
@VisibleForTesting |
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 is marked visible for testing but is not tested directly.
* google-cloud-shared-dependencies was not in the POM file, null if the POM file could not be | ||
* found | ||
*/ | ||
private static String getSharedDependenciesVersion(String pomUrl) { |
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.
Much of this code seems to share functionality with the dashboard, should we combine the artifacts and share logic?
} | ||
for (Dependency dependency : model.getDependencyManagement() | ||
.getDependencies()) { | ||
if ("com.google.cloud".equals(dependency.getGroupId()) && "google-cloud-shared-dependencies" |
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'd be nice if we tested this parsing.
Closing as it is addressed in #1752 |
Related to #735
Adds in two scripts to be used as presubmit checks:
(1) deps-finder.sh
If added as a presubmit check, this will first check if the associated PR was in the format of "deps: update...". If this is not the case, the script will return success and end. If a google-cloud dependency that is meant to use java-shared-dependencies was updated, then the script will check if the updated dependency has the most recent version of java-shared-dependencies. If it does not, the script will fail. In all other cases (e.g. something that is not meant to use java-shared-dependencies), the script will pass.
(2) release-versions.sh
If added as a presubmit check, this will check if the PR was made on a non-SNAPSHOT release branch. If it was not, the script will return success and end. Otherwise, it will check our dependencyManagement list for any client libraries that utilize java-shared-dependencies.
If at least one client library is meant to use java-shared-dependencies and does not have the latest version of java-shared-dependencies, the script will fail. Currently there are many client libraries in this category, and they are all summarized by the script, including which version of shared-dependencies they use.
Summary of the current state of the client libraries (master branch, checked 07/09):
Number of incorrect dependencies found: 50. Number of correct dependencies: 11.
Edit (07/10): java-storage-nio is no longer incorrectly tagged, and is thus no longer blocking the script.