nfalco79 commented on a change in pull request #56:
URL: https://github.com/apache/maven-release/pull/56#discussion_r428658561
##########
File path:
maven-release-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 );
Review comment:
I had a look to the history of this file and also to the previous PR. I
have to agree with @rfscholte . The behavior to change the list to modifiable
list here is not clean and does something different than other methods that
also take a list.
Looking the deep the #50 it's better rework how the active profiles are
[added to the
ReleaseDescriptor](https://github.com/apache/maven-release/blob/b968279f72a12daea1836b795125a803bf7e8ccd/maven-release-manager/src/main/java/org/apache/maven/shared/release/DefaultReleaseManager.java#L317)
I do not understand why the
[performRequest.getReleaseDescriptorBuilder()](https://github.com/apache/maven-release/blob/b968279f72a12daea1836b795125a803bf7e8ccd/maven-release-manager/src/main/java/org/apache/maven/shared/release/DefaultReleaseManager.java#L304)
has a list of active profile and than after
[loadReleaseDescriptor](https://github.com/apache/maven-release/blob/b968279f72a12daea1836b795125a803bf7e8ccd/maven-release-manager/src/main/java/org/apache/maven/shared/release/DefaultReleaseManager.java#L307)
some profile has gone so that is needed to be add them again.
EDIT: Ok I got, the ReleaseUtils during property copy overrides any existing
value of the builder. The builder does not have getter...so we can not merge
neither in ReleaseUtil neither in DefaultReleaseManager so what should be done?
My opinion is that the ReleaseUtil must have the knowlegde that profiles are
gathered from many parts so it should handle the copy like a merge:
```
if ( properties.containsKey( "exec.activateProfiles" ) )
{
List<String> profiles = new ArrayList<>(
builder.build().getActivateProfiles() );
for ( String profile : Arrays.asList( properties.getProperty(
"exec.activateProfiles" ).split( "," ) ))
{
if ( StringUtils.isNotBlank(profile) )
{
profile = profile.trim();
if ( !profiles.contains(profile) )
{
profiles.add( profile.trim() );
}
}
}
builder.setActivateProfiles( profiles );
}
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
