Copilot commented on code in PR #3: URL: https://github.com/apache/maven-deploy-plugin/pull/3#discussion_r3676788516
########## src/main/java/org/apache/maven/plugins/deploy/AttachedArtifact.java: ########## @@ -0,0 +1,195 @@ +package org.apache.maven.plugins.deploy; + +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import org.apache.maven.artifact.Artifact; +import org.apache.maven.plugin.MojoExecutionException; +import org.apache.maven.plugins.annotations.Mojo; +import org.apache.maven.plugins.annotations.Parameter; +import org.apache.maven.shared.utils.StringUtils; + +import java.util.List; + +/** + * The attached artifact data + * + * @author <a href="mailto:[email protected]";>Gregory Callea</a> + * + */ +@Mojo( name = "artifact" ) +public class AttachedArtifact Review Comment: AttachedArtifact is a configuration/data object, but it is annotated as a @Mojo. This can unintentionally register a new plugin goal named \"artifact\" (and pull in mojo metadata), which is a functional/packaging change for the plugin. Remove the `@Mojo` annotation (and its import) so this class is treated purely as a configuration POJO. ########## src/main/java/org/apache/maven/plugins/deploy/AttachedArtifact.java: ########## @@ -0,0 +1,195 @@ +package org.apache.maven.plugins.deploy; + +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import org.apache.maven.artifact.Artifact; +import org.apache.maven.plugin.MojoExecutionException; +import org.apache.maven.plugins.annotations.Mojo; +import org.apache.maven.plugins.annotations.Parameter; +import org.apache.maven.shared.utils.StringUtils; + +import java.util.List; + +/** + * The attached artifact data + * + * @author <a href="mailto:[email protected]";>Gregory Callea</a> + * + */ +@Mojo( name = "artifact" ) +public class AttachedArtifact +{ + + /** + * GroupId of the attached artifact + */ + @Parameter( property = "groupId" ) + private String groupId; + + /** + * ArtifactId of the attached artifact + */ + @Parameter( property = "artifactId" ) + private String artifactId; + + /** + * Version of the attached artifact + */ + @Parameter( property = "version" ) + private String version; Review Comment: Using `@Parameter(property = ...)` on fields inside a nested configuration object makes them eligible for global user-property injection (e.g., `-DgroupId=...`) which can unexpectedly override these values across builds. For nested config beans, prefer `@Parameter` without `property` so values are sourced from the `<artifact>...</artifact>` elements only (and avoid collisions with common property names like `groupId`). ########## src/main/java/org/apache/maven/plugins/deploy/AttachedArtifact.java: ########## @@ -0,0 +1,195 @@ +package org.apache.maven.plugins.deploy; + +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import org.apache.maven.artifact.Artifact; +import org.apache.maven.plugin.MojoExecutionException; +import org.apache.maven.plugins.annotations.Mojo; +import org.apache.maven.plugins.annotations.Parameter; +import org.apache.maven.shared.utils.StringUtils; + +import java.util.List; + +/** + * The attached artifact data + * + * @author <a href="mailto:[email protected]";>Gregory Callea</a> + * + */ +@Mojo( name = "artifact" ) +public class AttachedArtifact +{ + + /** + * GroupId of the attached artifact + */ + @Parameter( property = "groupId" ) + private String groupId; Review Comment: Using `@Parameter(property = ...)` on fields inside a nested configuration object makes them eligible for global user-property injection (e.g., `-DgroupId=...`) which can unexpectedly override these values across builds. For nested config beans, prefer `@Parameter` without `property` so values are sourced from the `<artifact>...</artifact>` elements only (and avoid collisions with common property names like `groupId`). ########## src/main/java/org/apache/maven/plugins/deploy/AttachedArtifact.java: ########## @@ -0,0 +1,195 @@ +package org.apache.maven.plugins.deploy; + +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import org.apache.maven.artifact.Artifact; +import org.apache.maven.plugin.MojoExecutionException; +import org.apache.maven.plugins.annotations.Mojo; +import org.apache.maven.plugins.annotations.Parameter; +import org.apache.maven.shared.utils.StringUtils; + +import java.util.List; + +/** + * The attached artifact data + * + * @author <a href="mailto:[email protected]";>Gregory Callea</a> + * + */ +@Mojo( name = "artifact" ) +public class AttachedArtifact +{ + + /** + * GroupId of the attached artifact + */ + @Parameter( property = "groupId" ) + private String groupId; + + /** + * ArtifactId of the attached artifact + */ + @Parameter( property = "artifactId" ) + private String artifactId; + + /** + * Version of the attached artifact + */ + @Parameter( property = "version" ) + private String version; + + + /** + * Packaging of the attached artifact + */ + @Parameter( property = "packaging" ) + private String packaging; + + /** + * Classifier to the attached artifact + */ + @Parameter( property = "classifier" ) + private String classifier; Review Comment: Using `@Parameter(property = ...)` on fields inside a nested configuration object makes them eligible for global user-property injection (e.g., `-DgroupId=...`) which can unexpectedly override these values across builds. For nested config beans, prefer `@Parameter` without `property` so values are sourced from the `<artifact>...</artifact>` elements only (and avoid collisions with common property names like `groupId`). ########## src/main/java/org/apache/maven/plugins/deploy/AttachedArtifact.java: ########## @@ -0,0 +1,195 @@ +package org.apache.maven.plugins.deploy; + +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import org.apache.maven.artifact.Artifact; +import org.apache.maven.plugin.MojoExecutionException; +import org.apache.maven.plugins.annotations.Mojo; +import org.apache.maven.plugins.annotations.Parameter; +import org.apache.maven.shared.utils.StringUtils; + +import java.util.List; + +/** + * The attached artifact data + * + * @author <a href="mailto:[email protected]";>Gregory Callea</a> + * + */ +@Mojo( name = "artifact" ) +public class AttachedArtifact +{ + + /** + * GroupId of the attached artifact + */ + @Parameter( property = "groupId" ) + private String groupId; + + /** + * ArtifactId of the attached artifact + */ + @Parameter( property = "artifactId" ) + private String artifactId; + + /** + * Version of the attached artifact + */ + @Parameter( property = "version" ) + private String version; + + + /** + * Packaging of the attached artifact + */ + @Parameter( property = "packaging" ) + private String packaging; Review Comment: Using `@Parameter(property = ...)` on fields inside a nested configuration object makes them eligible for global user-property injection (e.g., `-DgroupId=...`) which can unexpectedly override these values across builds. For nested config beans, prefer `@Parameter` without `property` so values are sourced from the `<artifact>...</artifact>` elements only (and avoid collisions with common property names like `groupId`). ########## src/main/java/org/apache/maven/plugins/deploy/DeployMojo.java: ########## @@ -154,6 +163,19 @@ public void execute() ArtifactRepository repo = getDeploymentRepository( pdr ); + final List<Artifact> attachedArtifacts = pdr.getProject().getAttachedArtifacts(); + if ( skipAttachedArtifacts != null ) + { + for ( final AttachedArtifact attachedArtifactToSkip : skipAttachedArtifacts ) + { + final Artifact toSkip = attachedArtifactToSkip.checkIfExists( attachedArtifacts ); + attachedArtifacts.remove( toSkip ); + getLog().info( "Skipping artifact [" + + toSkip + + "]" ); + } + } Review Comment: This logic validates existence against the *mutating* `attachedArtifacts` list. If `skipAttachedArtifacts` contains duplicate entries (or two entries that resolve to the same attached artifact), the first removal succeeds and the second `checkIfExists(...)` will throw, even though the artifact existed originally. Consider de-duplicating `skipAttachedArtifacts` (by a computed key like `groupId:artifactId:type:classifier:version`) or validating against a snapshot/copy of the original attached artifacts and then removing matches in a separate pass. ########## src/test/java/org/apache/maven/plugins/deploy/DeployMojoTest.java: ########## @@ -522,7 +522,133 @@ public void testDeployWithAttachedArtifacts() assertEquals( 0, getSizeOfExpectedFiles( fileList, expectedFiles ) ); } - + + public void testDeployWithNotExistingAttachedArtifactsExcluded() + throws Exception { Review Comment: Brace placement/style here differs from the surrounding tests in this file (most methods place `{` on the next line and keep spacing consistent). Align this method signature formatting with the existing conventions in DeployMojoTest to keep the test suite consistent and easier to scan. ########## src/main/java/org/apache/maven/plugins/deploy/AttachedArtifact.java: ########## @@ -0,0 +1,195 @@ +package org.apache.maven.plugins.deploy; + +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import org.apache.maven.artifact.Artifact; +import org.apache.maven.plugin.MojoExecutionException; +import org.apache.maven.plugins.annotations.Mojo; +import org.apache.maven.plugins.annotations.Parameter; +import org.apache.maven.shared.utils.StringUtils; + +import java.util.List; + +/** + * The attached artifact data + * + * @author <a href="mailto:[email protected]";>Gregory Callea</a> + * + */ +@Mojo( name = "artifact" ) +public class AttachedArtifact +{ + + /** + * GroupId of the attached artifact + */ + @Parameter( property = "groupId" ) + private String groupId; + + /** + * ArtifactId of the attached artifact + */ + @Parameter( property = "artifactId" ) + private String artifactId; Review Comment: Using `@Parameter(property = ...)` on fields inside a nested configuration object makes them eligible for global user-property injection (e.g., `-DgroupId=...`) which can unexpectedly override these values across builds. For nested config beans, prefer `@Parameter` without `property` so values are sourced from the `<artifact>...</artifact>` elements only (and avoid collisions with common property names like `groupId`). ########## src/site/fml/faq.fml: ########## @@ -77,6 +77,37 @@ under the License. </plugin>]]></source> </answer> </faq> + <faq id="skipAttachedArtifacts"> + <question>I don't want to deploy some attached artifact on a specific module. Can I skip deployment for specific attached artifacts?</question> Review Comment: There is a double space after the period, and the wording is a bit grammatically inconsistent (\"some attached artifact\" vs \"attached artifacts\"). Consider adjusting to a single space and consistent plurality for readability. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
