Add process refKey #1558
Add process refKey #1558
Conversation
krhubert
force-pushed
the
feature/ref-key
branch
from
43d89e3
to
2a755bb
Compare
|
TODO:
|
NicolasMahe
changed the title
NicolasMahe
changed the title
process/process.go
Outdated
| if ref := output.GetRef(); ref != nil { | ||
| if _, err := w.FindNode(ref.NodeKey); err != nil { | ||
| if ref := output.GetRef(); ref != nil && ref.RefKey != "" { | ||
| if _, err := w.FindNode(ref.RefKey); err != nil { |
There was a problem hiding this comment.
I don't think this is right, findNode with check the nodeKey, in our case nodeKey is different than refKey.
This should be a find with a filter that the node is a task and has the same refKey or am I missing something here?
| @@ -90,7 +91,7 @@ func (s *Orchestrator) dependencyFilter(exec *execution.Execution) func(wf *proc | |||
| if len(parents) > 1 { | |||
| return false, fmt.Errorf("multi parents not supported") | |||
| } | |||
| return parents[0] == exec.NodeKey, nil | |||
| return parents[0] == exec.RefKey, nil | |||
There was a problem hiding this comment.
parents are the list of parent nodes, so the list of nodeKey, I don't think this is right
| @@ -33,6 +33,7 @@ func testOrchestratorRefTask(executionStream pb.Execution_StreamClient, instance | |||
| Key: "n1", | |||
| Type: &process.Process_Node_Task_{ | |||
| Task: &process.Process_Node_Task{ | |||
| RefKey: "n1", | |||
There was a problem hiding this comment.
Let's use a different name for key and refKey to avoid false positive in the tests
| RefKey: "n1", | |
| RefKey: "ref1", |
|
@antho1404 will finish the modif of this PR |
| if refNode == nil { | ||
| return fmt.Errorf("%q reference not found", ref.RefKey) | ||
| } |
There was a problem hiding this comment.
Shouldn't this error be returned by FindTaskNodeFromRef ?
| @@ -45,3 +49,25 @@ func (w *Process) Trigger() (*Process_Node, error) { | |||
| } | |||
| return triggers[0], nil | |||
| } | |||
|
|
|||
| // FindTaskNodeFromRef returns the node associated to a reference in the nodeKey's parents, returns an nil if not found | |||
| func (w *Process) FindTaskNodeFromRef(nodeKey string, refKey string) (*Process_Node, error) { | |||
There was a problem hiding this comment.
| func (w *Process) FindTaskNodeFromRef(nodeKey string, refKey string) (*Process_Node, error) { | |
| func (w *Process) FindTaskNodeFromRef(nodeKey, refKey string) (*Process_Node, error) { |
Not mandatory
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if node.GetTask() != nil && node.GetTask().RefKey == refKey { |
There was a problem hiding this comment.
| if node.GetTask() != nil && node.GetTask().RefKey == refKey { | |
| if task := node.GetTask(); task != nil && task.RefKey == refKey { |
| refNode = r | ||
| } |
There was a problem hiding this comment.
shoudn't we break here on refNode found?
|
I think this PR adds too much complexity for no real benefits. I created another PR (#1567) with a simple check on the parent and the ref that solves the data availability issue and still resolve based on the This way we have a single system (nodeKey) to construct the graph and reference values but with additional checks. There are more checks to do on the process that is listed on this issue #1568 |
close #1557
From what I see in this implementation
nodeKey(in Reference) was also needed to find the right execution, @NicolasMahe any thoughts on that?