-
Notifications
You must be signed in to change notification settings - Fork 53
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
Remove usage of Maven SCM API #404
Conversation
Co-authored-by: Olivier Lamy <olamy@apache.org>
.start(); | ||
try { | ||
int exitStatus = p.waitFor(); | ||
String output = new String(p.getInputStream().readAllBytes(), Charset.defaultCharset()).trim(); |
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.
Note: once my other PR is merged, I'll update this to use the gobbler introduced there
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.
Actually I did it in c20578d; some slight duplication with my other PR, but that will collapse based on which PR is merged first
if (StringUtils.startsWith(connectionURL, "scm:git:")) { | ||
gitUrl = StringUtils.substringAfter(connectionURL, "scm:git:"); |
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 https://github.com/jenkinsci/plugin-compat-tester/pull/399/files#r1069741815 you where trying to remove the dependency on commons-lang? can connectionUrl be null? (wont we just die horribly later)?
in which case
if (StringUtils.startsWith(connectionURL, "scm:git:")) { | |
gitUrl = StringUtils.substringAfter(connectionURL, "scm:git:"); | |
if (connectionURL.startsWith("scm:git:")) { | |
gitUrl = connectionUrl.substring("scm:git:".length()); |
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.
Not sure if it can be null or not. At the time of this writing the suggestion does not compile and is therefore a non-suggestion
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.
fixed the typo above.
note: needs local adaptions, I am working on updating. |
How's this looking this week James? |
the prerequisite for testing this was just merged, just updating the PR to check now. |
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.
tested, all ok.
Awesome, thank you! |
Problem
We currently rely on an old version of the Maven SCM API, which is not only inefficient but which also is a maintenance liability as can be seen in #393 (review).
Solution
Switch to running Git commands directly. Simpler and easier to maintain, and more efficient to boot.
Implementation
Extracted and cleaned up from #382 by @olamy (credited as
Co-authored-by
in the commit message), but without two portions of that PR:Testing done
Tested in context in
jenkinsci/bom
. I also manually corrupted the Git URL to make sure the error handling code path was exercised and worked as expected.