commands for resource adapter manipulation #146

ghost · 2016-11-15T09:48:48Z

No description provided.

Ladicek · 2016-11-15T11:13:14Z

commands/src/main/java/org/wildfly/extras/creaper/commands/ra/AddAdminObjectToRA.java

+        ops.add(address, Values.of("class-name", className)
+                .and("jndi-name", localJndiName).and("use-java-context", useJavaContext));
+        if (properties.size() > 0) {
+


Why the empty line? :-)

Ladicek · 2016-11-15T11:13:54Z

commands/src/main/java/org/wildfly/extras/creaper/commands/ra/AddAdminObjectToRA.java

+        if (properties.size() > 0) {
+
+            Iterator it = properties.entrySet().iterator();
+            while (it.hasNext()) {


Why not for (Map.Entry<String, String> entry : properties.entrySet()) { ... }?

Ladicek · 2016-11-15T11:14:07Z

commands/src/main/java/org/wildfly/extras/creaper/commands/ra/AddAdminObjectToRA.java

+            }
+        }
+
+


Empty lines...

Ladicek · 2016-11-15T11:14:17Z

commands/src/main/java/org/wildfly/extras/creaper/commands/ra/AddAdminObjectToRA.java

+/**
+ * Adds admin object to existing resource adapter
+ */
+public class AddAdminObjectToRA implements OnlineCommand {


Should probably be final.

Ladicek · 2016-11-15T11:15:03Z

commands/src/main/java/org/wildfly/extras/creaper/commands/ra/AddAdminObjectToRA.java

+        private Map<String, String> properties = new HashMap<String, String>();
+
+        public Builder(String poolName, String localJndiName, String className, boolean useJavaContext,
+                       String resourceAdapterId) {


Looks like a lot of parameters. What about using builder methods for them, or most of them? Presence/absence of mandatory values can be validates in the build method.

Is it necessary? I'd like to keep some consistency in relation to the other RA commands.

Consistency is good, but sanity is better. You have 5 parameters, 4 of them are strings. Invocation of such a constructor is basically unreadable. If you feel strongly about consistency with other RA commands, feel free to use parameter-less constructors for all their builders :-)

Ladicek · 2016-11-15T11:27:02Z

...alone/src/test/java/org/wildfly/extras/creaper/commands/ra/AddResourceAdapterOnlineTest.java

+    private OnlineManagementClient client;
+    private Operations ops;
+    private Administration admin;
+    private static final String RA_ID = "genericjmsRA";


Order of fields.

Ladicek · 2016-11-15T11:27:13Z

...alone/src/test/java/org/wildfly/extras/creaper/commands/ra/AddResourceAdapterOnlineTest.java

+        ModelNodeResult res = ops.readResource(Address.subsystem("resource-adapters").and("resource-adapter", RA_ID));
+        res.assertSuccess();
+    }
+}


Negative test case?

Ladicek · 2016-11-15T11:27:21Z

.../src/test/java/org/wildfly/extras/creaper/commands/ra/RemoveAdminObjectFromRAOnlineTest.java

+    private OnlineManagementClient client;
+    private Operations ops;
+    private Administration admin;
+    private static final String RA_ID = "genericjmsRA";


Order of fields.

Ladicek · 2016-11-15T11:27:45Z

...est/java/org/wildfly/extras/creaper/commands/ra/RemoveConnectionFactoryFromRAOnlineTest.java

+    private OnlineManagementClient client;
+    private Operations ops;
+    private Administration admin;
+    private static final String RA_ID = "genericjmsRA";


Order of fields.

Ladicek · 2016-11-15T11:28:08Z

...ne/src/test/java/org/wildfly/extras/creaper/commands/ra/RemoveResourceAdapterOnlineTest.java

+    private OnlineManagementClient client;
+    private Operations ops;
+    private Administration admin;
+    private static final String RA_ID = "genericjmsRA";


Order of fields.

Ladicek · 2016-11-15T15:02:19Z

I will review more tomorrow, but I'd like to point out that while I want command classes as much immutable as possible, builders don't have to be. That's one of the big points of using them -- mutate as you wish while you're building the command, but once you build it, there's no going back.

Ladicek

OK, I think this is almost done :-)

Ladicek · 2016-11-16T08:51:23Z

commands/src/main/java/org/wildfly/extras/creaper/commands/ra/AddConnectionFactoryToRA.java

+                    .and("resource-adapter", resourceAdapterId)
+                    .and("connection-definitions", poolName).and("config-properties", pair.getKey());
+            ops.add(configPropsAddress, Values.of("value", pair.getValue()));
+        }


Did you consider creating a Batch and then performing all ops in a single "transaction"? It's not strictly required, but very nice to have :-)

Tbh I didn't.

I won't force you to do it, but it would be nice :-)

Ladicek · 2016-11-16T08:51:46Z

commands/src/main/java/org/wildfly/extras/creaper/commands/ra/AddResourceAdapter.java

+ * Adds new resource adapter
+ */
+public final class AddResourceAdapter implements OnlineCommand {
+    private final  String id;


Two spaces between final and String.

Ladicek · 2016-11-16T08:52:10Z

commands/src/main/java/org/wildfly/extras/creaper/commands/ra/AddResourceAdapter.java

+
+        public AddResourceAdapter build() {
+
+


Empty lines.

Ladicek · 2016-11-16T08:53:10Z

commands/src/main/java/org/wildfly/extras/creaper/commands/ra/RemoveResourceAdapter.java

+
+    /**
+     * @param resourceAdapterId name of resource adapter
+     */


Most methods in this PR don't have a Javadoc. I don't think this one needs to have it, especially if it really doesn't say anything.

I usually have some javadoc on builder, so I used the same hare...

It's not a problem, feel free to keep it. But frankly, this kind of javadoc helps noone -- it merely restates what is obvious from the code.

Ladicek · 2016-11-16T08:53:59Z

...e/src/test/java/org/wildfly/extras/creaper/commands/ra/AddAdminObjectToRAOnlineTestCase.java

+        res.assertSuccess();
+    }
+
+    @Test(expected = org.wildfly.extras.creaper.core.CommandFailedException.class)


Import that class?

Ladicek · 2016-11-16T08:54:16Z

...src/test/java/org/wildfly/extras/creaper/commands/ra/AddConnectionFactoryToRAOnlineTest.java

+        res.assertSuccess();
+    }
+
+    @Test(expected = org.wildfly.extras.creaper.core.CommandFailedException.class)


Import that class?

Ladicek · 2016-11-16T08:54:31Z

...alone/src/test/java/org/wildfly/extras/creaper/commands/ra/AddResourceAdapterOnlineTest.java

+        res.assertSuccess();
+    }
+
+    @Test(expected = org.wildfly.extras.creaper.core.CommandFailedException.class)


Import that class?

Ladicek · 2016-11-16T08:55:02Z

...alone/src/test/java/org/wildfly/extras/creaper/commands/ra/AddResourceAdapterOnlineTest.java

+    @Test(expected = org.wildfly.extras.creaper.core.CommandFailedException.class)
+    public void addNewRaNegativeTestCase() throws Exception {
+        client.apply(new AddResourceAdapter.Builder(RA_ID, "org.jboss.nonexsitingmodule", TransactionType.XA).build());
+        admin.reloadIfRequired();


Is this needed? The exception will be thrown on the first line, no?

Ladicek

OK, I've looked at how you're using builders and I think maybe we should move that one constructor back to a trivial builder. There's too many builders that only have constructor and no additional method, and consistency would be good here.

Other than that, one single move to Batch and it's done :-)

Ladicek · 2016-11-16T12:04:30Z

commands/src/main/java/org/wildfly/extras/creaper/commands/ra/AddAdminObjectToRA.java

+            Address configPropsAddress = Address.subsystem("resource-adapters")
+                    .and("resource-adapter", resourceAdapterId)
+                    .and("admin-objects", poolName).and("config-properties", pair.getKey());
+            ops.add(configPropsAddress, Values.of("value", pair.getValue()));


This could also use Batch.

Ladicek

Please squash and I'll merge.

mchoma · 2016-11-16T12:54:47Z

@okalmanRH, don't believe him, I heard that 2 days ago :P

Ladicek · 2016-11-16T13:00:01Z

@mchoma is right, you shouldn't believe me. I won't merge yet, because your tests are failing on EAP 6 :-)

ghost · 2016-11-16T13:18:22Z

Is there any action needed from me because of it?

Ladicek · 2016-11-16T13:24:43Z

Yes? :-)

Ladicek · 2016-11-16T13:26:33Z

OK, I'll try to be more specific: are the commands supposed to work with EAP 6? (I don't see why not.) If yes, the tests should pass. If not, it should be documented, the commands should fail early (using something like OnlineCommandContext.version.assertAtLeast(ServerVersion.XXX)), and the tests should be in a correct category.

ghost · 2016-11-16T13:36:21Z

maybe dumb question, but how do I run the testsuite against EAP6 or its WildFly equivalent?
EDIT: don't bother to answer... it was really dumb question

Ladicek · 2016-11-16T13:42:30Z

NP, it's a good question. See https://github.com/wildfly-extras/creaper/blob/master/CONTRIBUTING.md

ghost · 2016-11-22T09:41:11Z

So the issue comes from org.jboss.genericjms module in EAP6. Because it requires another module with jms jars which needs to be present. In EAP7 this module is marked in module.xml as optional so everything works fine.

Ladicek · 2016-11-22T09:48:32Z

What would that mean for end users trying to use these commands?

ghost · 2016-11-22T10:00:09Z

It depends on use case.
If they want to use generic-ra they have to create new module with jms jars.
If they want to use other ra, like WebSphere, they have to deploy this RA to EAP as deployment.
Our issue is that for test cases I'm using generic RA (as it is probably the only RA which is out of the box available)...

Ladicek · 2016-11-22T10:08:34Z

OK, so is there a simple way to adjust the tests for EAP 6 so that they pass? Or would that mean that the test would have to download a JAR and whatnot, in which case I'd prefer to just skip them on EAP 6?

ghost · 2016-11-22T10:38:10Z

We can try to modify module.xml and add optional="true", but I don't know whether it's suitable solution for you.

Ladicek · 2016-11-22T10:53:50Z

I'm not a fan. I'd rather skip the test, with a comment. Something like

// doesn't work on AS7/EAP6 because ...
assumeThat(client.version().greaterThanOrEqualTo(...));

Ladicek · 2016-11-22T11:21:26Z

...e/src/test/java/org/wildfly/extras/creaper/commands/ra/AddAdminObjectToRAOnlineTestCase.java

@@ -26,6 +29,7 @@
    @Before
    public void connect() throws IOException {
        client = ManagementClient.online(OnlineOptions.standalone().localDefault().build());
+        Assume.assumeTrue(client.version().greaterThanOrEqualTo(ServerVersion.VERSION_2_1_0));


Did the change really happen in WildFly 8.1.0?

I think, that first builds of EAP7 were based on WF8, or not?

Hm.... It seems that WF8.1 doesn't contain the fix... I'll investigate this further

No, they were not :-) EAP 7.0 (loosely) corresponds to WildFly 10.

WF9 contains the fix, so I've set version to 3.0.0

Agreed, the tests pass in the wildfly9 profile.

Ladicek · 2016-11-22T12:01:38Z

OK, could you please squash?

Ladicek requested changes Nov 15, 2016

View reviewed changes

Ladicek requested changes Nov 16, 2016

View reviewed changes

Ladicek approved these changes Nov 16, 2016

View reviewed changes

Ladicek self-assigned this Nov 16, 2016

Ladicek added this to the 1.6.0 milestone Nov 16, 2016

Ladicek reviewed Nov 22, 2016

View reviewed changes

Ladicek approved these changes Nov 22, 2016

View reviewed changes

commands for resource adapter manipulation

b6cf1df

Ladicek merged commit eaa92db into wildfly-extras:master Nov 22, 2016

ghost deleted the ra-config branch November 22, 2016 14:27

commands for resource adapter manipulation #146

commands for resource adapter manipulation #146

Conversation

ghost commented Nov 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ladicek commented Nov 15, 2016

Ladicek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ladicek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ladicek left a comment

Choose a reason for hiding this comment

mchoma commented Nov 16, 2016

Ladicek commented Nov 16, 2016

ghost commented Nov 16, 2016

Ladicek commented Nov 16, 2016

Ladicek commented Nov 16, 2016

ghost commented Nov 16, 2016 • edited by ghost Loading

Ladicek commented Nov 16, 2016

ghost commented Nov 22, 2016

Ladicek commented Nov 22, 2016

ghost commented Nov 22, 2016

Ladicek commented Nov 22, 2016

ghost commented Nov 22, 2016

Ladicek commented Nov 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ladicek commented Nov 22, 2016

ghost commented Nov 16, 2016 •

edited by ghost

Loading