Daisy: Recursively build child workflows in NewFromFile when path doe…

…sn't have variable. (#1701)

This adds support for recursively reading child workflows. The feature was first added in #1693, but rolled back in #1700 due to a bug that it introduced.

The issue: Variable instantiate occurs during populate, and callers expect that behavior. Callers also expect to define child workflow names using variables. Those are in conflict if we want to read child workflows before populate.

To address this, I added a fix in eacb97a where child workflows are eagerly read only when they're fully specified. If the filename contains a variable, then the legacy behavior of reading the workflow during the step's populate is retained.

daisy/common.go

@@ -29,6 +29,10 @@ import (
 	"google.golang.org/api/compute/v1"
 )
 
+var (
+	varPattern = regexp.MustCompile(`\$\{.*\}`)
+)
+
 func getUser() string {
 	if cu, err := user.Current(); err == nil {
 		return cu.Username
@@ -124,6 +128,11 @@ func substitute(v reflect.Value, replacer *strings.Replacer) {
 	})
 }
 
+// hasVariableDeclaration determines whether s contains a variable declaration of the style `${varname}`
+func hasVariableDeclaration(s string) bool {
+	return varPattern.MatchString(s)
+}
+
 func getRegionFromZone(z string) string {
 	if z != "" {
 		return z[:len(z)-2]
@@ -159,19 +168,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 +210,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 +231,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 +245,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/common_test.go

@@ -74,6 +74,27 @@ func TestMinInt(t *testing.T) {
 	}
 }
 
+func TestHasVariableDeclaration(t *testing.T) {
+	tests := []struct {
+		desc string
+		s    string
+		want bool
+	}{
+		{"no declaration", "content", false},
+		{"no declaration: empty string", "", false},
+		{"no declaration: only dollar", "$var", false},
+		{"no declaration: no closing bracket", "{", false},
+		{"contains declaration", "content ${k}", true},
+		{"contains declaration: source", "${SOURCE: fname}", true},
+	}
+
+	for _, tt := range tests {
+		if got := hasVariableDeclaration(tt.s); got != tt.want {
+			t.Errorf("%s: %v != %v", tt.desc, got, tt.want)
+		}
+	}
+}
+
 func TestRandString(t *testing.T) {
 	for i := 0; i < 10; i++ {
 		l := len(randString(i))

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_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/TestNewFromFile_SupportsNestedVariables.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_SupportsNestedVariables.parent.wf.json

@@ -0,0 +1,20 @@
+{
+  "vars": {
+    "var_name_is_variable": {
+      "Value": "k1"
+    },
+    "var_value_is_variable": {
+      "Value": "v1"
+    }
+  },
+  "steps": {
+    "include-workflow": {
+      "IncludeWorkflow": {
+        "path": "./TestNewFromFile_SupportsNestedVariables.child.wf.json",
+        "Vars": {
+          "${var_name_is_variable}": "${var_value_is_variable}"
+        }
+      }
+    }
+  }
+}

daisy/test_data/TestNewFromFile_SupportsNestedVariables_VarInFilename.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_SupportsNestedVariables_VarInFilename.parent.wf.json

@@ -0,0 +1,23 @@
+{
+  "vars": {
+    "filename_is_variable": {
+      "Value": "TestNewFromFile_SupportsNestedVariables_VarInFilename.child.wf.json"
+    },
+    "var_name_is_variable": {
+      "Value": "k1"
+    },
+    "var_value_is_variable": {
+      "Value": "v1"
+    }
+  },
+  "steps": {
+    "include-workflow": {
+      "IncludeWorkflow": {
+        "path": "${filename_is_variable}",
+        "Vars": {
+          "${var_name_is_variable}": "${var_value_is_variable}"
+        }
+      }
+    }
+  }
+}

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"]
   }
 }