[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
