fix: Revert node creation logic #1818
Conversation
There was a problem hiding this comment.
Thanks @dtaniwaki, I readded TemplateScope. Do you have any comments regarding the rest of the implementation? Is there a reason as to why you moved node creation before execute* that I should be aware of?
There was a problem hiding this comment.
@simster7 Thank you for fixing the problem. The reason why I moved the initialization code to executeTemplate is that I didn't want to pass orgTmpl to each concrete executions and wanted to make sure a given node exists in the concrete execution. However, I now realized the behavior of some of the concrete execution is different from the others as you fixed here.
Is it possible to make some unit tests to avoid future regressions? If it's too hard, we can make it in a future task.
|
Will wait for #1836 to be merged before merging this |
|
@simster7 Could you use a concrete template over |
|
It would be great to get this moving again if possible so we can upgrade without hitting our resource limits! Thanks :) |
|
@benabineri We're planning on releasing this on |
simster7
force-pushed
the
revert-node-creation
branch
from
6f24261
to
8de2384
Compare
Codecov Report
@@ Coverage Diff @@
## master #1818 +/- ##
==========================================
+ Coverage 11.14% 11.15% +<.01%
==========================================
Files 35 35
Lines 23536 23537 +1
==========================================
+ Hits 2624 2625 +1
+ Misses 20576 20575 -1
- Partials 336 337 +1
Continue to review full report at Codecov.
|
| func (woc *wfOperationCtx) executeDAG(nodeName string, tmplCtx *templateresolution.Context, templateScope string, tmpl *wfv1.Template, orgTmpl wfv1.TemplateHolder, boundaryID string) (*wfv1.NodeStatus, error) { | ||
| node := woc.getNodeByName(nodeName) | ||
| if node == nil { | ||
| node = woc.initializeExecutableNode(nodeName, wfv1.NodeTypeSteps, templateScope, tmpl, orgTmpl, boundaryID, wfv1.NodeRunning) |
There was a problem hiding this comment.
The node type is not correct here.
I fixed it in this commit.
396faa0
Revert node creation back to
execute*methods (as it was before #1552). The code flow should be the same as it is now, however this change allows individualexecute*methods to control returning after a node is created once.In particular,
execute{Container, Script, Resource}now return after a node has already been created in order to avoid callingcreateWorkflowPodmore than once. This should fix #1720.@dtaniwaki Since this code is mostly from your changes please review.