Improvements from project usage #33

Closed
wants to merge 6 commits into
from

Projects

None yet

2 participants

@mp911de
Contributor
mp911de commented Jan 31, 2013

Hey, I've improved the plugin in following aspects:

  • Generic artifact deployment: You can deploy now artifacts via their maven coordinates. Its no longer required that those artifacts must be a direct pom dependency. Therefore also removed the package lifecycle binding
  • Improved exception handling/logging: Deployment error messages are shown when deploying
  • Conditional execution: Execution of commands may fail in case of missing components on the JBoss AS. You can specify a model address for executing or not execution a certain command (e.g. execute this command only in case a certain subsystem exists)
  • Redeploy using regex for finding the old artifact: You can redeploy a artifact which has a different name. Therefore you can specify a regex (e.g. artifactId.*). So you can overwrite the old version of an artifact with a new one.
@jamezp
Member
jamezp commented Feb 11, 2013

We require that all contributions be made under the terms of the MIT License, http://www.opensource.org/licenses/mit-license.php. Do you agree to the these terms?

@mp911de
Contributor
mp911de commented Feb 11, 2013

Yes, also already signed Contributor License Agreement on Jan, 2011.

@mp911de
Contributor
mp911de commented Feb 11, 2013

Can you also point me to the code formatting settings?

@jamezp
Member
jamezp commented Feb 11, 2013

Sure. I'm just using the same one as we do on JBoss AS 7 https://community.jboss.org/wiki/HackingOnAS7.

@jamezp
Member
jamezp commented Feb 11, 2013

BTW this might have a merge conflict. I'll try to sort it out, but if I can't I might have to ask you to give me a hand rebasing it. I'll let you know for sure though here shortly.

Also, I'm probably going to squash the commits down into one commit.

@jamezp
Member
jamezp commented Feb 11, 2013

I'm getting an NPE if I don't define a name or a replacement pattern.

Could you explain a bit more on the use case of the replacement pattern stuff? I'm not sure how I feel about it yet.

@jamezp jamezp commented on the diff Feb 11, 2013
.../as/plugin/deployment/AbstractArtifactDeployment.java
+ /**
+ *
+ */
+ @Component
+ private ArtifactResolver artifactResolver;
+
+ private Artifact artifact;
+
+ /**
+ * The resolved dependency file
+ */
+ @Parameter
+ private File file;
+
+ @Override
+ public void validate() throws DeploymentFailureException, MojoExecutionException, MojoFailureException {
@jamezp
jamezp Feb 11, 2013 Member

I'd prefer this doesn't throw any MOJO exceptions.

@mp911de
mp911de Feb 12, 2013 Contributor

K, it's also not needed anyway. I'll adjust the code.

@jamezp jamezp commented on the diff Feb 11, 2013
...rg/jboss/as/plugin/deployment/AbstractDeployment.java
@@ -163,7 +171,7 @@ protected void doExecute() throws MojoExecutionException, MojoFailureException {
*
* @throws DeploymentFailureException if the deployment is invalid.
*/
- protected void validate() throws DeploymentFailureException {
+ protected void validate() throws DeploymentFailureException, MojoExecutionException, MojoFailureException {
@jamezp
jamezp Feb 11, 2013 Member

Again I'd prefer here no MOJO exceptions to be thrown.

@mp911de
mp911de Feb 12, 2013 Contributor

K, it's also not needed anyway. I'll adjust the code.

@jamezp jamezp commented on the diff Feb 11, 2013
.../org/jboss/as/plugin/deployment/UndeployArtifact.java
@@ -26,12 +26,7 @@
import java.util.Set;
import org.apache.maven.artifact.Artifact;
-import org.apache.maven.plugins.annotations.Execute;
-import org.apache.maven.plugins.annotations.LifecyclePhase;
-import org.apache.maven.plugins.annotations.Mojo;
-import org.apache.maven.plugins.annotations.Parameter;
-import org.apache.maven.plugins.annotations.ResolutionScope;
-import org.apache.maven.project.MavenProject;
+import org.apache.maven.plugins.annotations.*;
@jamezp
jamezp Feb 11, 2013 Member

Please don't use wildcard imports.

@mp911de
mp911de Feb 12, 2013 Contributor

OK, I'll adjust the code.

@jamezp jamezp commented on the diff Feb 11, 2013
...ain/java/org/jboss/as/plugin/cli/ExecuteCommands.java
@@ -57,6 +57,12 @@
@Parameter(alias = "execute-commands", required = true)
private Commands execCommands;
+ /**
+ * Fail Mojo if command fails.
+ */
+ @Parameter(alias = "ignore-failure")
@jamezp
jamezp Feb 11, 2013 Member

This should probably have a property name too.

@jamezp jamezp commented on the diff Feb 11, 2013
.../jboss/as/plugin/common/AbstractServerConnection.java
+ protected ModelNode parseAddress(final String profileName, final String inputAddress) {
+
+ final ModelNode result = new ModelNode();
+ if (profileName != null) {
+ result.add(Operations.PROFILE, profileName);
+ }
+ String[] parts = inputAddress.split(",");
+ for (String part : parts) {
+ String[] address = part.split("=");
+ if (address.length != 2) {
+ throw new RuntimeException(part + " is not a valid address segment");
+ }
+ result.add(address[0], address[1]);
+ }
+ return result;
+ }
@jamezp
jamezp Feb 11, 2013 Member

Not if there is a better place for the above methods, but I'd prefer they're not in the AbstractServerConnection.

@mp911de
mp911de Feb 12, 2013 Contributor

I'll make up my mind about that.

@jamezp jamezp commented on the diff Feb 11, 2013
src/main/java/org/jboss/as/plugin/common/Operations.java
*/
public static ModelNode createOperation(final String operation, final ModelNode address) {
- if (address.getType() != ModelType.LIST) {
- throw new IllegalArgumentException("The address type must be a list.");
- }
@jamezp
jamezp Feb 11, 2013 Member

Why are we not throwing an exception anymore? An address is needed for an operation.

@mp911de
mp911de Feb 12, 2013 Contributor

Actually, this check prevents adding nodes at the root level (e.g. adding system properties by resource) . In case the address is empty, the operation is based on the root address.

@jamezp jamezp commented on the diff Feb 11, 2013
...oss/as/plugin/deployment/domain/DomainDeployment.java
@@ -216,8 +234,8 @@ private boolean exists() {
if (Operations.successful(result)) {
final List<ModelNode> deployments = (result.hasDefined(Operations.RESULT) ? result.get(Operations.RESULT).asList() : Collections.<ModelNode>emptyList());
for (ModelNode n : deployments) {
- if (n.asString().equals(deploymentName)) {
- return true;
+ if (n.asString().matches(deploymentNamePattern)) {
@jamezp
jamezp Feb 11, 2013 Member

This is where the NPE is thrown because of the null deploymentNamePattern.

@mp911de
mp911de Feb 12, 2013 Contributor

Right, I've got to fix this.

@jamezp jamezp commented on the diff Feb 11, 2013
...lugin/deployment/standalone/StandaloneDeployment.java
@@ -169,8 +202,8 @@ private boolean exists() {
if (Operations.successful(result)) {
final List<ModelNode> deployments = (result.hasDefined(Operations.RESULT) ? result.get(Operations.RESULT).asList() : Collections.<ModelNode>emptyList());
for (ModelNode n : deployments) {
- if (n.asString().equals(deploymentName)) {
- return true;
+ if (n.asString().matches(deploymentNamePattern)) {
@jamezp
jamezp Feb 11, 2013 Member

This is where the NPE is thrown because of the null deploymentNamePattern.

@jamezp jamezp commented on the diff Feb 11, 2013
...lugin/deployment/standalone/StandaloneDeployment.java
@@ -179,6 +212,7 @@ private boolean exists() {
} catch (IOException e) {
throw new IllegalStateException(String.format("Could not execute operation '%s'", op), e);
}
- return false;
+ return null;
@jamezp
jamezp Feb 11, 2013 Member

Not sure how I feel about this returning null. I'd have to review the JavaDocs for the deployment plan builder.

@mp911de
mp911de Feb 12, 2013 Contributor

My construction is not failsafe. I got to change this. The deployment plan builder has either to get the existing deployment name which is found by the pattern/exact name or, in case there is no deployment, the old way, as it was before.

@jamezp
Member
jamezp commented Feb 11, 2013

This PR also needs to be rebased with the latest upstream master. If you don't have the time to handle it, just let me know and I can take care of it.

@mp911de
Contributor
mp911de commented Feb 12, 2013

I'll prefer to close/reject the PR. I'll rebase, fix the findings and apply the formatting. After that I would create another PR.

@mp911de mp911de closed this Feb 12, 2013
@jamezp
Member
jamezp commented Feb 12, 2013

Thanks for everything on this. Really appreciate it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment