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(maven): Add maven relocation support #32550

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

jonasrutishauser
Copy link
Contributor

@jonasrutishauser jonasrutishauser commented Nov 14, 2024

Changes

Use maven relocation information to detect package name changes and allow replacement updates for maven.

Context

Closes #5667.

Documentation (please check one with an [x])

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

The issue #14149 should be updated after merging this.

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

@rarkins
Copy link
Collaborator

rarkins commented Nov 16, 2024

@jonasrutishauser thanks for this interesting feature. Could you share an example repo you've tested against?

@jonasrutishauser
Copy link
Contributor Author

@jonasrutishauser thanks for this interesting feature. Could you share an example repo you've tested against?

I created a simple example repo https://github.com/jonasrutishauser/renovate-32550 and I have run pnpm start jonasrutishauser/renovate-32550.

Copy link
Member

@viceice viceice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please split this PR into three parts

  • worker
  • datasource
  • manager

you can use this PR for one of them and the order probably doesn't matter.

@jonasrutishauser
Copy link
Contributor Author

As requested this now only contains the worker part.
The datasource part is in #32636 and the manager part is in #32635.

@astellingwerf
Copy link
Collaborator

@jonasrutishauser, do we understand why on your test repo, there's no PR for Ant 1.7.0? Is that just a matter of throttling, or is there a reason why the replacement does not get noticed?

@jonasrutishauser
Copy link
Contributor Author

@jonasrutishauser, do we understand why on your test repo, there's no PR for Ant 1.7.0? Is that just a matter of throttling, or is there a reason why the replacement does not get noticed?

I think the reason is, that the version 1.7.0 has no timestamp in the html and is therefore ignored/not seen. In the maven-metadata.xml this version is missing too.

Comment on lines +4960 to +4962
const { updates } = await Result.wrap(
lookup.lookupUpdates(config),
).unwrapOrThrow();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const { updates } = await Result.wrap(
lookup.lookupUpdates(config),
).unwrapOrThrow();
const { updates } = await lookup.lookupUpdates(config);

no need to wrap here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With your suggestion I receive Property 'updates' does not exist on type 'Result<UpdateResult, Error>'.ts(2339).

The only working code would be the following:

      const { updates } = (await lookup.lookupUpdates(config)).unwrapOrThrow();

As all other tests don't use this variant, I think it shouldn't be changed.

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.

Detect and follow Maven group and artifact moves
4 participants