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.

Sign up for GitHub

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

Jump to bottom

fix(maven): Check each release for corresponding jar #5614

Merged
merged 6 commits into from
Apr 30, 2020
Merged

fix(maven): Check each release for corresponding jar #5614

merged 6 commits into from
Apr 30, 2020

Conversation

zharinov
Copy link
Collaborator

@zharinov zharinov commented Mar 1, 2020

Rel #5538
Closes #4471

@zharinov zharinov changed the title fix(maven): Check each release for corresponding jar existence fix(maven): Check each release for corresponding jar existence [WIP] Mar 1, 2020
@zharinov zharinov changed the title fix(maven): Check each release for corresponding jar existence [WIP] fix(maven): Check each release for corresponding jar [WIP] Mar 1, 2020
@zharinov
Copy link
Collaborator Author

zharinov commented Mar 1, 2020

Do we still need cache entire results by lookupName key?

@zharinov
Copy link
Collaborator Author

zharinov commented Mar 3, 2020

Still leaves room for bugs if directory with jar exists, but metadata.xml doesn't list this version.

@rarkins
Copy link
Collaborator

rarkins commented Mar 3, 2020

As per #5538 (comment) there is a need for two levels of caching:

  1. Similar as done to day, cache the end result based on lookupName / url for a limited time (after which the cache expires/disappears)
  2. New: cache the list of JAR files and exists/not-exists results, also based on lookupName / url but storing the result for much longer (hours, perhaps a day)

The goal is: if the list of releases in the XML doesn't change, then we shouldn't query the JAR files again unless a long time has elapsed. We don't want to be requerying every JAR file once every 10 minutes, for example.

@zharinov
Copy link
Collaborator Author

zharinov commented Mar 3, 2020

What logger messages to add?

@zharinov
Copy link
Collaborator Author

zharinov commented Mar 4, 2020

Test on real repo is okay

@zharinov zharinov changed the title fix(maven): Check each release for corresponding jar [WIP] fix(maven): Check each release for corresponding jar Mar 4, 2020
rarkins
rarkins reviewed Mar 4, 2020
View reviewed changes
Copy link
Collaborator

@rarkins rarkins left a comment

Choose a reason for hiding this comment

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

Do we have any public examples that I can test/debug against? e.g. can you give me pom.xml that will demonstrate this filtering behaviour?

lib/datasource/maven/index.spec.ts Outdated
'8.0.11',
'8.0.12',
// '8.0.11',
// '8.0.12',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we just delete these now?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Let's leave it to inform they're still in use later?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe, it would be nice to refactor this file

rarkins
rarkins reviewed Mar 4, 2020
View reviewed changes
Copy link
Collaborator

@rarkins rarkins left a comment

Choose a reason for hiding this comment

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

Do we have any public examples that I can test/debug against? e.g. can you give me pom.xml that will demonstrate this filtering behaviour?

rarkins
rarkins reviewed Mar 5, 2020
View reviewed changes
lib/datasource/maven/index.ts Show resolved Hide resolved
lib/datasource/maven/index.ts Outdated

if (!isValidArtifactsInfo(artifactsInfo, versions)) {
artifactsInfo = {};
for (const version of versions) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wondering if doing this in series is too slow

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point, already made it concurrent

@zharinov
Copy link
Collaborator Author

Do we have any public examples that I can test/debug against? e.g. can you give me pom.xml that will demonstrate this filtering behaviour?

I have this example from the original issue, which upgrades to 2.2.1 in master and to 2.2 with this branch (according to the repo structure).

@rarkins
Copy link
Collaborator

rarkins commented Apr 7, 2020

@zharinov needs refactoring to use util/http

@rarkins rarkins merged commit d54836b into renovatebot:master Apr 30, 2020
@rarkins rarkins deleted the feat/maven-artifact-checks branch April 30, 2020 10:15
@renovate-release
Copy link
Collaborator

🎉 This PR is included in version 19.225.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@Jnosh Jnosh mentioned this pull request May 4, 2020
Closed
@jgarec jgarec mentioned this pull request Sep 30, 2020
Closed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@rarkins rarkins rarkins left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Renovate updates gradle dependency version that does not exist
3 participants
@zharinov @rarkins @renovate-release