Add helper method to create nesting relationship between workflows #41
Conversation
| @@ -752,6 +753,30 @@ public Activity createActivityFromProcessor(Processor processor, | |||
| return activity; | |||
| } | |||
|
|
|||
| public Configuration createNestedRelationship(Processor processor, Workflow childWorkflow, Profile profile) { | |||
There was a problem hiding this comment.
Unsure about relationship here.. Should we call this method setAsNestedWorkflow() or something?
There was a problem hiding this comment.
I agree, the name isn't very good. I changed it.
| tools.createNestedRelationship(processor, child, profile); | ||
| Workflow nested = tools.nestedWorkflowForProcessor(processor, profile); | ||
|
|
||
| assertEquals(child, nested); |
There was a problem hiding this comment.
Could this test also verify that the corresponding Activity and Configuration have been added with the correct workflow name and i/o ports?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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..
There was a problem hiding this comment.
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 !
| throw new IllegalStateException( | ||
| "Processor " + processor + " and workflow " + childWorkflow + " are not in the same Workflow bundle"); | ||
| } | ||
| if(nestedWorkflowForProcessor(processor, profile) != null) { |
There was a problem hiding this comment.
These are good guards, but they should also be verified by separate @Tests.
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?
There was a problem hiding this comment.
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.
|
Thank you so much for you comments! I replied to them and added a patch. |
| @@ -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 { | |||
There was a problem hiding this comment.
Uh, I see what you mean.. it's not good to have logic by exception catching, is it..
There was a problem hiding this comment.
I found a better way to test if a processor already has a bound activity. You were right about the exception catching logic. Thanks
- Check if nesting processor doesn't already have a bound activity
| @@ -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; | |||
There was a problem hiding this comment.
I don't think we need sun.security.krb5.Config (or that it's even legal to import)
| boolean found = false; | ||
| ProcessorBinding binding = processor.getBinding(profile); | ||
| Activity activity = binding.getBoundActivity(); | ||
| Configuration configuration = activity.getConfiguration(); |
There was a problem hiding this comment.
Except for inconsistent indentation this looks much better!
No description provided.