Skip to content
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

Merged
merged 13 commits into from
Feb 20, 2025
Merged

Conversation

ldt-fweichert
Copy link
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

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
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

@ldt-fweichert
Copy link
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
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
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
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.

@ldt-fweichert
Copy link
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
Collaborator

Churro commented Jan 22, 2025

You may also want to add bump here:

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

@ldt-fweichert
Copy link
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
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
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
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
# Conflicts:
#	lib/modules/versioning/gradle/index.spec.ts
viceice
viceice previously approved these changes Feb 12, 2025
@viceice viceice enabled auto-merge February 12, 2025 12:29
@viceice
Copy link
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
Contributor Author

@viceice prettier should be happy now

@viceice viceice requested a review from rarkins February 12, 2025 20:14
@ldt-fweichert
Copy link
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
Collaborator

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
39 checks passed
@renovate-release
Copy link
Collaborator

🎉 This PR is included in version 39.177.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants