-
Notifications
You must be signed in to change notification settings - Fork 225
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[incubator-kie-issues-1306] Exclusive gateway v7 port #3551
Conversation
b2e9d29
to
186be93
Compare
034eeac
to
9d87851
Compare
9d87851
to
4a7ea73
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 when green. I only left a comment to clarify, everything else seems fine.
@@ -55,6 +56,12 @@ public class ActionNodeVisitor extends AbstractNodeVisitor<ActionNode> { | |||
|
|||
private static final String INTERMEDIATE_COMPENSATION_TYPE = "IntermediateThrowEvent-None"; | |||
|
|||
private ReturnValueEvaluatorBuilderService returnValueEvaluatorBuilderService; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this really needed? I don't see it's being used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@elguardian in addition to my comment on FlowTest#testConditionalFlow
- I'd expect the process to execute start > script > end1 only, but the following assertion fails as the other end nodes are executed as well:
assertThat(listener.tracked())
.anyMatch(ProcessTestHelper.triggered("start"))
.anyMatch(ProcessTestHelper.triggered("script"))
.anyMatch(ProcessTestHelper.triggered("end1"));
assertThat(listener.tracked())
.noneMatch(ProcessTestHelper.triggered("end2"));
Maybe I am missing something?!
4a7ea73
to
ed42466
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work @elguardian
One question: previously, users could set the jbpm.enable.multi.con
system property. Now it seems you need to define this as metadata property element on the process definition itself, unless you define the system property at compile time (I am not sure exactly how this would need to be done). Setting it at runtime won't be too late, as the process generation already failed - correct?
@martinweiler you are correct. runtime is too late as the code was generated already (failed because the flag was not defined) |
ed42466
to
e1eb0c8
Compare
96de253
to
0e0c4a9
Compare
Closes: apache/incubator-kie-issues#1306