You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by GitBox <gi...@apache.org> on 2020/05/18 21:39:38 UTC
[GitHub] [maven-release] bguerin opened a new pull request #56: [MRELEASE-1042] releaseProfiles get overriden by activeProfiles
bguerin opened a new pull request #56:
URL: https://github.com/apache/maven-release/pull/56
Fix UnsupportedOperationException when altering releaseDescriptor.activateProfiles
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [maven-release] nfalco79 commented on a change in pull request #56: [MRELEASE-1042] releaseProfiles get overriden by activeProfiles
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [maven-release] bguerin commented on a change in pull request #56: [MRELEASE-1042] releaseProfiles get overriden by activeProfiles
Posted by GitBox <gi...@apache.org>.
bguerin commented on a change in pull request #56:
URL: https://github.com/apache/maven-release/pull/56#discussion_r427316112
##########
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 confirm that the fix works.
YOUPI !!!
> change the test case
Ok, no problem, I just changed the test I wrote thinking I should not change others
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [maven-release] nfalco79 commented on a change in pull request #56: [MRELEASE-1042] releaseProfiles get overriden by activeProfiles
Posted by GitBox <gi...@apache.org>.
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 o unmodifiable 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 [this](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.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [maven-release] rfscholte commented on a change in pull request #56: [MRELEASE-1042] releaseProfiles get overriden by activeProfiles
Posted by GitBox <gi...@apache.org>.
rfscholte commented on a change in pull request #56:
URL: https://github.com/apache/maven-release/pull/56#discussion_r427064656
##########
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:
This now acts as a merge, not as a setter, so this can't be the right approach.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [maven-release] bguerin commented on a change in pull request #56: [MRELEASE-1042] releaseProfiles get overriden by activeProfiles
Posted by GitBox <gi...@apache.org>.
bguerin commented on a change in pull request #56:
URL: https://github.com/apache/maven-release/pull/56#discussion_r427316112
##########
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 confirm that the fix works.
YOUPI !!!
> change the test case
This is already part of this PR ;)
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [maven-release] rfscholte closed pull request #56: [MRELEASE-1042] releaseProfiles get overriden by activeProfiles
Posted by GitBox <gi...@apache.org>.
rfscholte closed pull request #56:
URL: https://github.com/apache/maven-release/pull/56
--
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:
users@infra.apache.org
[GitHub] [maven-release] nfalco79 commented on a change in pull request #56: [MRELEASE-1042] releaseProfiles get overriden by activeProfiles
Posted by GitBox <gi...@apache.org>.
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 [this](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.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [maven-release] rfscholte commented on a change in pull request #56: [MRELEASE-1042] releaseProfiles get overriden by activeProfiles
Posted by GitBox <gi...@apache.org>.
rfscholte commented on a change in pull request #56:
URL: https://github.com/apache/maven-release/pull/56#discussion_r428602013
##########
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:
Looks a bit weird, right? I can imagine there should be an extra method or class.
Based on this I discovered that ReleaseUtils should be adjusted a bit:
```
public static ReleaseDescriptor buildReleaseDescriptor( ReleaseDescriptorBuilder builder )
{
return builder.build();
}
```
It exposes another hack: `config.setCompletedPhase( name );`, but this one is easy to fix.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [maven-release] bguerin commented on a change in pull request #56: [MRELEASE-1042] releaseProfiles get overriden by activeProfiles
Posted by GitBox <gi...@apache.org>.
bguerin commented on a change in pull request #56:
URL: https://github.com/apache/maven-release/pull/56#discussion_r427111609
##########
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:
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 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:
users@infra.apache.org
[GitHub] [maven-release] nfalco79 commented on a change in pull request #56: [MRELEASE-1042] releaseProfiles get overriden by activeProfiles
Posted by GitBox <gi...@apache.org>.
nfalco79 commented on a change in pull request #56:
URL: https://github.com/apache/maven-release/pull/56#discussion_r429150187
##########
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:
>
>
> 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 it's not so strong.
```
ReleaseDescriptor releaseDescriptor =
loadReleaseDescriptor( new ReleaseDescriptorBuilder() {
public void setActivateProfiles( List<String> profiles ) {
/* merge with */ performRequest.getReleaseDescriptorBuilder(),
}
}, performRequest.getReleaseManagerListener() );
```
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [maven-release] bguerin commented on a change in pull request #56: [MRELEASE-1042] releaseProfiles get overriden by activeProfiles
Posted by GitBox <gi...@apache.org>.
bguerin commented on a change in pull request #56:
URL: https://github.com/apache/maven-release/pull/56#discussion_r428857571
##########
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:
> 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
Yep, this is it
> ReleaseUtil must have the knowlegde that profiles are gathered from many parts so it should handle the copy like a merge
I can't find history of my previous PR anymore, but if I am right, this is what I start to do, and Robert said he prefered to see this specific code in DefaultReleaseManager ...
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [maven-release] nfalco79 commented on a change in pull request #56: [MRELEASE-1042] releaseProfiles get overriden by activeProfiles
Posted by GitBox <gi...@apache.org>.
nfalco79 commented on a change in pull request #56:
URL: https://github.com/apache/maven-release/pull/56#discussion_r429145205
##########
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 the same question
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [maven-release] bguerin commented on a change in pull request #56: [MRELEASE-1042] releaseProfiles get overriden by activeProfiles
Posted by GitBox <gi...@apache.org>.
bguerin commented on a change in pull request #56:
URL: https://github.com/apache/maven-release/pull/56#discussion_r429138172
##########
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:
> copyPropertiesToReleaseDescriptor means: if property exists, apply that to release descriptor
apply == set (override) or merge (keep existing if possible) ?
##########
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:
> copyPropertiesToReleaseDescriptor means: if property exists, apply that to release descriptor
apply == set (override) or merge (keep existing if possible) ?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [maven-release] nfalco79 commented on a change in pull request #56: [MRELEASE-1042] releaseProfiles get overriden by activeProfiles
Posted by GitBox <gi...@apache.org>.
nfalco79 commented on a change in pull request #56:
URL: https://github.com/apache/maven-release/pull/56#discussion_r427272733
##########
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:
we will test ASAP
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [maven-release] nfalco79 commented on a change in pull request #56: [MRELEASE-1042] releaseProfiles get overriden by activeProfiles
Posted by GitBox <gi...@apache.org>.
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.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [maven-release] bguerin commented on a change in pull request #56: [MRELEASE-1042] releaseProfiles get overriden by activeProfiles
Posted by GitBox <gi...@apache.org>.
bguerin commented on a change in pull request #56:
URL: https://github.com/apache/maven-release/pull/56#discussion_r506185320
##########
File path: maven-release-manager/src/main/java/org/apache/maven/shared/release/DefaultReleaseManager.java
##########
@@ -304,21 +305,26 @@ private void perform( ReleasePerformRequest performRequest, ReleaseResult result
ReleaseUtils.buildReleaseDescriptor( performRequest.getReleaseDescriptorBuilder() )
.getActivateProfiles();
- ReleaseDescriptor releaseDescriptor =
- loadReleaseDescriptor( performRequest.getReleaseDescriptorBuilder(),
- performRequest.getReleaseManagerListener() );
+ ReleaseDescriptorBuilder builder =
+ loadReleaseDescriptorBuilder( performRequest.getReleaseDescriptorBuilder(),
+ performRequest.getReleaseManagerListener() );
if ( specificProfiles != null && !specificProfiles.isEmpty() )
{
+ List<String> allProfiles = new ArrayList<>();
Review comment:
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
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [maven-release] bguerin commented on a change in pull request #56: [MRELEASE-1042] releaseProfiles get overriden by activeProfiles
Posted by GitBox <gi...@apache.org>.
bguerin commented on a change in pull request #56:
URL: https://github.com/apache/maven-release/pull/56#discussion_r506256133
##########
File path: maven-release-manager/src/main/java/org/apache/maven/shared/release/DefaultReleaseManager.java
##########
@@ -304,21 +305,26 @@ private void perform( ReleasePerformRequest performRequest, ReleaseResult result
ReleaseUtils.buildReleaseDescriptor( performRequest.getReleaseDescriptorBuilder() )
.getActivateProfiles();
- ReleaseDescriptor releaseDescriptor =
- loadReleaseDescriptor( performRequest.getReleaseDescriptorBuilder(),
- performRequest.getReleaseManagerListener() );
+ ReleaseDescriptorBuilder builder =
+ loadReleaseDescriptorBuilder( performRequest.getReleaseDescriptorBuilder(),
+ performRequest.getReleaseManagerListener() );
if ( specificProfiles != null && !specificProfiles.isEmpty() )
{
+ List<String> allProfiles = new ArrayList<>();
Review comment:
Some backgroud is in https://issues.apache.org/jira/browse/MRELEASE-1042 also ...
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [maven-release] bguerin commented on a change in pull request #56: [MRELEASE-1042] releaseProfiles get overriden by activeProfiles
Posted by GitBox <gi...@apache.org>.
bguerin commented on a change in pull request #56:
URL: https://github.com/apache/maven-release/pull/56#discussion_r429159142
##########
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:
> 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 ?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [maven-release] nfalco79 commented on a change in pull request #56: [MRELEASE-1042] releaseProfiles get overriden by activeProfiles
Posted by GitBox <gi...@apache.org>.
nfalco79 commented on a change in pull request #56:
URL: https://github.com/apache/maven-release/pull/56#discussion_r606108773
##########
File path: maven-release-manager/src/main/java/org/apache/maven/shared/release/DefaultReleaseManager.java
##########
@@ -304,21 +305,26 @@ private void perform( ReleasePerformRequest performRequest, ReleaseResult result
ReleaseUtils.buildReleaseDescriptor( performRequest.getReleaseDescriptorBuilder() )
.getActivateProfiles();
- ReleaseDescriptor releaseDescriptor =
- loadReleaseDescriptor( performRequest.getReleaseDescriptorBuilder(),
- performRequest.getReleaseManagerListener() );
+ ReleaseDescriptorBuilder builder =
+ loadReleaseDescriptorBuilder( performRequest.getReleaseDescriptorBuilder(),
+ performRequest.getReleaseManagerListener() );
if ( specificProfiles != null && !specificProfiles.isEmpty() )
{
+ List<String> allProfiles = new ArrayList<>();
Review comment:
@rfscholte please could you finalize this issue?
--
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:
users@infra.apache.org
[GitHub] [maven-release] rfscholte commented on a change in pull request #56: [MRELEASE-1042] releaseProfiles get overriden by activeProfiles
Posted by GitBox <gi...@apache.org>.
rfscholte commented on a change in pull request #56:
URL: https://github.com/apache/maven-release/pull/56#discussion_r429149122
##########
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:
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 )
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [maven-release] nfalco79 commented on a change in pull request #56: [MRELEASE-1042] releaseProfiles get overriden by activeProfiles
Posted by GitBox <gi...@apache.org>.
nfalco79 commented on a change in pull request #56:
URL: https://github.com/apache/maven-release/pull/56#discussion_r427295559
##########
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:
@bguerin I confirm that the fix works.
I would also suggest to change the test case [here](https://github.com/apache/maven-release/blob/3efc9993aae247cff8e2c7dcd97113a7ef3e69d0/maven-release-manager/src/test/java/org/apache/maven/shared/release/DefaultReleaseManagerTest.java#L728) and use the same approach at [few lines above](https://github.com/apache/maven-release/blob/3efc9993aae247cff8e2c7dcd97113a7ef3e69d0/maven-release-manager/src/test/java/org/apache/maven/shared/release/DefaultReleaseManagerTest.java#L721).
In short replace:
`secondBuilder.setActivateProfiles( new ArrayList( Arrays.asList("aProfile", "bProfile") ) );`
with
`secondBuilder.setActivateProfiles( Arrays.asList( Arrays.asList("aProfile", "bProfile") ) );`
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [maven-release] nfalco79 commented on a change in pull request #56: [MRELEASE-1042] releaseProfiles get overriden by activeProfiles
Posted by GitBox <gi...@apache.org>.
nfalco79 commented on a change in pull request #56:
URL: https://github.com/apache/maven-release/pull/56#discussion_r428507839
##########
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:
If I understand right here you would to instantiate a new ReleaseDescriptorBuilder with the sum of active profiles something like:
```
Properties props = new Properties();
props.put("exec.activateProfiles", profiles);
ReleaseUtil.copyPropertiesToReleaseDescriptor(props , releaseDescriptor)
```
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [maven-release] bguerin commented on a change in pull request #56: [MRELEASE-1042] releaseProfiles get overriden by activeProfiles
Posted by GitBox <gi...@apache.org>.
bguerin commented on a change in pull request #56:
URL: https://github.com/apache/maven-release/pull/56#discussion_r506256133
##########
File path: maven-release-manager/src/main/java/org/apache/maven/shared/release/DefaultReleaseManager.java
##########
@@ -304,21 +305,26 @@ private void perform( ReleasePerformRequest performRequest, ReleaseResult result
ReleaseUtils.buildReleaseDescriptor( performRequest.getReleaseDescriptorBuilder() )
.getActivateProfiles();
- ReleaseDescriptor releaseDescriptor =
- loadReleaseDescriptor( performRequest.getReleaseDescriptorBuilder(),
- performRequest.getReleaseManagerListener() );
+ ReleaseDescriptorBuilder builder =
+ loadReleaseDescriptorBuilder( performRequest.getReleaseDescriptorBuilder(),
+ performRequest.getReleaseManagerListener() );
if ( specificProfiles != null && !specificProfiles.isEmpty() )
{
+ List<String> allProfiles = new ArrayList<>();
Review comment:
Some background is in https://issues.apache.org/jira/browse/MRELEASE-1042 also ...
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [maven-release] bguerin commented on a change in pull request #56: [MRELEASE-1042] releaseProfiles get overriden by activeProfiles
Posted by GitBox <gi...@apache.org>.
bguerin commented on a change in pull request #56:
URL: https://github.com/apache/maven-release/pull/56#discussion_r576812607
##########
File path: maven-release-manager/src/main/java/org/apache/maven/shared/release/DefaultReleaseManager.java
##########
@@ -304,21 +305,26 @@ private void perform( ReleasePerformRequest performRequest, ReleaseResult result
ReleaseUtils.buildReleaseDescriptor( performRequest.getReleaseDescriptorBuilder() )
.getActivateProfiles();
- ReleaseDescriptor releaseDescriptor =
- loadReleaseDescriptor( performRequest.getReleaseDescriptorBuilder(),
- performRequest.getReleaseManagerListener() );
+ ReleaseDescriptorBuilder builder =
+ loadReleaseDescriptorBuilder( performRequest.getReleaseDescriptorBuilder(),
+ performRequest.getReleaseManagerListener() );
if ( specificProfiles != null && !specificProfiles.isEmpty() )
{
+ List<String> allProfiles = new ArrayList<>();
Review comment:
@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 ...
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [maven-release] nfalco79 commented on a change in pull request #56: [MRELEASE-1042] releaseProfiles get overriden by activeProfiles
Posted by GitBox <gi...@apache.org>.
nfalco79 commented on a change in pull request #56:
URL: https://github.com/apache/maven-release/pull/56#discussion_r428507839
##########
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:
If I understand right here you would to instantiate a new ReleaseDescriptorBuilder with the sum of active profiles something like:
```
Properties props = new Properties();
props.put("exec.activateProfiles", profiles.join(","));
ReleaseUtil.copyPropertiesToReleaseDescriptor(props , releaseDescriptor)
```
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [maven-release] rfscholte commented on a change in pull request #56: [MRELEASE-1042] releaseProfiles get overriden by activeProfiles
Posted by GitBox <gi...@apache.org>.
rfscholte commented on a change in pull request #56:
URL: https://github.com/apache/maven-release/pull/56#discussion_r429133494
##########
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:
`ReleaseUtil` should stay as stupid as possible. `copyPropertiesToReleaseDescriptor` means: if property exists, apply that to release descriptor.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [maven-release] bguerin commented on pull request #56: [MRELEASE-1042] releaseProfiles get overriden by activeProfiles
Posted by GitBox <gi...@apache.org>.
bguerin commented on pull request #56:
URL: https://github.com/apache/maven-release/pull/56#issuecomment-709878608
Hello,
Any chance to get this merged ?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [maven-release] bguerin commented on pull request #56: [MRELEASE-1042] releaseProfiles get overriden by activeProfiles
Posted by GitBox <gi...@apache.org>.
bguerin commented on pull request #56:
URL: https://github.com/apache/maven-release/pull/56#issuecomment-650565391
Sorry, could not find spare time before .. New try, does it sound better ?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [maven-release] bguerin commented on pull request #56: [MRELEASE-1042] releaseProfiles get overriden by activeProfiles
Posted by GitBox <gi...@apache.org>.
bguerin commented on pull request #56:
URL: https://github.com/apache/maven-release/pull/56#issuecomment-669893662
This PR is a fix of a previous one : https://github.com/apache/maven-release/pull/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
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [maven-release] bguerin commented on a change in pull request #56: [MRELEASE-1042] releaseProfiles get overriden by activeProfiles
Posted by GitBox <gi...@apache.org>.
bguerin commented on a change in pull request #56:
URL: https://github.com/apache/maven-release/pull/56#discussion_r506255109
##########
File path: maven-release-manager/src/main/java/org/apache/maven/shared/release/DefaultReleaseManager.java
##########
@@ -304,21 +305,26 @@ private void perform( ReleasePerformRequest performRequest, ReleaseResult result
ReleaseUtils.buildReleaseDescriptor( performRequest.getReleaseDescriptorBuilder() )
.getActivateProfiles();
- ReleaseDescriptor releaseDescriptor =
- loadReleaseDescriptor( performRequest.getReleaseDescriptorBuilder(),
- performRequest.getReleaseManagerListener() );
+ ReleaseDescriptorBuilder builder =
+ loadReleaseDescriptorBuilder( performRequest.getReleaseDescriptorBuilder(),
+ performRequest.getReleaseManagerListener() );
if ( specificProfiles != null && !specificProfiles.isEmpty() )
{
+ List<String> allProfiles = new ArrayList<>();
Review comment:
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
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [maven-release] nfalco79 commented on a change in pull request #56: [MRELEASE-1042] releaseProfiles get overriden by activeProfiles
Posted by GitBox <gi...@apache.org>.
nfalco79 commented on a change in pull request #56:
URL: https://github.com/apache/maven-release/pull/56#discussion_r606108773
##########
File path: maven-release-manager/src/main/java/org/apache/maven/shared/release/DefaultReleaseManager.java
##########
@@ -304,21 +305,26 @@ private void perform( ReleasePerformRequest performRequest, ReleaseResult result
ReleaseUtils.buildReleaseDescriptor( performRequest.getReleaseDescriptorBuilder() )
.getActivateProfiles();
- ReleaseDescriptor releaseDescriptor =
- loadReleaseDescriptor( performRequest.getReleaseDescriptorBuilder(),
- performRequest.getReleaseManagerListener() );
+ ReleaseDescriptorBuilder builder =
+ loadReleaseDescriptorBuilder( performRequest.getReleaseDescriptorBuilder(),
+ performRequest.getReleaseManagerListener() );
if ( specificProfiles != null && !specificProfiles.isEmpty() )
{
+ List<String> allProfiles = new ArrayList<>();
Review comment:
@rfscholte, @slachiewicz please could you finalize this issue?
--
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:
users@infra.apache.org
[GitHub] [maven-release] rfscholte commented on a change in pull request #56: [MRELEASE-1042] releaseProfiles get overriden by activeProfiles
Posted by GitBox <gi...@apache.org>.
rfscholte commented on a change in pull request #56:
URL: https://github.com/apache/maven-release/pull/56#discussion_r429174424
##########
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:
`copyPropertiesToReleaseDescriptor` means `set`
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [maven-release] bguerin commented on a change in pull request #56: [MRELEASE-1042] releaseProfiles get overriden by activeProfiles
Posted by GitBox <gi...@apache.org>.
bguerin commented on a change in pull request #56:
URL: https://github.com/apache/maven-release/pull/56#discussion_r427111609
##########
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:
This is not a merge, I do not keel the old value. It is a defensive setter, ensuring that the list implementation used is not read only / immuable
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [maven-release] nfalco79 commented on a change in pull request #56: [MRELEASE-1042] releaseProfiles get overriden by activeProfiles
Posted by GitBox <gi...@apache.org>.
nfalco79 commented on a change in pull request #56:
URL: https://github.com/apache/maven-release/pull/56#discussion_r428611240
##########
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 did get. What should do `config.setCompletedPhase( name )` ?
I mean the request builder it's a builder so I expect I can change all I want and only after executed the build method than I obtain an unmodifiable object. You agree?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [maven-release] bguerin commented on a change in pull request #56: [MRELEASE-1042] releaseProfiles get overriden by activeProfiles
Posted by GitBox <gi...@apache.org>.
bguerin commented on a change in pull request #56:
URL: https://github.com/apache/maven-release/pull/56#discussion_r427111609
##########
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:
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
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [maven-release] nfalco79 commented on a change in pull request #56: [MRELEASE-1042] releaseProfiles get overriden by activeProfiles
Posted by GitBox <gi...@apache.org>.
nfalco79 commented on a change in pull request #56:
URL: https://github.com/apache/maven-release/pull/56#discussion_r428611240
##########
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 didn't get.
What should do `config.setCompletedPhase( name )` ?
I mean the request builder it's a builder so I expect I can change all I want and only after execute the build method than I obtain an unmodifiable object. You agree?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [maven-release] rfscholte commented on pull request #56: [MRELEASE-1042] releaseProfiles get overriden by activeProfiles
Posted by GitBox <gi...@apache.org>.
rfscholte commented on pull request #56:
URL: https://github.com/apache/maven-release/pull/56#issuecomment-632628280
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.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [maven-release] rfscholte commented on pull request #56: [MRELEASE-1042] releaseProfiles get overriden by activeProfiles
Posted by GitBox <gi...@apache.org>.
rfscholte commented on pull request #56:
URL: https://github.com/apache/maven-release/pull/56#issuecomment-812486023
Merged with https://github.com/apache/maven-release/commit/a553e1921a3633848e8a886a0286bf45047b9ab5
Thanks for the PR
--
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:
users@infra.apache.org
[GitHub] [maven-release] rfscholte commented on a change in pull request #56: [MRELEASE-1042] releaseProfiles get overriden by activeProfiles
Posted by GitBox <gi...@apache.org>.
rfscholte commented on a change in pull request #56:
URL: https://github.com/apache/maven-release/pull/56#discussion_r506198790
##########
File path: maven-release-manager/src/main/java/org/apache/maven/shared/release/DefaultReleaseManager.java
##########
@@ -304,21 +305,26 @@ private void perform( ReleasePerformRequest performRequest, ReleaseResult result
ReleaseUtils.buildReleaseDescriptor( performRequest.getReleaseDescriptorBuilder() )
.getActivateProfiles();
- ReleaseDescriptor releaseDescriptor =
- loadReleaseDescriptor( performRequest.getReleaseDescriptorBuilder(),
- performRequest.getReleaseManagerListener() );
+ ReleaseDescriptorBuilder builder =
+ loadReleaseDescriptorBuilder( performRequest.getReleaseDescriptorBuilder(),
+ performRequest.getReleaseManagerListener() );
if ( specificProfiles != null && !specificProfiles.isEmpty() )
{
+ List<String> allProfiles = new ArrayList<>();
Review comment:
But if the current build is green, then there is no need to merge this.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [maven-release] nfalco79 commented on a change in pull request #56: [MRELEASE-1042] releaseProfiles get overriden by activeProfiles
Posted by GitBox <gi...@apache.org>.
nfalco79 commented on a change in pull request #56:
URL: https://github.com/apache/maven-release/pull/56#discussion_r506370075
##########
File path: maven-release-manager/src/main/java/org/apache/maven/shared/release/DefaultReleaseManager.java
##########
@@ -304,21 +305,26 @@ private void perform( ReleasePerformRequest performRequest, ReleaseResult result
ReleaseUtils.buildReleaseDescriptor( performRequest.getReleaseDescriptorBuilder() )
.getActivateProfiles();
- ReleaseDescriptor releaseDescriptor =
- loadReleaseDescriptor( performRequest.getReleaseDescriptorBuilder(),
- performRequest.getReleaseManagerListener() );
+ ReleaseDescriptorBuilder builder =
+ loadReleaseDescriptorBuilder( performRequest.getReleaseDescriptorBuilder(),
+ performRequest.getReleaseManagerListener() );
if ( specificProfiles != null && !specificProfiles.isEmpty() )
{
+ List<String> allProfiles = new ArrayList<>();
Review comment:
Yes I remember the discussion. @rfscholte you wanted by design that the state of descriptor is immutable ([link](https://github.com/apache/maven-release/pull/56#discussion_r427064656)).
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!).
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [maven-release] rfscholte commented on a change in pull request #56: [MRELEASE-1042] releaseProfiles get overriden by activeProfiles
Posted by GitBox <gi...@apache.org>.
rfscholte commented on a change in pull request #56:
URL: https://github.com/apache/maven-release/pull/56#discussion_r429134067
##########
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:
In the `DefaultReleaseManager` you can already see some anonymous classes that are used to control such logic.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [maven-release] rfscholte commented on a change in pull request #56: [MRELEASE-1042] releaseProfiles get overriden by activeProfiles
Posted by GitBox <gi...@apache.org>.
rfscholte commented on a change in pull request #56:
URL: https://github.com/apache/maven-release/pull/56#discussion_r428489987
##########
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 still don't like this approach and think any value of the releaseDescriptor should be immutable. With the introduction of the release-api it should be easier to write your own phases, but I don't want any of the phases to change the release descriptor.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [maven-release] bguerin commented on pull request #56: [MRELEASE-1042] releaseProfiles get overriden by activeProfiles
Posted by GitBox <gi...@apache.org>.
bguerin commented on pull request #56:
URL: https://github.com/apache/maven-release/pull/56#issuecomment-632654403
Ok, thanks for answer, I will change and try to find a way that sounds good to you
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [maven-release] bguerin commented on pull request #56: [MRELEASE-1042] releaseProfiles get overriden by activeProfiles
Posted by GitBox <gi...@apache.org>.
bguerin commented on pull request #56:
URL: https://github.com/apache/maven-release/pull/56#issuecomment-669797524
Hello @eolivelli
I already did ..
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [maven-release] rfscholte commented on a change in pull request #56: [MRELEASE-1042] releaseProfiles get overriden by activeProfiles
Posted by GitBox <gi...@apache.org>.
rfscholte commented on a change in pull request #56:
URL: https://github.com/apache/maven-release/pull/56#discussion_r506167903
##########
File path: maven-release-manager/src/main/java/org/apache/maven/shared/release/DefaultReleaseManager.java
##########
@@ -304,21 +305,26 @@ private void perform( ReleasePerformRequest performRequest, ReleaseResult result
ReleaseUtils.buildReleaseDescriptor( performRequest.getReleaseDescriptorBuilder() )
.getActivateProfiles();
- ReleaseDescriptor releaseDescriptor =
- loadReleaseDescriptor( performRequest.getReleaseDescriptorBuilder(),
- performRequest.getReleaseManagerListener() );
+ ReleaseDescriptorBuilder builder =
+ loadReleaseDescriptorBuilder( performRequest.getReleaseDescriptorBuilder(),
+ performRequest.getReleaseManagerListener() );
if ( specificProfiles != null && !specificProfiles.isEmpty() )
{
+ List<String> allProfiles = new ArrayList<>();
Review comment:
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.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [maven-release] nfalco79 commented on a change in pull request #56: [MRELEASE-1042] releaseProfiles get overriden by activeProfiles
Posted by GitBox <gi...@apache.org>.
nfalco79 commented on a change in pull request #56:
URL: https://github.com/apache/maven-release/pull/56#discussion_r429150187
##########
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:
>
>
> 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() );
```
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [maven-release] slachiewicz commented on a change in pull request #56: [MRELEASE-1042] releaseProfiles get overriden by activeProfiles
Posted by GitBox <gi...@apache.org>.
slachiewicz commented on a change in pull request #56:
URL: https://github.com/apache/maven-release/pull/56#discussion_r506953558
##########
File path: maven-release-manager/src/main/java/org/apache/maven/shared/release/DefaultReleaseManager.java
##########
@@ -304,21 +305,26 @@ private void perform( ReleasePerformRequest performRequest, ReleaseResult result
ReleaseUtils.buildReleaseDescriptor( performRequest.getReleaseDescriptorBuilder() )
.getActivateProfiles();
- ReleaseDescriptor releaseDescriptor =
- loadReleaseDescriptor( performRequest.getReleaseDescriptorBuilder(),
- performRequest.getReleaseManagerListener() );
+ ReleaseDescriptorBuilder builder =
+ loadReleaseDescriptorBuilder( performRequest.getReleaseDescriptorBuilder(),
+ performRequest.getReleaseManagerListener() );
if ( specificProfiles != null && !specificProfiles.isEmpty() )
{
+ List<String> allProfiles = new ArrayList<>();
Review comment:
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
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [maven-release] nfalco79 commented on a change in pull request #56: [MRELEASE-1042] releaseProfiles get overriden by activeProfiles
Posted by GitBox <gi...@apache.org>.
nfalco79 commented on a change in pull request #56:
URL: https://github.com/apache/maven-release/pull/56#discussion_r428611240
##########
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 did get. What should do `config.setCompletedPhase( name )` ?
----------------------------------------------------------------
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:
users@infra.apache.org