Recursive variant expansion

Summary:
Problem running blubber cfg.yml test with this config:

```
base: nodejs-slim
variants:
  build:
    base: nodejs-devel
  development:
    includes: [build]
    entrypoint: [npm, start]
  test:
    includes: [development]
    entrypoint: [npm, test]
```

yields a Dockerfile with `FROM nodejs-slim`; however, I expected that
the base would be `nodejs-devel` since `test` inherits from
`development` which inherits from `build`.

In order for this to work as expected we have to recursively expand
variants as in this patch.

Reviewers: dduvall, #release-engineering-team

Reviewed By: dduvall, #release-engineering-team

Tags: #release-engineering-team

Differential Revision: https://phabricator.wikimedia.org/D773

2 files changed, 81 insertions, 7 deletions

diff --git a/config/reader.go b/config/reader.go
index 05f50ce..0f4cc12 100644
--- a/config/reader.go
+++ b/config/reader.go
@@ -2,26 +2,52 @@ package config
 
 import (
 	"errors"
+	"fmt"
 	"io/ioutil"
 
 	"gopkg.in/yaml.v2"
 )
 
-func ExpandVariant(config *Config, name string) (*VariantConfig, error) {
+func expandIncludes(config *Config, name string, included map[string]bool) ([]string, error) {
 	variant, found := config.Variants[name]
 
 	if !found {
-		return nil, errors.New("variant does not exist")
+		return nil, fmt.Errorf("variant '%s' does not exist", name)
 	}
 
-	expanded := new(VariantConfig)
-	expanded.CommonConfig.Merge(config.CommonConfig)
-	expanded.Merge(variant)
+	if included[name] == true {
+		return nil, errors.New("variant expansion detected loop")
+	}
 
 	for _, include := range variant.Includes {
-		if includedVariant, found := config.Variants[include]; found {
-			expanded.Merge(includedVariant)
+		included[name] = true
+		inc, err := expandIncludes(config, include, included)
+
+		if err != nil {
+			return nil, err
 		}
+
+		return append(inc, include), nil
+	}
+
+	return []string{}, nil
+}
+
+// ExpandVariant merges a named variant with a config. It also attempts to
+// recursively expand any included variants in the expanded variant.
+func ExpandVariant(config *Config, name string) (*VariantConfig, error) {
+	expanded := new(VariantConfig)
+	expanded.CommonConfig.Merge(config.CommonConfig)
+
+	includes, err := expandIncludes(config, name, map[string]bool{})
+	includes = append(includes, name)
+
+	if err != nil {
+		return nil, err
+	}
+
+	for _, include := range includes {
+		expanded.Merge(config.Variants[include])
 	}
 
 	return expanded, nil
diff --git a/config/variant_test.go b/config/variant_test.go
index 666b4d7..1cf143f 100644
--- a/config/variant_test.go
+++ b/config/variant_test.go
@@ -44,6 +44,54 @@ func TestVariantDependencies(t *testing.T) {
 	assert.Equal(t, []string{"foo", "build"}, cfg.VariantDependencies())
 }
 
+func TestVariantLoops(t *testing.T) {
+	cfg := config.Config{
+		Variants: map[string]config.VariantConfig{
+			"foo": config.VariantConfig{Includes: []string{"bar"}},
+			"bar": config.VariantConfig{Includes: []string{"foo"}}}}
+
+	cfgTwo := config.Config{
+		Variants: map[string]config.VariantConfig{
+			"foo": config.VariantConfig{},
+			"bar": config.VariantConfig{Includes: []string{"foo"}}}}
+
+	// Configuration that contains a loop in "Includes" should error
+	_, err := config.ExpandVariant(&cfg, "bar")
+	assert.NotNil(t, err)
+
+	_, errTwo := config.ExpandVariant(&cfgTwo, "bar")
+	assert.Nil(t, errTwo)
+}
+
+func TestMultiLevelIncludes(t *testing.T) {
+	cfg, err := config.ReadConfig([]byte(`---
+    base: nodejs-slim
+    variants:
+      build:
+        base: nodejs-devel
+        node: {env: build}
+      development:
+        includes: [build]
+        node: {env: development}
+        entrypoint: [npm, start]
+      test:
+        includes: [development]
+        node: {dependencies: true}
+        entrypoint: [npm, test]`))
+
+	assert.Nil(t, err)
+
+	variant, _ := config.ExpandVariant(cfg, "test")
+
+	assert.Equal(t, "nodejs-devel", variant.Base)
+	assert.Equal(t, "development", variant.Node.Env)
+
+	devVariant, _ := config.ExpandVariant(cfg, "development")
+
+	assert.True(t, variant.Node.Dependencies.True)
+	assert.False(t, devVariant.Node.Dependencies.True)
+}
+
 func TestVariantConfigInstructions(t *testing.T) {
 	t.Run("PhaseInstall", func(t *testing.T) {
 		t.Run("copies", func(t *testing.T) {