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 #837: Add includeParent (default true) to UpdatePropertiesMojo #838

Conversation

andrzejj0
Copy link
Contributor

@andrzejj0 andrzejj0 commented Dec 1, 2022

Upon request of a user in #793, adding includeParent (true by default) to UpdatePropertiesMojo.

Setting it to false will speed up execution to a great extend without sacrificing accuracy in most cases. However, if the POM redefines properties used in parent POMs, dependencies bound to those properties will not be discovered, so the plugin will not redefine those properties. Therefore the default value is true.

@andrzejj0 andrzejj0 changed the title Resolves #837: Add includeParent (default false) to UpdatePropertiesMojo Resolves #837: Add includeParent (default true) to UpdatePropertiesMojo Dec 1, 2022
@andrzejj0 andrzejj0 force-pushed the issue-837-update-property-includes-parent branch from a742a88 to 3689b13 Compare December 1, 2022 17:51
@andrzejj0
Copy link
Contributor Author

andrzejj0 commented Dec 1, 2022

@slawekjaranowski please review

Spotted a minor bug: change recorder would always output changes, even if they did not occur in the POM that was being processed. Fixed.

This resolves #837

@andrzejj0 andrzejj0 force-pushed the issue-837-update-property-includes-parent branch 2 times, most recently from 35372a8 to 67035ad Compare December 2, 2022 07:52
@andrzejj0
Copy link
Contributor Author

Added the same to UpdatePropertyMojo

@andrzejj0 andrzejj0 force-pushed the issue-837-update-property-includes-parent branch from 67035ad to b324f5c Compare December 2, 2022 08:16
@andrzejj0
Copy link
Contributor Author

And abstracted a common base for the two mojos.

@andrzejj0
Copy link
Contributor Author

Tried to resolve conflicts using GitHub 🤦

@andrzejj0 andrzejj0 force-pushed the issue-837-update-property-includes-parent branch from 5292c53 to 122d4f7 Compare December 2, 2022 18:03
@andrzejj0 andrzejj0 force-pushed the issue-837-update-property-includes-parent branch from 122d4f7 to c0a951d Compare December 2, 2022 18:11
@slawekjaranowski
Copy link
Member

We set such parameters to false in #817

Should be here default as true?

@andrzejj0
Copy link
Contributor Author

andrzejj0 commented Dec 2, 2022

Should be here default as true?

No. With false, it will break #582. Like I said in the first comment,

Setting it to false will speed up execution to a great extend without sacrificing accuracy in most cases. However, if the POM redefines properties used in parent POMs, dependencies bound to those properties will not be discovered, so the plugin will not redefine those properties. Therefore the default value is true.

So, it can only be treated as a speedup if one knows what they're doing. It will sometimes render the results wrong.

So, it must be true.

By the way, should I remove this?

getLog().debug( "Using Maven 2.0.10+ strategy to determine lifecycle defined plugins" );
        try
        {
            Method getLifecycles = LifecycleExecutor.class.getMethod( "getLifecycles" );
            List<Lifecycle> lifecycles = (List) getLifecycles.invoke( lifecycleExecutor, new Object[0] );
            // lookup the bindings for all the passed in phases
            return Arrays.stream( thePhases.split( "," ) )
                    .filter( StringUtils::isNotEmpty )
                    .map( phase -> getLifecycleForPhase( lifecycles, phase ) )
                    .filter( Objects::nonNull )
                    .collect( HashSet::new,
                            ( s, l ) -> s.addAll( getAllPlugins( project, l ) ),
                            Set::addAll );

@slawekjaranowski
Copy link
Member

Maven 2.x code should be remove, can be done in separate PR.
We use 3.2.5 as minimum.

@slawekjaranowski slawekjaranowski linked an issue Dec 3, 2022 that may be closed by this pull request
@slawekjaranowski slawekjaranowski added this to the 2.14.0 milestone Dec 3, 2022
@slawekjaranowski slawekjaranowski merged commit bc10d1a into mojohaus:master Dec 3, 2022
@andrzejj0
Copy link
Contributor Author

Maven 2.x code should be remove, can be done in separate PR.
We use 3.2.5 as minimum.

I'll do that in another PR then

@andrzejj0 andrzejj0 deleted the issue-837-update-property-includes-parent branch December 3, 2022 13:57
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.

UpdatePropertiesMojo processes parent POMs
2 participants