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/12/10 13:17:20 UTC
[GitHub] [maven-deploy-plugin] cstamas opened a new pull request #12: [MDEPLOY-279] Better validation for alt deploy repository mojo parameter
cstamas opened a new pull request #12:
URL: https://github.com/apache/maven-deploy-plugin/pull/12
Do accept legacy format, but ONLY if layout is "default", otherwise
fail with meaningful error message. Also, do log a WARN log if
legacy parameter form used, tell user to align parameter form
with plugin version.
https://issues.apache.org/jira/browse/MDEPLOY-279
----------------------------------------------------------------
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-deploy-plugin] michael-o commented on pull request #12: [MDEPLOY-279] Better validation for alt deploy repository mojo parameter
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #12:
URL: https://github.com/apache/maven-deploy-plugin/pull/12#issuecomment-748683495
Thanks for bearing with me...
----------------------------------------------------------------
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-deploy-plugin] cstamas commented on a change in pull request #12: [MDEPLOY-279] Better validation for alt deploy repository mojo parameter
Posted by GitBox <gi...@apache.org>.
cstamas commented on a change in pull request #12:
URL: https://github.com/apache/maven-deploy-plugin/pull/12#discussion_r543553533
##########
File path: src/main/java/org/apache/maven/plugins/deploy/DeployMojo.java
##########
@@ -51,6 +51,9 @@
public class DeployMojo
extends AbstractDeployMojo
{
+ private static final Pattern ALT_INVALID_REPO_SYNTAX_PATTERN = Pattern.compile( "(.+)::(.+)::(.+)::(.+)" );
Review comment:
Done
----------------------------------------------------------------
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-deploy-plugin] michael-o commented on pull request #12: [MDEPLOY-279] Better validation for alt deploy repository mojo parameter
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #12:
URL: https://github.com/apache/maven-deploy-plugin/pull/12#issuecomment-748681563
Looks good to me now, I will run tests 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-deploy-plugin] cstamas commented on a change in pull request #12: [MDEPLOY-279] Better validation for alt deploy repository mojo parameter
Posted by GitBox <gi...@apache.org>.
cstamas commented on a change in pull request #12:
URL: https://github.com/apache/maven-deploy-plugin/pull/12#discussion_r541172518
##########
File path: src/main/java/org/apache/maven/plugins/deploy/DeployMojo.java
##########
@@ -248,19 +249,40 @@ else if ( !ArtifactUtils.isSnapshot( project.getVersion() ) && altReleaseDeploym
{
getLog().info( "Using alternate deployment repository " + altDeploymentRepo );
- Matcher matcher = ALT_REPO_SYNTAX_PATTERN.matcher( altDeploymentRepo );
+ Matcher matcher = ALT_LEGACY_REPO_SYNTAX_PATTERN.matcher( altDeploymentRepo );
- if ( !matcher.matches() )
+ if ( matcher.matches() )
{
- throw new MojoFailureException( altDeploymentRepo, "Invalid syntax for repository.",
- "Invalid syntax for alternative repository. Use \"id::url\"." );
+ getLog().warn( "Legacy form of alternate deployment parameter used, update parameter to match plugin version." );
Review comment:
Idea: Maybe move this log below, and use `id` and `url` to show end user to what it should update. And not show this very same warn if legacy layout is NOT `"default"` but fail Mojo with exception...
----------------------------------------------------------------
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-deploy-plugin] asfgit closed pull request #12: [MDEPLOY-279] Better validation for alt deploy repository mojo parameter
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #12:
URL: https://github.com/apache/maven-deploy-plugin/pull/12
----------------------------------------------------------------
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-deploy-plugin] cstamas commented on pull request #12: [MDEPLOY-279] Better validation for alt deploy repository mojo parameter
Posted by GitBox <gi...@apache.org>.
cstamas commented on pull request #12:
URL: https://github.com/apache/maven-deploy-plugin/pull/12#issuecomment-743361871
I literally copy+pasted m-d-p regexp from 2.x to be 100% sure it is what it is (to match exactly what 2.x did match). Also, IMHO this IF-ELSE clearly explains what and why happens. Is redundant, but buys it out at clarity IMO.
----------------------------------------------------------------
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-deploy-plugin] cstamas commented on a change in pull request #12: [MDEPLOY-279] Better validation for alt deploy repository mojo parameter
Posted by GitBox <gi...@apache.org>.
cstamas commented on a change in pull request #12:
URL: https://github.com/apache/maven-deploy-plugin/pull/12#discussion_r541561424
##########
File path: src/main/java/org/apache/maven/plugins/deploy/DeployMojo.java
##########
@@ -248,19 +249,40 @@ else if ( !ArtifactUtils.isSnapshot( project.getVersion() ) && altReleaseDeploym
{
getLog().info( "Using alternate deployment repository " + altDeploymentRepo );
- Matcher matcher = ALT_REPO_SYNTAX_PATTERN.matcher( altDeploymentRepo );
+ Matcher matcher = ALT_LEGACY_REPO_SYNTAX_PATTERN.matcher( altDeploymentRepo );
- if ( !matcher.matches() )
+ if ( matcher.matches() )
{
- throw new MojoFailureException( altDeploymentRepo, "Invalid syntax for repository.",
- "Invalid syntax for alternative repository. Use \"id::url\"." );
+ getLog().warn( "Legacy form of alternate deployment parameter used, update parameter to match plugin version." );
Review comment:
... now only to figure out on which computer I created this PR :laughing:
----------------------------------------------------------------
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-deploy-plugin] michael-o commented on a change in pull request #12: [MDEPLOY-279] Better validation for alt deploy repository mojo parameter
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #12:
URL: https://github.com/apache/maven-deploy-plugin/pull/12#discussion_r542003179
##########
File path: src/main/java/org/apache/maven/plugins/deploy/DeployMojo.java
##########
@@ -51,6 +51,9 @@
public class DeployMojo
extends AbstractDeployMojo
{
+ private static final Pattern ALT_INVALID_REPO_SYNTAX_PATTERN = Pattern.compile( "(.+)::(.+)::(.+)::(.+)" );
Review comment:
I do not fully understand the purpose of this. You can extend this ad absurdum with zillions of double-colons. We cannot avoid this, but is not wrong:
`funny-id::https:///server/some/url/sub::/...`. I would rather drop this and make both other formats non-greedy all `n-1` matching groups.
----------------------------------------------------------------
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-deploy-plugin] cstamas commented on a change in pull request #12: [MDEPLOY-279] Better validation for alt deploy repository mojo parameter
Posted by GitBox <gi...@apache.org>.
cstamas commented on a change in pull request #12:
URL: https://github.com/apache/maven-deploy-plugin/pull/12#discussion_r541560654
##########
File path: src/main/java/org/apache/maven/plugins/deploy/DeployMojo.java
##########
@@ -248,19 +249,40 @@ else if ( !ArtifactUtils.isSnapshot( project.getVersion() ) && altReleaseDeploym
{
getLog().info( "Using alternate deployment repository " + altDeploymentRepo );
- Matcher matcher = ALT_REPO_SYNTAX_PATTERN.matcher( altDeploymentRepo );
+ Matcher matcher = ALT_LEGACY_REPO_SYNTAX_PATTERN.matcher( altDeploymentRepo );
- if ( !matcher.matches() )
+ if ( matcher.matches() )
{
- throw new MojoFailureException( altDeploymentRepo, "Invalid syntax for repository.",
- "Invalid syntax for alternative repository. Use \"id::url\"." );
+ getLog().warn( "Legacy form of alternate deployment parameter used, update parameter to match plugin version." );
Review comment:
Will rework the PR then as follows:
* Mojo will first match LEGACY pattern,
* if matched, build will fail, but the message will be saying what the problem is (legacy notation) and provide solution (modern notation), will not "magically" continue if layout is `default`
* continue with current pattern...
This order (legacy then modern) is needed, as we saw, that modern pattern "matches" legacy notation, but messes up repositoryId (ends up w/ `serverId::default` and fails to find serverId in settings). This way, we exclude that possibility to have 2 (legacy), 3 (plain wrong) and more `::`s in string, as all those will be caught by legacy pattern (and in that case would provide bad "solution".
----------------------------------------------------------------
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-deploy-plugin] michael-o commented on pull request #12: [MDEPLOY-279] Better validation for alt deploy repository mojo parameter
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #12:
URL: https://github.com/apache/maven-deploy-plugin/pull/12#issuecomment-748673432
Going through now...
----------------------------------------------------------------
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-deploy-plugin] michael-o commented on pull request #12: [MDEPLOY-279] Better validation for alt deploy repository mojo parameter
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #12:
URL: https://github.com/apache/maven-deploy-plugin/pull/12#issuecomment-745464748
Thanks, will look into new changes.
----------------------------------------------------------------
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-deploy-plugin] mickroll commented on a change in pull request #12: [MDEPLOY-279] Better validation for alt deploy repository mojo parameter
Posted by GitBox <gi...@apache.org>.
mickroll commented on a change in pull request #12:
URL: https://github.com/apache/maven-deploy-plugin/pull/12#discussion_r541552391
##########
File path: src/main/java/org/apache/maven/plugins/deploy/DeployMojo.java
##########
@@ -248,19 +249,40 @@ else if ( !ArtifactUtils.isSnapshot( project.getVersion() ) && altReleaseDeploym
{
getLog().info( "Using alternate deployment repository " + altDeploymentRepo );
- Matcher matcher = ALT_REPO_SYNTAX_PATTERN.matcher( altDeploymentRepo );
+ Matcher matcher = ALT_LEGACY_REPO_SYNTAX_PATTERN.matcher( altDeploymentRepo );
- if ( !matcher.matches() )
+ if ( matcher.matches() )
{
- throw new MojoFailureException( altDeploymentRepo, "Invalid syntax for repository.",
- "Invalid syntax for alternative repository. Use \"id::url\"." );
+ getLog().warn( "Legacy form of alternate deployment parameter used, update parameter to match plugin version." );
Review comment:
This message hides a lot of information that was gathered before. At this point, the plugin knows that `id::layout::url` was used (but expects `id::url`, of course). The message should reflect that, not just by saying `you used the old notation, use the new one. RTFM`, but more like `you used the 2.x notation id::layout::url, 3.x expects id::url`.
This would give not just a hint to whats wrong, but provide a solution... To me, this is the difference of a good plugin to a great one :sunglasses:
With this, it would be totally fine (and expected) to fail with the message, not continuing the build 'magically' if the layout is `default`.
----------------------------------------------------------------
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-deploy-plugin] michael-o commented on a change in pull request #12: [MDEPLOY-279] Better validation for alt deploy repository mojo parameter
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #12:
URL: https://github.com/apache/maven-deploy-plugin/pull/12#discussion_r546440502
##########
File path: src/test/java/org/apache/maven/plugins/deploy/DeployMojoTest.java
##########
@@ -551,7 +552,88 @@ public void _testBasicDeployWithScpAsProtocol()
FileUtils.deleteDirectory( sshFile );
}
-
+
+ public void testLegacyAltDeploymentRepositoryWithDefaultLayout()
+ throws Exception
+ {
+ DeployMojo mojo = spy( new DeployMojo() );
+
+ ArtifactRepository repository = mock( ArtifactRepository.class );
+ when( mojo.createDeploymentArtifactRepository( "altDeploymentRepository", "http://localhost"
+ ) ).thenReturn( repository );
+
+ project.setVersion( "1.0-SNAPSHOT" );
+
+ ProjectDeployerRequest pdr =
+ new ProjectDeployerRequest()
+ .setProject( project )
+ .setAltDeploymentRepository( "altDeploymentRepository::default::http://localhost" );
+ try
+ {
+ mojo.getDeploymentRepository( pdr );
+ fail( "Should throw: Invalid legacy syntax for repository." );
+ }
+ catch( MojoFailureException e )
+ {
+ assertEquals( e.getMessage(), "Invalid legacy syntax for repository.");
+ assertEquals( e.getLongMessage(), "Invalid legacy syntax for alternative repository. Use \"altDeploymentRepository::http://localhost\" instead.");
+ }
+ }
+
+ public void testLegacyAltDeploymentRepositoryWithLegacyLayout()
+ throws Exception
+ {
+ DeployMojo mojo = spy( new DeployMojo() );
+
+ ArtifactRepository repository = mock( ArtifactRepository.class );
+ when( mojo.createDeploymentArtifactRepository( "altDeploymentRepository", "http://localhost"
+ ) ).thenReturn( repository );
+
+ project.setVersion( "1.0-SNAPSHOT" );
+
+ ProjectDeployerRequest pdr =
+ new ProjectDeployerRequest()
+ .setProject( project )
+ .setAltDeploymentRepository( "altDeploymentRepository::legacy::http://localhost" );
+ try
+ {
+ mojo.getDeploymentRepository( pdr );
+ fail( "Should throw: Invalid legacy syntax for repository." );
+ }
+ catch( MojoFailureException e )
+ {
+ assertEquals( e.getMessage(), "Invalid legacy syntax for repository.");
+ assertEquals( e.getLongMessage(), "Invalid legacy syntax for alternative repository. Use \"altDeploymentRepository::http://localhost\" instead.");
+ }
+ }
+
+ public void testInsaneAltDeploymentRepository()
+ throws Exception
+ {
+ DeployMojo mojo = spy( new DeployMojo() );
+
+ ArtifactRepository repository = mock( ArtifactRepository.class );
+ when( mojo.createDeploymentArtifactRepository( "altDeploymentRepository", "http://localhost"
+ ) ).thenReturn( repository );
+
+ project.setVersion( "1.0-SNAPSHOT" );
+
+ ProjectDeployerRequest pdr =
+ new ProjectDeployerRequest()
+ .setProject( project )
+ .setAltDeploymentRepository( "altDeploymentRepository::hey::wow::foo::http://localhost" );
Review comment:
I consider this to be wrong too because the spilt should read:
`altDeploymentRepository`: id
`hey`: layout
`wow::foo::http://localhost`: URL
I think a valid case for a URL is `scm:svn:https://...` That's why we have to be non-greedy.
New style is `altDeploymentRepository::scm:svn:https://...`
##########
File path: src/test/java/org/apache/maven/plugins/deploy/DeployMojoTest.java
##########
@@ -551,7 +552,88 @@ public void _testBasicDeployWithScpAsProtocol()
FileUtils.deleteDirectory( sshFile );
}
-
+
+ public void testLegacyAltDeploymentRepositoryWithDefaultLayout()
+ throws Exception
+ {
+ DeployMojo mojo = spy( new DeployMojo() );
+
+ ArtifactRepository repository = mock( ArtifactRepository.class );
+ when( mojo.createDeploymentArtifactRepository( "altDeploymentRepository", "http://localhost"
+ ) ).thenReturn( repository );
+
+ project.setVersion( "1.0-SNAPSHOT" );
+
+ ProjectDeployerRequest pdr =
+ new ProjectDeployerRequest()
+ .setProject( project )
+ .setAltDeploymentRepository( "altDeploymentRepository::default::http://localhost" );
+ try
+ {
+ mojo.getDeploymentRepository( pdr );
+ fail( "Should throw: Invalid legacy syntax for repository." );
+ }
+ catch( MojoFailureException e )
+ {
+ assertEquals( e.getMessage(), "Invalid legacy syntax for repository.");
+ assertEquals( e.getLongMessage(), "Invalid legacy syntax for alternative repository. Use \"altDeploymentRepository::http://localhost\" instead.");
+ }
+ }
+
+ public void testLegacyAltDeploymentRepositoryWithLegacyLayout()
+ throws Exception
+ {
+ DeployMojo mojo = spy( new DeployMojo() );
+
+ ArtifactRepository repository = mock( ArtifactRepository.class );
+ when( mojo.createDeploymentArtifactRepository( "altDeploymentRepository", "http://localhost"
+ ) ).thenReturn( repository );
+
+ project.setVersion( "1.0-SNAPSHOT" );
+
+ ProjectDeployerRequest pdr =
+ new ProjectDeployerRequest()
+ .setProject( project )
+ .setAltDeploymentRepository( "altDeploymentRepository::legacy::http://localhost" );
Review comment:
I think this testcase is wrong because it should clearly say that only `default` layout is supported. You cannot simply drop the arg from the middle and assume that legacy layout will work. It should reject anything which is not `default` since `createDeploymentArtifactRepository()` uses `new DefaultRepositoryLayout()`.
----------------------------------------------------------------
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-deploy-plugin] michael-o commented on pull request #12: [MDEPLOY-279] Better validation for alt deploy repository mojo parameter
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #12:
URL: https://github.com/apache/maven-deploy-plugin/pull/12#issuecomment-743369558
OK, let me think about this. If we can make it even more idiotproof then we should do 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-deploy-plugin] michael-o commented on a change in pull request #12: [MDEPLOY-279] Better validation for alt deploy repository mojo parameter
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #12:
URL: https://github.com/apache/maven-deploy-plugin/pull/12#discussion_r541214981
##########
File path: src/main/java/org/apache/maven/plugins/deploy/DeployMojo.java
##########
@@ -248,19 +249,40 @@ else if ( !ArtifactUtils.isSnapshot( project.getVersion() ) && altReleaseDeploym
{
getLog().info( "Using alternate deployment repository " + altDeploymentRepo );
- Matcher matcher = ALT_REPO_SYNTAX_PATTERN.matcher( altDeploymentRepo );
+ Matcher matcher = ALT_LEGACY_REPO_SYNTAX_PATTERN.matcher( altDeploymentRepo );
- if ( !matcher.matches() )
+ if ( matcher.matches() )
{
- throw new MojoFailureException( altDeploymentRepo, "Invalid syntax for repository.",
- "Invalid syntax for alternative repository. Use \"id::url\"." );
+ getLog().warn( "Legacy form of alternate deployment parameter used, update parameter to match plugin version." );
Review comment:
I agree, the message itself is partially not helpful. Indicate what is required.
----------------------------------------------------------------
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-deploy-plugin] michael-o commented on a change in pull request #12: [MDEPLOY-279] Better validation for alt deploy repository mojo parameter
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #12:
URL: https://github.com/apache/maven-deploy-plugin/pull/12#discussion_r541561101
##########
File path: src/main/java/org/apache/maven/plugins/deploy/DeployMojo.java
##########
@@ -248,19 +249,40 @@ else if ( !ArtifactUtils.isSnapshot( project.getVersion() ) && altReleaseDeploym
{
getLog().info( "Using alternate deployment repository " + altDeploymentRepo );
- Matcher matcher = ALT_REPO_SYNTAX_PATTERN.matcher( altDeploymentRepo );
+ Matcher matcher = ALT_LEGACY_REPO_SYNTAX_PATTERN.matcher( altDeploymentRepo );
- if ( !matcher.matches() )
+ if ( matcher.matches() )
{
- throw new MojoFailureException( altDeploymentRepo, "Invalid syntax for repository.",
- "Invalid syntax for alternative repository. Use \"id::url\"." );
+ getLog().warn( "Legacy form of alternate deployment parameter used, update parameter to match plugin version." );
Review comment:
Very nice! As I have said it is all about greedy regex....
----------------------------------------------------------------
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