Skip to content
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

[Fix #3390] Process is not completed when node is retriggered (but end node is executed) #3432

Merged
merged 1 commit into from
Mar 11, 2024

Conversation

fjtirado
Copy link
Contributor

@fjtirado fjtirado commented Mar 8, 2024

Calling getNodeInstance over containernode creates another instance, hence the previous node container instance is there when the engine checks if the process can be completed (so it does not complete it)
Therefore, as an alternative, a existing instance with that node id is searched, if found, that is the parent container instance where to recreate the node instance to be retriggered. If not found, we call getNodeInstance over the container node.

@fjtirado fjtirado force-pushed the Fix_#3390_second_attempt branch 2 times, most recently from 126bc75 to c705786 Compare March 8, 2024 17:25
@fjtirado fjtirado changed the title [Fix #3390] Do not create container intance, search it [Fix #3390] Do not create container instance, search it Mar 8, 2024
@fjtirado fjtirado force-pushed the Fix_#3390_second_attempt branch from c705786 to ecd3e8c Compare March 8, 2024 17:29
@fjtirado fjtirado force-pushed the Fix_#3390_second_attempt branch from ecd3e8c to 41d300a Compare March 8, 2024 17:49
@fjtirado fjtirado changed the title [Fix #3390] Do not create container instance, search it [Fix #3390] Process is not completed when node is retriggered (but end node is executed) Mar 8, 2024
@fjtirado fjtirado requested review from nmirasch and caponetto March 8, 2024 17:51
@apache apache deleted a comment from kie-ci3 Mar 8, 2024
@fjtirado fjtirado merged commit 5728052 into apache:main Mar 11, 2024
6 checks passed
@elguardian
Copy link
Contributor

@fjtirado this is incorrectly merged You need two commiters approval before merge, not much of a hurry you have. That is the procedure. You also reused an issue.
@porcelli @martinweiler @baldimir

@fjtirado
Copy link
Contributor Author

fjtirado commented Mar 11, 2024

@elguardian I reused the original issue because it was not completely working, as can be easily seen following the code changes, the explanation in the description and the additional IT test check. I do not really see any problem, other than I forgot to reopen it (to be honest I do not know if that is possible, which I know is that all commits related to a problem inherited from V7 are in the same github issue, which is good for inquiries)
I believe two approvals was not a strict requirement and one approval was enough, but from your reaction I guess thats not longer the case. My apologies, next time, I will wait to have two approvals (in this particular case, I was not in a hurry, the reason I merged is because I really believe one commiter was enough and the PR was green)
To avoid that happening again, I recommend enforcing this two commiter restriction using github rules.

@caponetto
Copy link

caponetto commented Mar 11, 2024

LGTM! (I can't approve a merged PR though)

rgdoliveira pushed a commit to rgdoliveira/kogito-runtimes that referenced this pull request Mar 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants