Merge pull request #12383 from hashicorp/jbardin/multi-var-errs

report all errors from module validation

config/module/test-fixtures/validate-required-var/child/main.tf

@@ -1 +1,2 @@
 variable "memory" {}
+variable "feature" {}

config/module/tree.go

@@ -259,7 +259,7 @@ func (t *Tree) Validate() error {
 	}
 
 	// If something goes wrong, here is our error template
-	newErr := &TreeError{Name: []string{t.Name()}}
+	newErr := &treeError{Name: []string{t.Name()}}
 
 	// Terraform core does not handle root module children named "root".
 	// We plan to fix this in the future but this bug was brought up in
@@ -271,15 +271,14 @@ func (t *Tree) Validate() error {
 
 	// Validate our configuration first.
 	if err := t.config.Validate(); err != nil {
-		newErr.Err = err
-		return newErr
+		newErr.Add(err)
 	}
 
 	// If we're the root, we do extra validation. This validation usually
 	// requires the entire tree (since children don't have parent pointers).
 	if len(t.path) == 0 {
 		if err := t.validateProviderAlias(); err != nil {
-			return err
+			newErr.Add(err)
 		}
 	}
 
@@ -293,7 +292,7 @@ func (t *Tree) Validate() error {
 			continue
 		}
 
-		verr, ok := err.(*TreeError)
+		verr, ok := err.(*treeError)
 		if !ok {
 			// Unknown error, just return...
 			return err
@@ -301,7 +300,7 @@ func (t *Tree) Validate() error {
 
 		// Append ourselves to the error and then return
 		verr.Name = append(verr.Name, t.Name())
-		return verr
+		newErr.AddChild(verr)
 	}
 
 	// Go over all the modules and verify that any parameters are valid
@@ -327,10 +326,9 @@ func (t *Tree) Validate() error {
 		// Compare to the keys in our raw config for the module
 		for k, _ := range m.RawConfig.Raw {
 			if _, ok := varMap[k]; !ok {
-				newErr.Err = fmt.Errorf(
+				newErr.Add(fmt.Errorf(
 					"module %s: %s is not a valid parameter",
-					m.Name, k)
-				return newErr
+					m.Name, k))
 			}
 
 			// Remove the required
@@ -339,10 +337,9 @@ func (t *Tree) Validate() error {
 
 		// If we have any required left over, they aren't set.
 		for k, _ := range requiredMap {
-			newErr.Err = fmt.Errorf(
-				"module %s: required variable %s not set",
-				m.Name, k)
-			return newErr
+			newErr.Add(fmt.Errorf(
+				"module %s: required variable %q not set",
+				m.Name, k))
 		}
 	}
 
@@ -369,33 +366,61 @@ func (t *Tree) Validate() error {
 				}
 			}
 			if !found {
-				newErr.Err = fmt.Errorf(
+				newErr.Add(fmt.Errorf(
 					"%s: %s is not a valid output for module %s",
-					source, mv.Field, mv.Name)
-				return newErr
+					source, mv.Field, mv.Name))
 			}
 		}
 	}
 
-	return nil
+	return newErr.ErrOrNil()
+}
+
+// treeError is an error use by Tree.Validate to accumulates all
+// validation errors.
+type treeError struct {
+	Name     []string
+	Errs     []error
+	Children []*treeError
+}
+
+func (e *treeError) Add(err error) {
+	e.Errs = append(e.Errs, err)
+}
+
+func (e *treeError) AddChild(err *treeError) {
+	e.Children = append(e.Children, err)
 }
 
-// TreeError is an error returned by Tree.Validate if an error occurs
-// with validation.
-type TreeError struct {
-	Name []string
-	Err  error
+func (e *treeError) ErrOrNil() error {
+	if len(e.Errs) > 0 || len(e.Children) > 0 {
+		return e
+	}
+	return nil
 }
 
-func (e *TreeError) Error() string {
-	// Build up the name
-	var buf bytes.Buffer
-	for _, n := range e.Name {
-		buf.WriteString(n)
-		buf.WriteString(".")
+func (e *treeError) Error() string {
+	name := strings.Join(e.Name, ".")
+	var out bytes.Buffer
+	fmt.Fprintf(&out, "module %s: ", name)
+
+	if len(e.Errs) == 1 {
+		// single like error
+		out.WriteString(e.Errs[0].Error())
+	} else {
+		// multi-line error
+		for _, err := range e.Errs {
+			fmt.Fprintf(&out, "\n    %s", err)
+		}
+	}
+
+	if len(e.Children) > 0 {
+		// start the next error on a new line
+		out.WriteString("\n  ")
+	}
+	for _, child := range e.Children {
+		out.WriteString(child.Error())
 	}
-	buf.Truncate(buf.Len() - 1)
 
-	// Format the value
-	return fmt.Sprintf("module %s: %s", buf.String(), e.Err)
+	return out.String()
 }

config/module/tree_test.go

@@ -410,9 +410,18 @@ func TestTreeValidate_requiredChildVar(t *testing.T) {
 		t.Fatalf("err: %s", err)
 	}
 
-	if err := tree.Validate(); err == nil {
+	err := tree.Validate()
+	if err == nil {
 		t.Fatal("should error")
 	}
+
+	// ensure both variables are mentioned in the output
+	errMsg := err.Error()
+	for _, v := range []string{"feature", "memory"} {
+		if !strings.Contains(errMsg, v) {
+			t.Fatalf("no mention of missing variable %q", v)
+		}
+	}
 }
 
 const treeLoadStr = `