feat(maven): Implicit grouping by versions defined via properties #4028

zharinov · 2019-07-06T14:01:18Z

Closes #4024, Closes #4031

Add a groupName to extracted deps whenever a placeholder is used. This provides implicit grouping.

zharinov · 2019-07-06T14:04:43Z

Minimal example:
https://github.com/zharinov/java-test/pulls

rarkins

Need to review the design approach and hopefully not change the datasource signature.

rarkins · 2019-07-07T07:14:02Z

lib/config/definitions.js

@@ -1650,6 +1650,7 @@ const options = [
    default: {
      fileMatch: ['\\.pom\\.xml$', '(^|/)pom\\.xml$'],
      versionScheme: 'maven',
+      groupByProps: true,


If users are to be able to configure this, we need to add it to definitions too

rarkins · 2019-07-07T07:16:21Z

lib/datasource/maven/index.js

@@ -11,8 +11,54 @@ module.exports = {
  getPkgReleases,
 };

+const cacheNamespace = 'datasource-maven';
+
+async function getPkgReleases({ lookupName, registryUrls, groupName, deps }) {


Can you explain more about your design choice for groupName? I hadn't expected we need to pass groupName to the datasource. I would prefer if we could handle it using depName/lookupName and not add parameters like grouName/deps that are not shared with other datasources.

I needed some key to handle grouping, as well as other dependencies in this group. Setting the groupName in extracting phase led to one PR per variable, instead of many PRs for each dependency. I found I can use this fields in datasource.

Actually I would like to implement the better design. At the extracting phase we can use extractAllPackageFiles and handle cross-file props. It would be better to handle common props in updateDependency(content, upgrade), but I'm not sure I can use context there.

lib/datasource/maven/index.js

rarkins · 2019-07-07T07:50:52Z

How about this?

Refactor existing code (ignoring this feature for now) to use extractAllPackageFiles for greater efficiency. Raise as separate PR
If datasource needs caching added, do that in another PR

Then let's work out the best way to implement this. For example:

If there are 5 deps using the same placeholder, should we represent that as one "update" internally or 5? If it's one, then which of the deps to use as lookupName?
What to do if not all of those 5 deps have the same update version available?

If we use the highest available, then that will likely break the build in the PR. But if we use the lowest available, maybe the user doesn't know that a critical hotfix is available for one of the deps. Frankly they shouldn't be using a shared placeholder unless the deps are guaranteed to all have the same latest version, so the question is about what we should do when encountering this type of invalid configuration. I prefer the first approach because it increases the chance that they realise their mistake. But for simplistic reasons I don't mind if we use a "random" approach either, e.g. always using the first-defined dep as the lookupName.

zharinov · 2019-07-07T08:14:12Z

It's possible to drop all the deps except first in extractAllPackageFiles, but "highest" and "lowest" approaches require some additional rework. And I agree: there are pros and cons for each of them. So, leaving just one dep extracted for each of common variable sounds good for you? If so, I'll turn this option off by default, right?

zharinov · 2019-07-07T08:18:46Z

Just thought, its possible to make decision about which version to replace in updateDependency but it seems we don't have access to config options there.

rarkins · 2019-07-07T09:21:11Z

UpdateDependency gets called once per “update” - there is no “updateAllDependencies” right now.

Seems like a good idea if extractAllDependencies can filter it down to just one “update” per placeholder. Lets use a deterministic way to determine the “first” dependency to use as “lookupName” and put placeholder as “depName”. It needs to be deterministic (same inputs always results in the same first package) so we don’t “flip flop” between runs.

rarkins · 2019-07-07T09:44:25Z

In the ideal scenario we’d warn if not all the packages have the same update available, but for now I think let’s go with the simplest approach (de-duplicating at the extract stage). And let’s make this placeholder approach non-configurable.

rarkins · 2019-07-10T07:35:52Z

@zharinov what's the current status of this? e.g. ready for review/merge?

rarkins · 2019-07-10T07:37:09Z

Also, once it's ready for review, can you update the description of this PR to describe what changes to logic it's making?

zharinov · 2019-07-10T08:04:42Z

@rarkins Yes, I think it's ready

rarkins · 2019-07-10T08:15:02Z

Has the example repo pull requests been updated to match latest ?

zharinov · 2019-07-10T08:15:36Z

Yep, just double-checked it

zharinov · 2019-07-12T09:12:56Z

test/manager/maven/__snapshots__/index.spec.js.snap

+        "datasource": "maven",
+        "depName": "org.example:quux",
+        "fileReplacePosition": 698,
+        "groupName": "quuxVersion",


zharinov · 2019-07-12T09:13:04Z

test/manager/maven/__snapshots__/index.spec.js.snap

+        "datasource": "maven",
+        "depName": "org.example:quux-test",
+        "fileReplacePosition": 698,
+        "groupName": "quuxVersion",


zharinov · 2019-07-12T09:16:13Z

In example, it opened extra PR for security update, but I think that's not the bug, but feature.

rarkins · 2019-07-12T09:27:23Z

@zharinov yes we deliberately "break out" of any grouping if there's a security warning. Can we test against a non-security update?

zharinov · 2019-07-12T09:37:38Z

@rarkins Yes, take a look at https://github.com/zharinov/java-test/pull/14

renovate-bot · 2019-07-13T06:06:17Z

🎉 This PR is included in version 18.27.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

rarkins requested changes Jul 7, 2019

View reviewed changes

zharinov force-pushed the feat/group-by-maven-props branch 4 times, most recently from 492b5ce to b46b861 Compare July 8, 2019 07:37

feat(maven): Group dependencies by common version property

b1f694e

zharinov force-pushed the feat/group-by-maven-props branch from 2854a7d to b1f694e Compare July 12, 2019 09:11

zharinov commented Jul 12, 2019

View reviewed changes

Merge branch 'master' into feat/group-by-maven-props

d2f3b89

rarkins merged commit 9a3c9ca into renovatebot:master Jul 13, 2019

renovate-bot added the released label Jul 13, 2019

github-actions bot locked as resolved and limited conversation to collaborators Dec 16, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(maven): Implicit grouping by versions defined via properties #4028

feat(maven): Implicit grouping by versions defined via properties #4028

zharinov commented Jul 6, 2019 •

edited by rarkins

zharinov commented Jul 6, 2019

rarkins left a comment

rarkins Jul 7, 2019

rarkins Jul 7, 2019

zharinov Jul 7, 2019

rarkins commented Jul 7, 2019

zharinov commented Jul 7, 2019

zharinov commented Jul 7, 2019

rarkins commented Jul 7, 2019

rarkins commented Jul 7, 2019

rarkins commented Jul 10, 2019

rarkins commented Jul 10, 2019

zharinov commented Jul 10, 2019

rarkins commented Jul 10, 2019

zharinov commented Jul 10, 2019

zharinov Jul 12, 2019

zharinov Jul 12, 2019

zharinov commented Jul 12, 2019

rarkins commented Jul 12, 2019

zharinov commented Jul 12, 2019

renovate-bot commented Jul 13, 2019

feat(maven): Implicit grouping by versions defined via properties #4028

feat(maven): Implicit grouping by versions defined via properties #4028

Conversation

zharinov commented Jul 6, 2019 • edited by rarkins

zharinov commented Jul 6, 2019

rarkins left a comment

Choose a reason for hiding this comment

rarkins Jul 7, 2019

Choose a reason for hiding this comment

rarkins Jul 7, 2019

Choose a reason for hiding this comment

zharinov Jul 7, 2019

Choose a reason for hiding this comment

rarkins commented Jul 7, 2019

zharinov commented Jul 7, 2019

zharinov commented Jul 7, 2019

rarkins commented Jul 7, 2019

rarkins commented Jul 7, 2019

rarkins commented Jul 10, 2019

rarkins commented Jul 10, 2019

zharinov commented Jul 10, 2019

rarkins commented Jul 10, 2019

zharinov commented Jul 10, 2019

zharinov Jul 12, 2019

Choose a reason for hiding this comment

zharinov Jul 12, 2019

Choose a reason for hiding this comment

zharinov commented Jul 12, 2019

rarkins commented Jul 12, 2019

zharinov commented Jul 12, 2019

renovate-bot commented Jul 13, 2019

zharinov commented Jul 6, 2019 •

edited by rarkins