Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Closed
wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 20 additions & 8 deletions ...-release-manager/src/main/java/org/apache/maven/shared/release/DefaultReleaseManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
*/

import java.io.File;
import java.util.ArrayList;
import java.util.Collections;
import java.util.LinkedHashSet;
import java.util.List;
Expand Down Expand Up @@ -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<>();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@bguerin bguerin Oct 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 ...

Copy link

@nfalco79 nfalco79 Apr 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

allProfiles.addAll( ReleaseUtils.buildReleaseDescriptor( builder ).getActivateProfiles() );
for ( String specificProfile : specificProfiles )
{
if ( !releaseDescriptor.getActivateProfiles().contains( specificProfile ) )
if ( !allProfiles.contains( specificProfile ) )
{
releaseDescriptor.getActivateProfiles().add( specificProfile );
allProfiles.add( specificProfile );
}
}
builder.setActivateProfiles( allProfiles );
}

ReleaseDescriptor releaseDescriptor = ReleaseUtils.buildReleaseDescriptor( builder );

Strategy releaseStrategy = getStrategy( releaseDescriptor.getReleaseStrategyId() );

List<String> performPhases = getGoalPhases( releaseStrategy, "perform" );
Expand Down Expand Up @@ -521,13 +527,20 @@ protected File determineWorkingDirectory( File checkoutDirectory, String relativ
private BuilderReleaseDescriptor loadReleaseDescriptor( ReleaseDescriptorBuilder builder,
ReleaseManagerListener listener )
throws ReleaseExecutionException
{
return ReleaseUtils.buildReleaseDescriptor( loadReleaseDescriptorBuilder( builder, listener ) );
}

private ReleaseDescriptorBuilder loadReleaseDescriptorBuilder( ReleaseDescriptorBuilder builder,
ReleaseManagerListener listener )
throws ReleaseExecutionException
{
try
{
updateListener( listener, "verify-release-configuration", PHASE_START );
BuilderReleaseDescriptor descriptor = ReleaseUtils.buildReleaseDescriptor( configStore.read( builder ) );
ReleaseDescriptorBuilder result = configStore.read( builder );
updateListener( listener, "verify-release-configuration", PHASE_END );
return descriptor;
return result;
}
catch ( ReleaseDescriptorStoreException e )
{
Expand All @@ -537,7 +550,6 @@ private BuilderReleaseDescriptor loadReleaseDescriptor( ReleaseDescriptorBuilder
}
}


protected void clean( AbstractReleaseRequest releaseRequest ) throws ReleaseFailureException
{
ReleaseCleanRequest cleanRequest = new ReleaseCleanRequest();
Expand Down
2 changes: 1 addition & 1 deletion ...ease-manager/src/test/java/org/apache/maven/shared/release/DefaultReleaseManagerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -725,7 +725,7 @@ public void testKeepProfilesOnPerform()
DefaultReleaseManager releaseManager = (DefaultReleaseManager) lookup( ReleaseManager.class, "test" );

ReleaseDescriptorBuilder secondBuilder = new ReleaseDescriptorBuilder();
secondBuilder.setActivateProfiles( new ArrayList( Arrays.asList("aProfile", "bProfile") ) );
secondBuilder.setActivateProfiles( Arrays.asList("aProfile", "bProfile") );
secondBuilder.setScmSourceUrl( "scm-url" );
ReleaseDescriptorStore configStoreMock = mock( ReleaseDescriptorStore.class );
when( configStoreMock.read( any( ReleaseDescriptorBuilder.class ) ) ).thenReturn( secondBuilder );
Expand Down