[MRELEASE-1042] releaseProfiles get overriden by activeProfiles #56

bguerin · 2020-05-18T21:39:30Z

Fix UnsupportedOperationException when altering releaseDescriptor.activateProfiles

rfscholte · 2020-05-19T06:42:57Z

...e-manager/src/main/java/org/apache/maven/shared/release/config/ReleaseDescriptorBuilder.java

@@ -58,7 +59,9 @@ public ReleaseDescriptorBuilder addCheckModificationExclude( String string )

    public ReleaseDescriptorBuilder setActivateProfiles( List<String> profiles )
    {
-        releaseDescriptor.setActivateProfiles( profiles );
+        List<String> copy = new ArrayList<>();
+        copy.addAll( profiles );


This now acts as a merge, not as a setter, so this can't be the right approach.

This is not a merge, I do not keep the old value. It is a defensive setter, ensuring that the list implementation used is not read only / immuable

This change is the minimal one is we want to keep your remarks on my previous PR

we will test ASAP

@bguerin I confirm that the fix works.
I would also suggest to change the test case here and use the same approach at few lines above.
In short replace:
secondBuilder.setActivateProfiles( new ArrayList( Arrays.asList("aProfile", "bProfile") ) );
with
secondBuilder.setActivateProfiles( Arrays.asList( Arrays.asList("aProfile", "bProfile") ) );

I confirm that the fix works.

YOUPI !!!

change the test case

This is already part of this PR ;)

I had the same question

there are 3 ways of merging when having a lefthandside and a righthandside:

LHS over RHS ( setter )

RHS over LHS ( setter )

In case of Collections: combining LHS with RHS ( appender )

In the DefaultReleaseManager you can already see some anonymous classes that are used to control such logic.

I see. Maybe could be the case implement an inner class MergeReleaseDescriptorBuilder and replace all the instancies unless in some cases you really want to merge only a specific properties. Anyway this way it's quite tricky, I say tricky because it's applied more times so the idea of a ReleaseDescriptorBuilder unmutable seems to be not so strong.

ReleaseDescriptor releaseDescriptor = loadReleaseDescriptor( new ReleaseDescriptorBuilder() { public void setActivateProfiles( List<String> profiles ) { builder = performRequest.getReleaseDescriptorBuilder() // merge with actual builder.setActivateProfiles(builder.build().getActivateProfiles() + profiles); } }, performRequest.getReleaseManagerListener() );

there are 3 ways of merging ..

You do not answer my question. When you say

copyPropertiesToReleaseDescriptor means: if property exists, apply that to release descriptor

What should it do in case of a collection ? what "apply" means in your sentence : override existing value, or append to it ?

copyPropertiesToReleaseDescriptor means set

rfscholte · 2020-05-22T10:49:33Z

I don't mind if this piece of code is rewritten. For 3.0.0 I extracted a new interface ReleaseDescriptor, which is part of the API with only (immutable) getters. All new classes introduced were written to keep the original behavior, but with clear separation of reading and writing. DefaultReleaseManager contains code to get the final Release final descriptor based on goal-configuration and release.properties. It looks like that code deserves extra classes in the org.apache.maven.shared.release.config package.

bguerin · 2020-05-22T11:56:13Z

Ok, thanks for answer, I will change and try to find a way that sounds good to you

bguerin · 2020-06-27T14:05:48Z

Sorry, could not find spare time before .. New try, does it sound better ?

Fix UnsupportedOperationException when altering releaseDescriptor.activateProfiles

eolivelli

Hi
Thanks for working on this.
Can you please add tests that ensure that your code does what is expected to achieve?

bguerin · 2020-08-06T08:43:55Z

Hello @eolivelli

I already did ..

eolivelli

I can't see any new test.
Can you please point me those new tests?

bguerin · 2020-08-06T12:20:58Z

This PR is a fix of a previous one : #50, where a new test was added :
https://github.com/apache/maven-release/pull/50/files#diff-1579aec6b35365b482da7a7e6b257f30R710

Here, in this PR, I am also fixing the previously added test

bguerin · 2020-10-16T07:31:49Z

Hello,

Any chance to get this merged ?

rfscholte · 2020-10-16T08:28:38Z

maven-release-manager/src/main/java/org/apache/maven/shared/release/DefaultReleaseManager.java


        if ( specificProfiles != null && !specificProfiles.isEmpty() )
        {
+            List<String> allProfiles = new ArrayList<>();


I guess this part can result is a different set of activated profiles, but it is not covered by a test. Currently, the build is green and we've seen issues with profiles before, so we really need a higher code coverage here.

once again ... this PR fix my previous one, where a Unit Test WAS ADDED

https://github.com/apache/maven-release/pull/50/files#diff-c4d606c66477b937977eccf603f2f04a7b87f464b7439bcb8e4576307792ff2cR709

and this PR changes a little bit this test

But if the current build is green, then there is no need to merge this.

the current build is green because of the new ArrayList( ... ) in the unit test :
secondBuilder.setActivateProfiles( new ArrayList( Arrays.asList("aProfile", "bProfile") ) );

Remove it and the build will become red. And IT WAS a MISTAKE to add it, it is false comparing to what happens when using the plugin

This PR fix the code so that this new ArrayList( ... ) is not needed anymore, and the unit test is representative of the plugin usage

Some background is in https://issues.apache.org/jira/browse/MRELEASE-1042 also ...

Yes I remember the discussion. @rfscholte you wanted by design that the state of descriptor is immutable (link).
The test case use a new arraylist that is mutable but this is not at runtime. I know because we was forced to fork this repo and make our version of release plugin with a issue (1 year ago!).

Here we inject an immutable list - so the test case is in fact wrong.

https://github.com/apache/maven-release/blob/master/maven-release-manager/src/main/java/org/apache/maven/shared/release/config/ReleaseUtils.java#L130-L134

@slachiewicz Yes, EXACTLY !!! The test case added in my previous PR is WRONG, and HERE, in THIS PR, I want to fix it ...
Please please, tell me what to do to get this merged and released, my team just NEED this ...

@rfscholte, @slachiewicz please could you finalize this issue?

rfscholte · 2021-04-02T11:13:07Z

Merged with a553e19
Thanks for the PR

rfscholte reviewed May 19, 2020

View reviewed changes

bguerin requested a review from rfscholte June 27, 2020 14:04

[MRELEASE-1042] releaseProfiles get overriden by activeProfiles

4259e71

Fix UnsupportedOperationException when altering releaseDescriptor.activateProfiles

eolivelli reviewed Aug 5, 2020

View reviewed changes

eolivelli reviewed Aug 6, 2020

View reviewed changes

rfscholte reviewed Oct 16, 2020

View reviewed changes

rfscholte closed this Apr 2, 2021

bguerin deleted the bugfix/MRELEASE-1042 branch April 3, 2021 16:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MRELEASE-1042] releaseProfiles get overriden by activeProfiles #56

[MRELEASE-1042] releaseProfiles get overriden by activeProfiles #56

bguerin commented May 18, 2020

rfscholte May 19, 2020

bguerin May 19, 2020 •

edited

Loading

nfalco79 May 19, 2020

nfalco79 May 19, 2020

bguerin May 19, 2020 •

edited

Loading

nfalco79 May 22, 2020

rfscholte May 22, 2020

nfalco79 May 22, 2020 •

edited

Loading

bguerin May 22, 2020

rfscholte May 22, 2020

rfscholte commented May 22, 2020

bguerin commented May 22, 2020

bguerin commented Jun 27, 2020

eolivelli left a comment

bguerin commented Aug 6, 2020

eolivelli left a comment

bguerin commented Aug 6, 2020

bguerin commented Oct 16, 2020

rfscholte Oct 16, 2020

bguerin Oct 16, 2020

rfscholte Oct 16, 2020

bguerin Oct 16, 2020

bguerin Oct 16, 2020 •

edited

Loading

nfalco79 Oct 16, 2020

slachiewicz Oct 17, 2020

bguerin Feb 16, 2021

nfalco79 Apr 2, 2021 •

edited

Loading

rfscholte commented Apr 2, 2021

[MRELEASE-1042] releaseProfiles get overriden by activeProfiles #56

[MRELEASE-1042] releaseProfiles get overriden by activeProfiles #56

Conversation

bguerin commented May 18, 2020

Choose a reason for hiding this comment

bguerin May 19, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bguerin May 19, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nfalco79 May 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rfscholte commented May 22, 2020

bguerin commented May 22, 2020

bguerin commented Jun 27, 2020

eolivelli left a comment

Choose a reason for hiding this comment

bguerin commented Aug 6, 2020

eolivelli left a comment

Choose a reason for hiding this comment

bguerin commented Aug 6, 2020

bguerin commented Oct 16, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bguerin Oct 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nfalco79 Apr 2, 2021 • edited Loading

Choose a reason for hiding this comment

rfscholte commented Apr 2, 2021

bguerin May 19, 2020 •

edited

Loading

bguerin May 19, 2020 •

edited

Loading

nfalco79 May 22, 2020 •

edited

Loading

bguerin Oct 16, 2020 •

edited

Loading

nfalco79 Apr 2, 2021 •

edited

Loading