Skip to content

feat(gradle): bump strategy#33453

Merged
rarkins merged 13 commits intorenovatebot:mainfrom
ldt-fweichert:gradle-version-ranges
Feb 20, 2025
Merged

feat(gradle): bump strategy#33453
rarkins merged 13 commits intorenovatebot:mainfrom
ldt-fweichert:gradle-version-ranges

Conversation

@ldt-fweichert
Copy link
Copy Markdown
Contributor

@ldt-fweichert ldt-fweichert commented Jan 7, 2025

Changes

  • added support for the bump strategy for gradle versioning using the maven style range syntax
  • added support for maven style range syntax to the gradle manager
  • repro repo

Todos

  • Sign the CLA
  • Fix last failling test
  • Do code //TODO 's
  • Get feedback on the implementation
  • Add docs

Context

Documentation (please check one with an [x])

  • I have updated the documentation, or
  • No documentation update is required

How I've tested my work (please select one)

I have verified these changes via:

  • Code inspection only, or
  • Newly added/modified unit tests, or
  • No unit tests but ran on a real repository, or
  • Both unit tests + ran on a real repository

@CLAassistant
Copy link
Copy Markdown

CLAassistant commented Jan 7, 2025

CLA assistant check
All committers have signed the CLA.

@ldt-fweichert ldt-fweichert changed the title Gradle version ranges Gradle bump strategy Jan 7, 2025
@rarkins rarkins requested a review from Churro January 8, 2025 09:19
Copy link
Copy Markdown
Collaborator

@Churro Churro left a 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

Comment thread lib/modules/manager/gradle/utils.ts Outdated
Comment thread lib/modules/manager/gradle/parser.ts Outdated
Comment thread lib/modules/manager/gradle/utils.ts Outdated
Comment thread lib/modules/manager/gradle/utils.ts Outdated
Comment thread lib/modules/manager/gradle/utils.ts Outdated
Comment thread lib/modules/versioning/gradle/compare.ts Outdated
Comment thread lib/modules/versioning/gradle/index.ts Outdated
Comment thread lib/modules/versioning/gradle/index.ts Outdated
Comment thread lib/modules/manager/gradle/utils.ts Outdated
@ldt-fweichert
Copy link
Copy Markdown
Contributor Author

Thank you for the quick and precise feedback.

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

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.

@rarkins rarkins changed the title Gradle bump strategy feat(gradle): bump strategy Jan 14, 2025
@Churro
Copy link
Copy Markdown
Collaborator

Churro commented Jan 14, 2025

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 ! and to versionLikeSubstring and extend the gradle versioning part with support for strict version definitions.

@ldt-fweichert
Copy link
Copy Markdown
Contributor Author

I've adjusted this merge-request to match the new scope.

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.

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?

Copy link
Copy Markdown
Collaborator

@Churro Churro left a 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.

Comment thread lib/modules/manager/gradle/__snapshots__/parser.spec.ts.snap Outdated
Comment thread lib/modules/manager/gradle/utils.spec.ts Outdated
@ldt-fweichert
Copy link
Copy Markdown
Contributor Author

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
@Churro
Copy link
Copy Markdown
Collaborator

Churro commented Jan 22, 2025

You may also want to add bump here:

export const supportedRangeStrategies: RangeStrategy[] = ['pin'];

@ldt-fweichert
Copy link
Copy Markdown
Contributor Author

ldt-fweichert commented Jan 22, 2025

I am trying to get the repro running right now.
The repro repo is here
To verify that my test setup is working correctly i wanted to try the already implemented pin strategy on the main branch first. The weird thing is, that even that doesn't even seem to work. The version update fails due to a wrongly positioned fileReplacePosition:
grafik
Have i misconfigured something?
I am running renovate locally using pnpm debug ldt-fweichert/renotave-gradle-repro, nothing unusual there i guess.

@Churro
Copy link
Copy Markdown
Collaborator

Churro commented Jan 22, 2025

Have i misconfigured something? I am running renovate locally using pnpm debug ldt-fweichert/renotave-gradle-repro, nothing unusual there i guess.

Your build.gradle.kts uses Unix line endings, so fileReplacePosition 814 is correct for io.ktor:ktor-server-netty:3.0.0. See https://github.com/renovatebot/renovate/blob/main/docs/development/best-practices.md#windows on how to resolve your CRLF issue.

@ldt-fweichert
Copy link
Copy Markdown
Contributor Author

The bump strategy is working in my reproduction repo - thanks for the quick assistance! Should the documentation be updated to reflect this change somewhere?

@rarkins rarkins requested a review from Churro January 22, 2025 13:31
Copy link
Copy Markdown
Collaborator

@Churro Churro left a 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'];

rarkins
rarkins previously approved these changes Jan 28, 2025
@rarkins rarkins requested a review from viceice January 28, 2025 04:34
Comment thread lib/modules/manager/gradle/utils.ts
# Conflicts:
#	lib/modules/versioning/gradle/index.spec.ts
Comment thread lib/modules/manager/gradle/utils.ts Outdated
viceice
viceice previously approved these changes Feb 12, 2025
@viceice viceice enabled auto-merge February 12, 2025 12:29
@viceice
Copy link
Copy Markdown
Member

viceice commented Feb 12, 2025

needs prettier run

auto-merge was automatically disabled February 12, 2025 12:47

Head branch was pushed to by a user without write access

@ldt-fweichert
Copy link
Copy Markdown
Contributor Author

@viceice prettier should be happy now

@viceice viceice requested a review from rarkins February 12, 2025 20:14
@ldt-fweichert
Copy link
Copy Markdown
Contributor Author

Does the failed Build / test-success (pull_request) check mean that some tests are failing?

@rarkins rarkins added this pull request to the merge queue Feb 20, 2025
@rarkins
Copy link
Copy Markdown
Contributor

rarkins commented Feb 20, 2025

Test failures appear to have been a temporary problem

Merged via the queue into renovatebot:main with commit d022b83 Feb 20, 2025
@renovate-release
Copy link
Copy Markdown

🎉 This PR is included in version 39.177.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Mar 25, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants