Daisy: Recursively build child workflows in NewFromFile (#1693)

Although workflow.NewFromFile's documentation says that it recursively reads subworkflows, that feature was never implemented. This PR ensures that sub workflows and included workflows are read when workflow.NewFromFile is called.

daisy/common.go

@@ -159,19 +159,40 @@ func (w *Workflow) substituteSourceVars(ctx context.Context, v reflect.Value) DE
 	})
 }
 
+// traverseAction allows callers of traverseData to customize the function's traversal.
+type traverseAction uint
+
+const (
+	// Continue the traversal; this is the default action of
+	// traverseData, which traverses to all nodes.
+	continueTraversal traverseAction = iota
+	// Do not process this node or any of its children.
+	prune
+)
+
 // traverseData traverses complex data structures and runs
 // a function, f, on its basic data types.
 // Traverses arrays, maps, slices, and public fields of structs.
 // For example, f will be run on bool, int, string, etc.
 // Slices, maps, and structs will not have f called on them, but will
 // traverse their subelements.
 // Errors returned from f will be returned by traverseDataStructure.
-func traverseData(v reflect.Value, f func(reflect.Value) DError) DError {
+// actions allows the caller to determine which action to take at a node.
+// The default action is 'continueTraverse'.
+func traverseData(v reflect.Value, f func(reflect.Value) DError, actions ...func(reflect.Value) traverseAction) DError {
+
 	if !v.CanSet() {
 		// Don't run on private fields.
 		return nil
 	}
 
+	for _, action := range actions {
+		switch action(v) {
+		case prune:
+			return nil
+		}
+	}
+
 	switch v.Kind() {
 	case reflect.Chan, reflect.Func:
 		return nil
@@ -180,13 +201,13 @@ func traverseData(v reflect.Value, f func(reflect.Value) DError) DError {
 			return nil
 		}
 		// I'm a pointer, dereference me.
-		return traverseData(v.Elem(), f)
+		return traverseData(v.Elem(), f, actions...)
 	}
 
 	switch v.Kind() {
 	case reflect.Array, reflect.Slice:
 		for i := 0; i < v.Len(); i++ {
-			if err := traverseData(v.Index(i), f); err != nil {
+			if err := traverseData(v.Index(i), f, actions...); err != nil {
 				return err
 			}
 		}
@@ -201,10 +222,10 @@ func traverseData(v reflect.Value, f func(reflect.Value) DError) DError {
 			newKv.Set(kv)
 			newVv := reflect.New(vv.Type()).Elem()
 			newVv.Set(vv)
-			if err := traverseData(newKv, f); err != nil {
+			if err := traverseData(newKv, f, actions...); err != nil {
 				return err
 			}
-			if err := traverseData(newVv, f); err != nil {
+			if err := traverseData(newVv, f, actions...); err != nil {
 				return err
 			}
 
@@ -215,7 +236,7 @@ func traverseData(v reflect.Value, f func(reflect.Value) DError) DError {
 		}
 	case reflect.Struct:
 		for i := 0; i < v.NumField(); i++ {
-			if err := traverseData(v.Field(i), f); err != nil {
+			if err := traverseData(v.Field(i), f, actions...); err != nil {
 				return err
 			}
 		}

daisy/step_includeworkflow.go

@@ -33,7 +33,9 @@ type IncludeWorkflow struct {
 }
 
 func (i *IncludeWorkflow) populate(ctx context.Context, s *Step) DError {
-	if i.Path != "" {
+	// Typically s.Workflow is instantiated when the parent workflow is read in NewFromFile.
+	// Workflow could be nil when the parent workflow is constructed manually using Go structs.
+	if i.Path != "" && i.Workflow == nil {
 		var err error
 		if i.Workflow, err = s.w.NewIncludedWorkflowFromFile(i.Path); err != nil {
 			return newErr("failed to parse duration for step includeworkflow", err)
@@ -97,6 +99,14 @@ Loop:
 	}
 	substitute(reflect.ValueOf(i.Workflow).Elem(), strings.NewReplacer(replacements...))
 
+	for name, st := range i.Workflow.Steps {
+		st.name = name
+		st.w = i.Workflow
+		if err := st.w.populateStep(ctx, st); err != nil {
+			return err
+		}
+	}
+
 	// We do this here, and not in validate, as embedded startup scripts could
 	// have what we think are daisy variables.
 	if err := i.Workflow.validateVarsSubbed(); err != nil {
@@ -107,14 +117,6 @@ Loop:
 		return err
 	}
 
-	for name, st := range i.Workflow.Steps {
-		st.name = name
-		st.w = i.Workflow
-		if err := st.w.populateStep(ctx, st); err != nil {
-			return err
-		}
-	}
-
 	// Copy Sources up to parent resolving relative paths as we go.
 	for k, v := range i.Workflow.Sources {
 		if v == "" {

daisy/step_includeworkflow_test.go

@@ -107,7 +107,21 @@ func TestIncludeWorkflowPopulate(t *testing.T) {
 	}
 }
 
-func TestIncludeWorkflowRun(t *testing.T) {}
+func TestIncludeWorkflowPopulate_SkipsReadingPathWhenWorkflowNil(t *testing.T) {
+	child := testWorkflow()
+	parent := testWorkflow()
+	parent.Steps = map[string]*Step{
+		"child": {
+			IncludeWorkflow: &IncludeWorkflow{
+				Path:     "test-will-fail-if-this-is-read",
+				Workflow: child,
+			},
+		},
+	}
+	if err := parent.populate(context.Background()); err != nil {
+		t.Fatal(err)
+	}
+}
 
 func TestIncludeWorkflowValidate(t *testing.T) {
 	ctx := context.Background()

daisy/step_sub_workflow.go

@@ -27,7 +27,9 @@ type SubWorkflow struct {
 }
 
 func (s *SubWorkflow) populate(ctx context.Context, st *Step) DError {
-	if s.Path != "" {
+	// Typically s.Workflow is instantiated when the parent workflow is read in NewFromFile.
+	// Workflow could be nil when the parent workflow is constructed manually using Go structs.
+	if s.Path != "" && s.Workflow == nil {
 		var err error
 		if s.Workflow, err = st.w.NewSubWorkflowFromFile(s.Path); err != nil {
 			return ToDError(err)

daisy/step_sub_workflow_test.go

@@ -58,6 +58,22 @@ func TestSubWorkflowPopulate(t *testing.T) {
 	}
 }
 
+func TestSubWorkflowPopulate_SkipsReadingPathWhenWorkflowNil(t *testing.T) {
+	child := testWorkflow()
+	parent := testWorkflow()
+	parent.Steps = map[string]*Step{
+		"child": {
+			SubWorkflow: &SubWorkflow{
+				Path:     "test-will-fail-if-this-is-read",
+				Workflow: child,
+			},
+		},
+	}
+	if err := parent.populate(context.Background()); err != nil {
+		t.Fatal(err)
+	}
+}
+
 func TestSubWorkflowRun(t *testing.T) {
 	ctx := context.Background()
 	w := testWorkflow()

daisy/test_data/TestNewFromFile_PopulatesVariables.child.wf.json

@@ -0,0 +1,19 @@
+{
+  "Vars": {
+    "k1": {
+      "Required": true,
+      "Description": "The Ubuntu release to translate."
+    }
+  },
+  "steps": {
+    "create-disks": {
+      "createDisks": [
+        {
+          "SourceImage": "image-${k1}",
+          "SizeGb": "50",
+          "Type": "pd-ssd"
+        }
+      ]
+    }
+  }
+}

daisy/test_data/TestNewFromFile_PopulatesVariables.parent.wf.json

@@ -0,0 +1,12 @@
+{
+  "steps": {
+    "include-workflow": {
+      "IncludeWorkflow": {
+        "path": "./TestNewFromFile_PopulatesVariables.child.wf.json",
+        "Vars": {
+          "k1": "v1"
+        }
+      }
+    }
+  }
+}

daisy/test_data/TestNewFromFile_ReadsChildWorkflows.child1.wf.json

@@ -0,0 +1,20 @@
+{
+  "steps": {
+    "include-workflow": {
+      "IncludeWorkflow": {
+        "path": "./TestNewFromFile_ReadsChildWorkflows.child2.wf.json",
+        "Vars": {
+          "k3": "v3"
+        }
+      }
+    },
+    "sub-workflow": {
+      "SubWorkflow": {
+        "path": "./TestNewFromFile_ReadsChildWorkflows.child2.wf.json",
+        "Vars": {
+          "k4": "v4"
+        }
+      }
+    }
+  }
+}

daisy/test_data/TestNewFromFile_ReadsChildWorkflows.child2.wf.json

@@ -0,0 +1,4 @@
+{
+  "steps": {
+  }
+}

daisy/test_data/TestNewFromFile_ReadsChildWorkflows.parent.wf.json

@@ -0,0 +1,20 @@
+{
+  "steps": {
+    "include-workflow": {
+      "IncludeWorkflow": {
+        "path": "./TestNewFromFile_ReadsChildWorkflows.child1.wf.json",
+        "Vars": {
+          "k1": "v1"
+        }
+      }
+    },
+    "sub-workflow": {
+      "SubWorkflow": {
+        "path": "./TestNewFromFile_ReadsChildWorkflows.child1.wf.json",
+        "Vars": {
+          "k2": "v2"
+        }
+      }
+    }
+  }
+}

daisy/test_data/TestNewIncludedWorkflowFromFile_UsesResourcesFromParent.wf.json

@@ -0,0 +1,4 @@
+{
+  "steps": {
+  }
+}

daisy/test_data/test.wf.json

@@ -133,19 +133,6 @@
           "StorageLocations": ["eu", "us-west2"]
         }
       ]
-    },
-
-    "include-workflow": {
-      "IncludeWorkflow": {
-        "path": "./test_sub.wf.json",
-        "Vars": {"key": "value"}
-      }
-    },
-    "sub-workflow": {
-      "subWorkflow": {
-        "path": "./test_sub.wf.json",
-        "Vars": {"key": "value"}
-      }
     }
   },
   "dependencies": {
@@ -156,8 +143,6 @@
     "postinstall-stopped": ["postinstall"],
     "create-image-locality": ["postinstall-stopped"],
     "create-image": ["create-image-locality"],
-    "create-machine-image": ["create-image"],
-    "include-workflow": ["create-image"],
-    "sub-workflow": ["create-image"]
+    "create-machine-image": ["create-image"]
   }
 }

daisy/validate.go

@@ -114,5 +114,11 @@ func (w *Workflow) validateVarsSubbed() DError {
 			}
 		}
 		return nil
+	}, func(v reflect.Value) traverseAction {
+		_, ok := v.Interface().(*Workflow)
+		if ok {
+			return prune
+		}
+		return continueTraversal
 	})
 }