[GitHub] [maven-release] bguerin commented on a change in pull request #56: [MRELEASE-1042] releaseProfiles get overriden by activeProfiles
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 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: us...@infra.apache.org
[GitHub] [maven-release] bguerin commented on a change in pull request #56: [MRELEASE-1042] releaseProfiles get overriden by activeProfiles
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 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: us...@infra.apache.org
[GitHub] [maven-release] bguerin commented on a change in pull request #56: [MRELEASE-1042] releaseProfiles get overriden by activeProfiles
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 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: us...@infra.apache.org
[GitHub] [maven-release] bguerin commented on a change in pull request #56: [MRELEASE-1042] releaseProfiles get overriden by activeProfiles
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 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: us...@infra.apache.org
[GitHub] [maven-release] bguerin commented on a change in pull request #56: [MRELEASE-1042] releaseProfiles get overriden by activeProfiles
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 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: us...@infra.apache.org
[GitHub] [maven-release] bguerin commented on a change in pull request #56: [MRELEASE-1042] releaseProfiles get overriden by activeProfiles
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 profiles ) { -releaseDescriptor.setActivateProfiles( profiles ); +List 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: us...@infra.apache.org
[GitHub] [maven-release] bguerin commented on a change in pull request #56: [MRELEASE-1042] releaseProfiles get overriden by activeProfiles
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 profiles ) { -releaseDescriptor.setActivateProfiles( profiles ); +List 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 profiles ) { -releaseDescriptor.setActivateProfiles( profiles ); +List 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: us...@infra.apache.org
[GitHub] [maven-release] bguerin commented on a change in pull request #56: [MRELEASE-1042] releaseProfiles get overriden by activeProfiles
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 profiles ) { -releaseDescriptor.setActivateProfiles( profiles ); +List 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: us...@infra.apache.org
[GitHub] [maven-release] bguerin commented on a change in pull request #56: [MRELEASE-1042] releaseProfiles get overriden by activeProfiles
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 profiles ) { -releaseDescriptor.setActivateProfiles( profiles ); +List 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: us...@infra.apache.org
[GitHub] [maven-release] bguerin commented on a change in pull request #56: [MRELEASE-1042] releaseProfiles get overriden by activeProfiles
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 profiles ) { -releaseDescriptor.setActivateProfiles( profiles ); +List 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: us...@infra.apache.org
[GitHub] [maven-release] bguerin commented on a change in pull request #56: [MRELEASE-1042] releaseProfiles get overriden by activeProfiles
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 profiles ) { -releaseDescriptor.setActivateProfiles( profiles ); +List 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: us...@infra.apache.org
[GitHub] [maven-release] bguerin commented on a change in pull request #56: [MRELEASE-1042] releaseProfiles get overriden by activeProfiles
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 profiles ) { -releaseDescriptor.setActivateProfiles( profiles ); +List 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: us...@infra.apache.org
[GitHub] [maven-release] bguerin commented on a change in pull request #56: [MRELEASE-1042] releaseProfiles get overriden by activeProfiles
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 profiles ) { -releaseDescriptor.setActivateProfiles( profiles ); +List 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: us...@infra.apache.org