You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@taverna.apache.org by haouech <gi...@git.apache.org> on 2018/07/28 12:29:34 UTC
[GitHub] incubator-taverna-language pull request #41: Add helper method to create nes...
GitHub user haouech opened a pull request:
https://github.com/apache/incubator-taverna-language/pull/41
Add helper method to create nesting relationship between workflows
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/haouech/incubator-taverna-language nested
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-taverna-language/pull/41.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #41
----
commit edb12c6f7715de1ff78999a130b4038efb73cd9d
Author: Majdi Haouech <m....@...>
Date: 2018-07-18T21:39:21Z
API: Add a tool to nest a workflow in a process
commit d93452d7ab30dc36bf0cabafe923ff3707bb0c55
Author: Majdi Haouech <m....@...>
Date: 2018-07-28T12:28:08Z
Add test for nested helper in Scufl2Tools
----
---
[GitHub] incubator-taverna-language pull request #41: Add helper method to create nes...
Posted by stain <gi...@git.apache.org>.
Github user stain commented on a diff in the pull request:
https://github.com/apache/incubator-taverna-language/pull/41#discussion_r208521198
--- Diff: taverna-scufl2-api/src/test/java/org/apache/taverna/scufl2/api/common/TestScufl2Tools.java ---
@@ -63,6 +64,25 @@ public void makeBundle() {
makeWorkflowBundle();
assertNotNull(workflowBundle);
}
+
+ @Test
+ public void testNestedWorkflows() {
+ Workflow child = new Workflow();
+ child.setName("childWorkflow");
+ child.setParent(workflowBundle);
+
+ Workflow mainWorkflow = workflowBundle.getMainWorkflow();
+ Processor processor = new Processor();
+ processor.setParent(mainWorkflow);
+
+ Profile profile = workflowBundle.getMainProfile();
+
+ Scufl2Tools tools = new Scufl2Tools();
+ tools.createNestedRelationship(processor, child, profile);
+ Workflow nested = tools.nestedWorkflowForProcessor(processor, profile);
+
+ assertEquals(child, nested);
--- End diff --
Great!
---
[GitHub] incubator-taverna-language pull request #41: Add helper method to create nes...
Posted by stain <gi...@git.apache.org>.
Github user stain commented on a diff in the pull request:
https://github.com/apache/incubator-taverna-language/pull/41#discussion_r208521372
--- Diff: taverna-scufl2-api/src/test/java/org/apache/taverna/scufl2/api/common/TestScufl2Tools.java ---
@@ -81,19 +82,13 @@ public void testNestedWorkflows() {
assertEquals(child, nested);
- boolean found = false;
+ ProcessorBinding binding = processor.getBinding(profile);
+ Activity activity = binding.getBoundActivity();
+ Configuration configuration = activity.getConfiguration();
--- End diff --
Except for inconsistent indentation this looks much better!
---
[GitHub] incubator-taverna-language pull request #41: Add helper method to create nes...
Posted by stain <gi...@git.apache.org>.
Github user stain commented on a diff in the pull request:
https://github.com/apache/incubator-taverna-language/pull/41#discussion_r207915625
--- Diff: taverna-scufl2-api/src/main/java/org/apache/taverna/scufl2/api/common/Scufl2Tools.java ---
@@ -752,6 +753,30 @@ public Activity createActivityFromProcessor(Processor processor,
return activity;
}
+ public Configuration createNestedRelationship(Processor processor, Workflow childWorkflow, Profile profile) {
+ if(processor.getParent() == null) {
+ throw new IllegalStateException("Processor " + processor + " has no parent");
+ }
+ if(processor.getParent().getParent() != childWorkflow.getParent()) {
+ throw new IllegalStateException(
+ "Processor " + processor + " and workflow " + childWorkflow + " are not in the same Workflow bundle");
+ }
+ if(nestedWorkflowForProcessor(processor, profile) != null) {
--- End diff --
These are good guards, but they should also be verified by separate `@Test`s.
`nestedWorkflowForProcessor()` will detect if it's already a nested workflow, but not if there's another activity on the processor. Although SCUFL2 allows multiple activities on the same processor (e.g. for `Failover`) this has in practice not been used, so perhaps this test can check instead for any existing activity on the processor, regardless of activity type?
---
[GitHub] incubator-taverna-language pull request #41: Add helper method to create nes...
Posted by stain <gi...@git.apache.org>.
Github user stain commented on a diff in the pull request:
https://github.com/apache/incubator-taverna-language/pull/41#discussion_r208319161
--- Diff: taverna-scufl2-api/src/test/java/org/apache/taverna/scufl2/api/common/TestScufl2Tools.java ---
@@ -63,6 +64,25 @@ public void makeBundle() {
makeWorkflowBundle();
assertNotNull(workflowBundle);
}
+
+ @Test
+ public void testNestedWorkflows() {
+ Workflow child = new Workflow();
+ child.setName("childWorkflow");
+ child.setParent(workflowBundle);
+
+ Workflow mainWorkflow = workflowBundle.getMainWorkflow();
+ Processor processor = new Processor();
+ processor.setParent(mainWorkflow);
+
+ Profile profile = workflowBundle.getMainProfile();
+
+ Scufl2Tools tools = new Scufl2Tools();
+ tools.createNestedRelationship(processor, child, profile);
+ Workflow nested = tools.nestedWorkflowForProcessor(processor, profile);
+
+ assertEquals(child, nested);
--- End diff --
Yeah, safely you should pick the correct activity based on the `ProcessorBinding` rather than picking blindly the first one with `NESTED_WORKFLOW` in it - although that will be the case for this particular test case profile as there should be no other activites.
So perhaps improve it to look up the ProcessorBinding for the processor and from there find the activity and its configuration? Sorry it's a bit cumbersome..
---
[GitHub] incubator-taverna-language pull request #41: Add helper method to create nes...
Posted by stain <gi...@git.apache.org>.
Github user stain commented on a diff in the pull request:
https://github.com/apache/incubator-taverna-language/pull/41#discussion_r207896106
--- Diff: taverna-scufl2-api/src/main/java/org/apache/taverna/scufl2/api/common/Scufl2Tools.java ---
@@ -752,6 +753,30 @@ public Activity createActivityFromProcessor(Processor processor,
return activity;
}
+ public Configuration createNestedRelationship(Processor processor, Workflow childWorkflow, Profile profile) {
--- End diff --
Unsure about _relationship_ here.. Should we call this method `setAsNestedWorkflow()` or something?
---
[GitHub] incubator-taverna-language pull request #41: Add helper method to create nes...
Posted by haouech <gi...@git.apache.org>.
Github user haouech commented on a diff in the pull request:
https://github.com/apache/incubator-taverna-language/pull/41#discussion_r208042948
--- Diff: taverna-scufl2-api/src/test/java/org/apache/taverna/scufl2/api/common/TestScufl2Tools.java ---
@@ -63,6 +64,25 @@ public void makeBundle() {
makeWorkflowBundle();
assertNotNull(workflowBundle);
}
+
+ @Test
+ public void testNestedWorkflows() {
+ Workflow child = new Workflow();
+ child.setName("childWorkflow");
+ child.setParent(workflowBundle);
+
+ Workflow mainWorkflow = workflowBundle.getMainWorkflow();
+ Processor processor = new Processor();
+ processor.setParent(mainWorkflow);
+
+ Profile profile = workflowBundle.getMainProfile();
+
+ Scufl2Tools tools = new Scufl2Tools();
+ tools.createNestedRelationship(processor, child, profile);
+ Workflow nested = tools.nestedWorkflowForProcessor(processor, profile);
+
+ assertEquals(child, nested);
--- End diff --
I tried to add a patch where I check that the activity and configuration has been added to the workflow. But I'm not sure if it's what you asked for. Correct me please if you have something else in mind.
---
[GitHub] incubator-taverna-language pull request #41: Add helper method to create nes...
Posted by haouech <gi...@git.apache.org>.
Github user haouech commented on a diff in the pull request:
https://github.com/apache/incubator-taverna-language/pull/41#discussion_r208392499
--- Diff: taverna-scufl2-api/src/main/java/org/apache/taverna/scufl2/api/common/Scufl2Tools.java ---
@@ -764,6 +764,12 @@ public Configuration createNestedRelationship(Processor processor, Workflow chil
if(nestedWorkflowForProcessor(processor, profile) != null) {
throw new IllegalStateException("Processor " + processor + " already has a nested workflow");
}
+ try {
--- End diff --
I found a better way to test if a processor already has a bound activity. You were right about the exception catching logic. Thanks
---
[GitHub] incubator-taverna-language issue #41: Add helper method to create nesting re...
Posted by haouech <gi...@git.apache.org>.
Github user haouech commented on the issue:
https://github.com/apache/incubator-taverna-language/pull/41
Thank you so much for you comments! I replied to them and added a patch.
---
[GitHub] incubator-taverna-language pull request #41: Add helper method to create nes...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-taverna-language/pull/41
---
[GitHub] incubator-taverna-language pull request #41: Add helper method to create nes...
Posted by haouech <gi...@git.apache.org>.
Github user haouech commented on a diff in the pull request:
https://github.com/apache/incubator-taverna-language/pull/41#discussion_r208043243
--- Diff: taverna-scufl2-api/src/main/java/org/apache/taverna/scufl2/api/common/Scufl2Tools.java ---
@@ -752,6 +753,30 @@ public Activity createActivityFromProcessor(Processor processor,
return activity;
}
+ public Configuration createNestedRelationship(Processor processor, Workflow childWorkflow, Profile profile) {
+ if(processor.getParent() == null) {
+ throw new IllegalStateException("Processor " + processor + " has no parent");
+ }
+ if(processor.getParent().getParent() != childWorkflow.getParent()) {
+ throw new IllegalStateException(
+ "Processor " + processor + " and workflow " + childWorkflow + " are not in the same Workflow bundle");
+ }
+ if(nestedWorkflowForProcessor(processor, profile) != null) {
--- End diff --
I see your point. I took advantage of the already thrown exceptions by the 'getActivity' method of Processor. Now we make sure that the processor has no activity bound to it before nesting the workflows.
---
[GitHub] incubator-taverna-language pull request #41: Add helper method to create nes...
Posted by haouech <gi...@git.apache.org>.
Github user haouech commented on a diff in the pull request:
https://github.com/apache/incubator-taverna-language/pull/41#discussion_r208392734
--- Diff: taverna-scufl2-api/src/test/java/org/apache/taverna/scufl2/api/common/TestScufl2Tools.java ---
@@ -63,6 +64,25 @@ public void makeBundle() {
makeWorkflowBundle();
assertNotNull(workflowBundle);
}
+
+ @Test
+ public void testNestedWorkflows() {
+ Workflow child = new Workflow();
+ child.setName("childWorkflow");
+ child.setParent(workflowBundle);
+
+ Workflow mainWorkflow = workflowBundle.getMainWorkflow();
+ Processor processor = new Processor();
+ processor.setParent(mainWorkflow);
+
+ Profile profile = workflowBundle.getMainProfile();
+
+ Scufl2Tools tools = new Scufl2Tools();
+ tools.createNestedRelationship(processor, child, profile);
+ Workflow nested = tools.nestedWorkflowForProcessor(processor, profile);
+
+ assertEquals(child, nested);
--- End diff --
I wasn't happy with that way either. I used the ProcessorBinding as you suggested and it's much better. Thank you for your comments !
---
[GitHub] incubator-taverna-language pull request #41: Add helper method to create nes...
Posted by stain <gi...@git.apache.org>.
Github user stain commented on a diff in the pull request:
https://github.com/apache/incubator-taverna-language/pull/41#discussion_r208521155
--- Diff: taverna-scufl2-api/src/test/java/org/apache/taverna/scufl2/api/common/TestScufl2Tools.java ---
@@ -51,6 +51,7 @@
import org.apache.taverna.scufl2.api.profiles.Profile;
import org.junit.Before;
import org.junit.Test;
+import sun.security.krb5.Config;
--- End diff --
I don't think we need `sun.security.krb5.Config` (or that it's even legal to import)
---
[GitHub] incubator-taverna-language pull request #41: Add helper method to create nes...
Posted by stain <gi...@git.apache.org>.
Github user stain commented on a diff in the pull request:
https://github.com/apache/incubator-taverna-language/pull/41#discussion_r208318505
--- Diff: taverna-scufl2-api/src/main/java/org/apache/taverna/scufl2/api/common/Scufl2Tools.java ---
@@ -764,6 +764,12 @@ public Configuration createNestedRelationship(Processor processor, Workflow chil
if(nestedWorkflowForProcessor(processor, profile) != null) {
throw new IllegalStateException("Processor " + processor + " already has a nested workflow");
}
+ try {
--- End diff --
Uh, I see what you mean.. it's not good to have logic by exception catching, is it..
---
[GitHub] incubator-taverna-language pull request #41: Add helper method to create nes...
Posted by haouech <gi...@git.apache.org>.
Github user haouech commented on a diff in the pull request:
https://github.com/apache/incubator-taverna-language/pull/41#discussion_r208042714
--- Diff: taverna-scufl2-api/src/main/java/org/apache/taverna/scufl2/api/common/Scufl2Tools.java ---
@@ -752,6 +753,30 @@ public Activity createActivityFromProcessor(Processor processor,
return activity;
}
+ public Configuration createNestedRelationship(Processor processor, Workflow childWorkflow, Profile profile) {
--- End diff --
I agree, the name isn't very good. I changed it.
---
[GitHub] incubator-taverna-language pull request #41: Add helper method to create nes...
Posted by stain <gi...@git.apache.org>.
Github user stain commented on a diff in the pull request:
https://github.com/apache/incubator-taverna-language/pull/41#discussion_r207914837
--- Diff: taverna-scufl2-api/src/test/java/org/apache/taverna/scufl2/api/common/TestScufl2Tools.java ---
@@ -63,6 +64,25 @@ public void makeBundle() {
makeWorkflowBundle();
assertNotNull(workflowBundle);
}
+
+ @Test
+ public void testNestedWorkflows() {
+ Workflow child = new Workflow();
+ child.setName("childWorkflow");
+ child.setParent(workflowBundle);
+
+ Workflow mainWorkflow = workflowBundle.getMainWorkflow();
+ Processor processor = new Processor();
+ processor.setParent(mainWorkflow);
+
+ Profile profile = workflowBundle.getMainProfile();
+
+ Scufl2Tools tools = new Scufl2Tools();
+ tools.createNestedRelationship(processor, child, profile);
+ Workflow nested = tools.nestedWorkflowForProcessor(processor, profile);
+
+ assertEquals(child, nested);
--- End diff --
Could this test also verify that the corresponding `Activity` and `Configuration` have been added with the correct workflow name and i/o ports?
---