-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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: return itself when getOutboundNodes from memoization Hit steps/DAG. Fixes: #7873 #12780
fix: return itself when getOutboundNodes from memoization Hit steps/DAG. Fixes: #7873 #12780
Conversation
Co-authored-by: zjgemi <liuxin_zijian@163.com> Co-authored-by: sherwinkoo29 <sherwinkoo@163.com> Co-authored-by: shuangkun <tsk2013uestc@163.com> Signed-off-by: shuangkun <tsk2013uestc@163.com>
Co-authored-by: zjgemi <liuxin_zijian@163.com> Co-authored-by: sherwinkoo29 <sherwinkoo@163.com> Co-authored-by: shuangkun <tsk2013uestc@163.com> Signed-off-by: shuangkun <tsk2013uestc@163.com>
Hi,Joibel @Joibel I know you are very familiar with memoize, can you help review this, when a node hits the cache, its node id can be returned when getOutboundNodes. Thank you so much! |
Happy to look at it. I am at Kubecon/Argocon this week, so not sure whether I will get a chance until next week. Can you get the test suite to ✅? |
Thanks, enjoy your conference! My question is not urgent, you can take a look at my question later when you have time. The test suite is OK. |
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.
Looks good, just a couple of things to improve test robustness
assert.Equal(t, wfv1.WorkflowSucceeded, woc.wf.Status.Phase) | ||
|
||
for _, node := range woc.wf.Status.Nodes { | ||
if node.DisplayName == "hello-steps" { |
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.
I'd like to test that we hit this condition once and only once (just in case we end up without this node in the Nodes list.
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.
Thanks! I add it.
a682acc
to
c2b9efe
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.
LGTM. @agilgur5 @isubasinghe @terrytangyuan could you do the magic please
Backported cleanly to |
Fixes #7873
Motivation
The first step "hello-steps" should have one child (The SgNode of "whatever"),but when “hello-steps” hit memoize,it has no child because of getOutboundNodes from the node return nil. Prevents the next sgNode from becoming the child of the previous sgNode’s outboundNodes.
Modifications
return itself when getOutboundNodes from memoization Hit steps/DAG.
Verification
Local test and unit tests.