michael-o commented on code in PR #118:
URL: https://github.com/apache/maven-release/pull/118#discussion_r863513916
##########
maven-release-manager/pom.xml:
##########
@@ -59,14 +60,14 @@
</dependency>
<dependency>
- <groupId>org.sonatype.plexus</groupId>
+ <groupId>org.codehaus.plexus</groupId>
<artifactId>plexus-sec-dispatcher</artifactId>
- <version>1.3</version>
+ <version>2.0</version>
</dependency>
<dependency>
- <groupId>org.sonatype.plexus</groupId>
+ <groupId>org.codehaus.plexus</groupId>
<artifactId>plexus-cipher</artifactId>
- <version>1.7</version>
+ <version>2.0</version>
Review Comment:
Perfect thank you!
##########
maven-release-manager/src/main/java/org/apache/maven/shared/release/DefaultReleaseManager.java:
##########
@@ -37,38 +42,56 @@
import org.apache.maven.shared.release.phase.ReleasePhase;
import org.apache.maven.shared.release.phase.ResourceGenerator;
import org.apache.maven.shared.release.strategy.Strategy;
-import org.codehaus.plexus.component.annotations.Component;
-import org.codehaus.plexus.component.annotations.Requirement;
-import org.codehaus.plexus.logging.AbstractLogEnabled;
import org.codehaus.plexus.util.StringUtils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import static java.util.Objects.requireNonNull;
/**
* Implementation of the release manager.
*
* @author <a href="mailto:br...@apache.org";>Brett Porter</a>
*/
-@Component( role = ReleaseManager.class )
+@Singleton
+@Named
public class DefaultReleaseManager
- extends AbstractLogEnabled
implements ReleaseManager
{
- @Requirement
- private Map<String, Strategy> strategies;
+ private static final Logger LOGGER = LoggerFactory.getLogger(
DefaultReleaseManager.class );
+
+ private final Map<String, Strategy> strategies;
/**
* The available phases.
*/
- @Requirement
- private Map<String, ReleasePhase> releasePhases;
+ private final Map<String, ReleasePhase> releasePhases;
/**
* The configuration storage.
*/
- @Requirement( hint = "properties" )
- private ReleaseDescriptorStore configStore;
+ private final AtomicReference<ReleaseDescriptorStore> configStore;
private static final int PHASE_SKIP = 0, PHASE_START = 1, PHASE_END = 2,
GOAL_END = 12, ERROR = 99;
+ @Inject
+ public DefaultReleaseManager( Map<String, Strategy> strategies,
+ Map<String, ReleasePhase> releasePhases,
+ @Named( "properties" )
ReleaseDescriptorStore configStore )
+ {
+ this.strategies = requireNonNull( strategies );
+ this.releasePhases = requireNonNull( releasePhases );
+ this.configStore = new AtomicReference<>( requireNonNull( configStore
) );
Review Comment:
Can you explain why this requires an atomic ref?
##########
maven-release-manager/src/main/java/org/apache/maven/shared/release/config/PropertiesReleaseDescriptorStore.java:
##########
@@ -101,21 +103,21 @@ public ReleaseDescriptorBuilder read(
ReleaseDescriptorBuilder mergeDescriptor,
}
catch ( FileNotFoundException e )
{
- getLogger().debug( file.getName() + " not found - using empty
properties" );
+ LOGGER.debug( file.getName() + " not found - using empty
properties" );
}
catch ( IOException e )
{
throw new ReleaseDescriptorStoreException(
- "Error reading properties file '" + file.getName() + "': " +
e.getMessage(), e );
+ "Error reading properties file '" + file.getName() + "': "
+ e.getMessage(), e );
Review Comment:
I hate this ugly duplication of exception messages, but this is not part of
this PR.
##########
maven-release-manager/src/main/java/org/apache/maven/shared/release/config/PropertiesReleaseDescriptorStore.java:
##########
@@ -101,21 +103,21 @@ public ReleaseDescriptorBuilder read(
ReleaseDescriptorBuilder mergeDescriptor,
}
catch ( FileNotFoundException e )
{
- getLogger().debug( file.getName() + " not found - using empty
properties" );
+ LOGGER.debug( file.getName() + " not found - using empty
properties" );
}
catch ( IOException e )
{
throw new ReleaseDescriptorStoreException(
- "Error reading properties file '" + file.getName() + "': " +
e.getMessage(), e );
+ "Error reading properties file '" + file.getName() + "': "
+ e.getMessage(), e );
}
-
+
try
{
- decryptProperties( properties );
+ mavenCrypto.decryptProperties( properties );
}
- catch ( IllegalStateException | SecDispatcherException |
PlexusCipherException e )
+ catch ( MavenCryptoException e )
{
- getLogger().debug( e.getMessage() );
+ LOGGER.debug( e.getMessage() );
Review Comment:
Ha ha ha, crypto fails, but hey let's continue anyway.
##########
maven-release-manager/src/main/java/org/apache/maven/shared/release/phase/AbstractRewritePomsPhase.java:
##########
@@ -530,25 +537,25 @@ else if ( mappedVersion.equals( propertyValue ) )
{
// this property may have been updated during
processing a sibling.
logInfo( result, " Ignoring artifact version
update for expression " + rawVersion
- + " because it is already updated" );
+ + " because it is already updated" );
}
else if ( !mappedVersion.equals( rawVersion ) )
{
if ( mappedVersion.matches(
"\\$\\{project.+\\}" )
- || mappedVersion.matches( "\\$\\{pom.+\\}"
)
- || "${version}".equals( mappedVersion ) )
+ || mappedVersion.matches(
"\\$\\{pom.+\\}" )
+ || "${version}".equals( mappedVersion
) )
Review Comment:
Can you add a comment here that `pom.` and prefixless is gone in Maven 4?
##########
maven-release-manager/pom.xml:
##########
@@ -98,14 +116,25 @@
<dependency>
<groupId>commons-cli</groupId>
<artifactId>commons-cli</artifactId>
- <version>1.2</version>
+ <version>1.5.0</version>
Review Comment:
I don't know how much is needed here, but @mthmulders put some effect to
change Maven Code code to 1.5.0 for those APIs which are deprecated. Does this
apply here as well?
--
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.
To unsubscribe, e-mail: issues-unsubscr...@maven.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
