Add support for versions using git revision suffixes for Maven and Gradle#13998
Add support for versions using git revision suffixes for Maven and Gradle#13998yeikel wants to merge 1 commit intodependabot:mainfrom
Conversation
e36d11b to
4433784
Compare
00bb36a to
5107728
Compare
maven/spec/dependabot/maven/shared/shared_version_finder_spec.rb
Outdated
Show resolved
Hide resolved
| let(:dependency_version) { "5933.vcf06f7b_5d1a_2" } | ||
| let(:comparison_version) { "5857.vb_f3dd0731f44" } |
There was a problem hiding this comment.
Would suggest a test checking explicitly the case most recently observed to fail, that https://github.com/jenkinsci/plugin-pom/releases/tag/6.2108.v08c2b_01b_cf4d is not considered superseded by https://github.com/jenkinsci/plugin-pom/releases/tag/6.2122.v70b_7b_f659d72 which I suppose was a regression from #13818. None of the examples here seem to include a prefix prior to the revision count, as used for example in https://github.com/jenkinsci/plugin-pom/blob/623345749f5978fee57ab0e670f1f980dd5b05cd/pom.xml#L57 or even https://github.com/jenkinsci/jjwt-api-plugin/blob/b2c204d6cd36e8597dddade93d7cc95f12074943/pom.xml#L16
There was a problem hiding this comment.
Thank you for the feedback @jglick. I updated the tests cases to consider that format as well as others I could find. Could you please review and let me know if I am missing any scenario?
Thanks in advance for your time and understanding
410ea63 to
cab9a6a
Compare
| end | ||
|
|
||
| context "when the version has a short git commit" do | ||
| let(:dependency_version) { "5622.c9c3051" } |
There was a problem hiding this comment.
FWIW this would never apply to Jenkins CD, which always uses a 12-digit hash after v.
There was a problem hiding this comment.
Thanks, I tried to cover a mix of Jenkins use cases plus general usage for other libraries
| end | ||
|
|
||
| context "when the version has a single embedded git commit using different delimiters" do | ||
| let(:dependency_version) { "5622-c9c3051619f5" } |
There was a problem hiding this comment.
also never used by the Jenkins project, FWIW
There was a problem hiding this comment.
Same reasoning as above, just more coverage of potential examples
| let(:dependency_version) { "5723.v6f9c6b_d1218a_" } | ||
| let(:comparison_version) { "5622.c9c3051619f5" } |
There was a problem hiding this comment.
I am not sure what you mean with this
There was a problem hiding this comment.
Sorry, what is the confusion/question here? What this test is asserting is that the two versions are "Semantically similar" to be compared. Later on during sorting is when it is decided if they are newer or not
|
|
||
| context "when version has pre-release qualifier with git SHA" do | ||
| # Format: {number}.v{git-sha}-{qualifier} | ||
| let(:dependency_version) { "252.v356d312df76f-beta" } |
| context "when one version has git SHA and other is standard semver" do | ||
| let(:dependency_version) { "1.2.3" } | ||
| let(:comparison_version) { "1.2.4.va1b2c3d" } | ||
|
|
||
| it { is_expected.to be false } | ||
| end |
There was a problem hiding this comment.
Same reason as above. Not considered semantically the same line of releases
There was a problem hiding this comment.
This particular example is unlikely to ever occur in the Jenkins ecosystem but Maven does consider 1.2.4 to be newer than 1.2.3.
There was a problem hiding this comment.
Just to clarify, this class/logic not about deciding if something is newer or not, it is about deciding if two versions should be "compared" to begin with
There was a problem hiding this comment.
Got it, but as before, if a dep is currently written on version 1.2.3, and some 1.2.4* version is available, it should be offered.
There was a problem hiding this comment.
It’s a nuanced topic.
One could argue that Dependabot should not make this decision automatically and that the choice should remain with users. That is, if something is newer and sorts within Maven, just suggest it no matter what
Others believe Dependabot should make that call (a direction I lean toward), which puts us in a somewhat uncomfortable position.
At Dependabot's current configuration capabilities it is one or the other. We cannot have both
Ideally, this behavior would be configurable to satisfy both perspectives, but we are not there yet.
Consider the following examples:
1.2.3 → 1.2.4
1.2.3 → 1.2.4-<hash>
1.2.3 → 1.2.4-<anything>-whatever-I-want
1.2.3-jdk
1.2.5-jre
Which should Dependabot offer? Whatever is newer and sorts?
Perhaps, but it’s complicated, because library authors sometimes use versioning in inconsistent or unpredictable ways. While others are more strict about their intent. The later is what I often see and what Dependabot is trying to work with
There was a problem hiding this comment.
That said, I hope that the changes cover Jenkins use case. Curious to hear if I should test any other scenario.
Thanks for the feedback and I hope it makes sense. It is a great discussion!
maven/spec/dependabot/maven/shared/shared_version_finder_spec.rb
Outdated
Show resolved
Hide resolved
49ff117 to
2277801
Compare
671c58b to
1b1268d
Compare
4851f69 to
999a68e
Compare
999a68e to
3080b80
Compare
|
@jglick @timja Thanks for your feedback. I believe I’ve updated the changes to cover all the scenarios you mentioned. The main update in the recent commits is that the digest is now treated as irrelevant, allowing updates to proceed even if only one artifact includes a digest. @kbukum1 Given the impact (see #14102, affecting many repositories in the Jenkins organization), I would appreciate your review to help determine if we can move forward with releasing this change. |
Thanks @yeikel . I will check it hopefully today otherwise next week after Tuesday. |
10424f4 to
f4d7925
Compare
Hey @kbukum1 just pinging to see if you got a chance to review this? Thanks |
Hi @yeikel , sorry I have some workload and I really couldn't review that. I am hoping by mid of next week I will do review. |
f4d7925 to
d76c8b5
Compare
What are you trying to accomplish?
Adds support for detecting versions that include embedded Git commits in both short and long format.
The updated logic correctly parses version strings with Git revision suffixes, as used by some Maven projects (e.g., the Jenkins plugins organization). Examples:
2.9.2-29.v717aac953ff3,2.9.3-29.v717aac953ff3This is a follow-up for #13818 #13999
Fixes #14102
How will you know you've accomplished your goal?
Both the existing and new specs pass. I was able to reproduce the issue with the tests, and the new code resolves it
Note to reviewers
Checklist