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

Resolves #387: Provide an enforcer rule to specify the maximum number of allowed dependency updates #801

Merged
merged 1 commit into from
Nov 25, 2022

Conversation

andrzejj0
Copy link
Contributor

@andrzejj0 andrzejj0 commented Oct 27, 2022

Please review @slawekjaranowski

@andrzejj0 andrzejj0 changed the title Resolves #387: Provide an enforcer rule to specify a maximum number of allowed dependency updates Resolves #387: Provide an enforcer rule to specify the maximum number of allowed dependency updates Oct 27, 2022
@andrzejj0 andrzejj0 force-pushed the enforcer-rule branch 4 times, most recently from 65e3adf to 19498f9 Compare October 28, 2022 08:06
@andrzejj0 andrzejj0 marked this pull request as ready for review October 28, 2022 08:07
@andrzejj0
Copy link
Contributor Author

andrzejj0 commented Oct 28, 2022

One big difference is lack of segment controls like e.g. allowMinorUdates in the display dependency updates goal. I'm thinking of maybe adding such controls, i.e. ignoreMinorUpdates, ignoreIncrementalUpdates, and ignoreSubincrementalUpdates. What do you think?

-> Converting back to draft.

@andrzejj0 andrzejj0 marked this pull request as draft October 28, 2022 08:18
@andrzejj0 andrzejj0 marked this pull request as ready for review October 28, 2022 10:16
@andrzejj0
Copy link
Contributor Author

Implemented the three "ignore-" parameters. I think it's ready now.

I'll integrate the changes from #797 once it's merged in.

@andrzejj0
Copy link
Contributor Author

andrzejj0 commented Oct 28, 2022

Oh and regarding this:

.filter( dep -> finalDependencyManagement.parallelStream()
        .noneMatch( depMan -> dependenciesMatch( dep, depMan ) ) )                       

The above filters out dependency management from dependencies. The code replaces a lengthier method doing the same using a for iterator with a "terminator" variable. Why it was done in the first place I can only speculate about.

I think it's so that the dependency updates from dependencyManagemnt are not repeated in the depenedencies section.

If so, why isn't the same done wrt other sections -- that I don't know.

@andrzejj0
Copy link
Contributor Author

andrzejj0 commented Oct 28, 2022

Moved interpolateVersion to a static method inside MavenProjectUtils, since it is not using anything from VersionsHelper.

Copy link
Member

@slawekjaranowski slawekjaranowski left a comment

Choose a reason for hiding this comment

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

I think it will be to more complicated ... many configuration options will not be accepted like ruleSets ...

Maybe easier will be introduce new goal to existing plugin ...

pom.xml Outdated Show resolved Hide resolved
@slawekjaranowski
Copy link
Member

I still not convinced for such solution.

When user has configuration for versions-m-p like ruleSet and so on ...
When user want to use enforcer rule should copy paste configuration again to rule - right?

In such case maintain configuration in double place is difficult to maintenance.

@andrzejj0
Copy link
Contributor Author

andrzejj0 commented Oct 29, 2022

I still not convinced for such solution.

When user has configuration for versions-m-p like ruleSet and so on ... When user want to use enforcer rule should copy paste configuration again to rule - right?

In such case maintain configuration in double place is difficult to maintenance.

My problem with using display-dependency-updates to enforce a number of dependencies is that display-dependency-updates does not have "ignore...updates" options. "allow...updates" options have opposite semantics. It's like "include" vs "exclude". So, one cannot copy the options for one goal to another.

Display-dependency-updates allows ignoring the most major segemnts: *-2.3-1, *-*.3-1, *-*-*-1.

But here we need to be able to ignore the least major ones: 1.2.3-*, 1.2.*-*, 1.*.*-*.

That, plus the purpose of enforcing a number of updates is different.

A lot of the functionality is similar, to I abstracted that and moved it to that static utility class.

@andrzejj0 andrzejj0 force-pushed the enforcer-rule branch 4 times, most recently from a2f0d3a to 82426ce Compare November 3, 2022 18:52
@andrzejj0
Copy link
Contributor Author

I'll check later what went wrong there

@andrzejj0
Copy link
Contributor Author

andrzejj0 commented Nov 3, 2022

Soooo... I was a bit too optimistic with taking the enforcer-rule out to its own module. Too early for that. The rule needs to use some functionality from VersionsHelper, but the plugin also needs to package the rule, so it needs to depend on it. So, this is a circular dependency.

So, I'm putting it back until we extract the api to its own module.

Second stage of the module split:

  • enforcer-rules
  • versions-api
  • versions-test (TestUtils, MockUtils, etc. -- everything which is independent, but will be used by several different modules in tests)

The above is a discussion on its own, but right now, the api package is also not really mature -- it contains some API, but it also contains API implementation and utilities. Also, I think VersionsHelper and AbstractArtifactVersions seem to serve a very similar purpose.

@andrzejj0
Copy link
Contributor Author

@slawekjaranowski rebased with master. Please review.

@andrzejj0
Copy link
Contributor Author

@slawekjaranowski should I make any more changes to this PR (apart from resolving merge conflicts)?

@andrzejj0 andrzejj0 force-pushed the enforcer-rule branch 2 times, most recently from 56e12d6 to a64bb42 Compare November 24, 2022 17:17
@andrzejj0
Copy link
Contributor Author

So, I've waited with this quite some time as well, and it's also been increasingly more cumbersome to resolve all conflicts. Please review again and merge @slawekjaranowski

@andrzejj0
Copy link
Contributor Author

Changes applied, please re-review/merge.

@andrzejj0 andrzejj0 closed this Nov 25, 2022
@andrzejj0 andrzejj0 reopened this Nov 25, 2022
Copy link
Member

@slawekjaranowski slawekjaranowski left a comment

Choose a reason for hiding this comment

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

IT test for enforcer should be in versions-enforcer module

Now under test latest version of tested artifact is downloaded from remote repo
or used from previous build from local repository

@andrzejj0 andrzejj0 force-pushed the enforcer-rule branch 2 times, most recently from 4b7621f to 3d80c90 Compare November 25, 2022 08:00
@andrzejj0
Copy link
Contributor Author

@slawekjaranowski changes applied. One question remaining regarding the version of maven-enforcer-plugin in the docs.

versions-enforcer/pom.xml Show resolved Hide resolved
versions-enforcer/pom.xml Outdated Show resolved Hide resolved
@andrzejj0
Copy link
Contributor Author

@slawekjaranowski changes applied where possible (one was not possible -- yet)

@slawekjaranowski slawekjaranowski added this to the 2.14.0 milestone Nov 25, 2022
@slawekjaranowski slawekjaranowski linked an issue Nov 25, 2022 that may be closed by this pull request
@slawekjaranowski slawekjaranowski merged commit badd0c4 into mojohaus:master Nov 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Specifies if the build should be failed
2 participants