-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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(gradle): bump strategy #33453
feat(gradle): bump strategy #33453
Conversation
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.
Thanks for the contribution. Looks promising but can benefit from some refinement:
- Please add a reproduction repo so that your changes can be tested against, incl a dep with regular maven based range and one with a range + strict version enforcement.
- Run
prettier
and undo changes unrelated to the bump strategy. - Keep dep extraction to the minimum, it's not the place to decide what part to bump
Most importantly: To reduce the complexity of your changes, it would be great if you could limit this PR to the already supported Maven range syntax, for now ignoring gradle's strict version notation. This should allow you to reuse the existing maven range parsing, it should work without changes to the parser and, unless I missed something, only require a few adaptations in the gradle versioning
Thank you for the quick and precise feedback.
The primary driver for this PR was the need for Gradle's strict version notation at my company. It allows us to specify: yes, you can use a newer dependency if you want, but if there are options to choose from, prefer the one specified with the '!!' Unfortunately, this functionality isn’t achievable without adding Gradle’s syntax. Would your suggested path forward be to first implement Maven-only ranges (using the existing Maven module, which seems feasible), and then add Gradle syntax in a follow-up? If so, is there a way to minimize complexity and avoid lengthy delays? I work part-time (twice a week) and want to ensure this progresses smoothly without causing blockers. |
Yes, the suggestion to do it in two iterations was exactly aimed at more efficient reviews: in a first PR focus on already parsed Maven ranges and add the bump strategy + composition of a new satisfying version to the gradle versioning part. In a second PR add |
752f5ec
to
51b4091
Compare
I've adjusted this merge-request to match the new scope.
Is there a guide on how to do that or a pull request that had the same requirement where I could take a look at? |
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.
Regarding a minimal reproduction, take a look here -> https://github.com/renovatebot/renovate/blob/main/docs/development/minimal-reproductions.md
Please also rebase your changes on the current main branch.
I did the changes you've suggest, the repro will have to wait until my next workday this week. Thanks for the link! |
# Conflicts: # lib/modules/manager/gradle/utils.spec.ts
You may also want to add
|
I am trying to get the repro running right now. |
Your build.gradle.kts uses Unix line endings, so |
The bump strategy is working in my reproduction repo - thanks for the quick assistance! Should the documentation be updated to reflect this change somewhere? |
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.
Thanks for the updates. The documentation for gradle-versioning will be auto-updated and include supportedRangeStrategies: RangeStrategy[] = ['pin', 'bump'];
# Conflicts: # lib/modules/versioning/gradle/index.spec.ts
needs prettier run |
Head branch was pushed to by a user without write access
@viceice prettier should be happy now |
Does the failed Build / test-success (pull_request) check mean that some tests are failing? |
Test failures appear to have been a temporary problem |
🎉 This PR is included in version 39.177.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Changes
Todos
Context
Documentation (please check one with an [x])
How I've tested my work (please select one)
I have verified these changes via: