-
Notifications
You must be signed in to change notification settings - Fork 267
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
Fixes #426 restore default behavior on setting versions in all modules of the local aggregation root that was present before the fix for #82 #427
Fixes #426 restore default behavior on setting versions in all modules of the local aggregation root that was present before the fix for #82 #427
Conversation
…s of the local aggregation root that was present before the fix for #82 introduce a new flag processFromLocalAggregationRoot that enables the new (and probably more correct) behavior by setting it it false ensure old and new behavior are covered by integration tests
Any news on this? As we're starting to approach the anniversary of this PR :-) |
Hey @robincsmith thanks for your PR! I will review it soon. Sorry for the delay! ;-) |
Did a quick check with such a layout:
Contents of <parent>
<groupId>invalid.company.project</groupId>
<artifactId>project-parent</artifactId>
<version>2.9.0-SNAPSHOT</version>
<relativePath>project-parent/pom.xml</relativePath>
</parent> Executed:
|
Indeed. That matches the behaviour for version 2.7 and earlier:
The Maven invocation needs to include the link to the parent POM (as it did for versions <2.7) - in your case,
|
But that will not change any other pom files. E.g. the |
Ah, in that case (from having a quick look at the RP), I think you may need to set I'm not sure, as it's @stefanseifert 's PR. I'll try building their code. |
the start of this story was bug #82, for which i provided the PR #400 which was merged and released. this PR restored the previous behavior to work exactly as it was for years in the previous versions of the plugin - but still allows to switch on the new behavior as described in #82. the new behavior only gets active when plugin property ITs are included to cover both old and new behavior. |
@stefanseifert agreed - thanks for confirming. @bmarwell I've just built the PR code, and the following should work for you, running in the project root directory:
(i.e. no need to set any other flags, such as |
The only thing I can think is that you might have another snapshot version of the plugin installed? As the behaviour looks unchanged for you. My plugin installation steps were just to checkout the PR code and run |
Did the same, and as I use -X I can see the new property. Hmmm. But if I use the file parameter and point to the parent not containing any modules, how would Maven know about the root? 🤷🏻♂️ I have another specialty in my project. |
As soon as I started repeating the versions in all other modules again, it worked again. |
See mojohaus/versions#427, the parent POM in a sub-dir causes issues with 2.8.1.
Running into this in ModiTect as well; as a work-around, we've downgraded that invocation to 2.7 for the time being: https://github.com/moditect/moditect/blob/master/.github/workflows/trigger.yml#L87. |
Well, we could just wait for maven 4 (where versions are not needed to be specified if the parent is local), or just use the workaround: |
if there are no objections i will merge this PR next week as lot of people seem to depend on the old behavior. |
merged to master with 279d90a |
introduce a new flag processFromLocalAggregationRoot that enables the new (and probably more correct) behavior by setting it it false
ensure old and new behavior are covered by integration tests
Fixes #426