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

3 files changed, 96 insertions, 19 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) {
diff --git a/docker/compiler.go b/docker/compiler.go
index c9ca0fb..d7043a0 100644
--- a/docker/compiler.go
+++ b/docker/compiler.go
@@ -2,6 +2,7 @@ package docker
 
 import (
 	"bytes"
+	"log"
 	"strings"
 
 	"phabricator.wikimedia.org/source/blubber.git/build"
@@ -13,23 +14,25 @@ func Compile(cfg *config.Config, variant string) *bytes.Buffer {
 
 	vcfg, err := config.ExpandVariant(cfg, variant)
 
-	if err == nil {
-		// omit the main stage name unless multi-stage is required below
-		mainStage := ""
+	if err != nil {
+		log.Fatal(err)
+	}
 
-		// write multi-stage sections for each variant dependency
-		for _, stage := range vcfg.VariantDependencies() {
-			dependency, err := config.ExpandVariant(cfg, stage)
+	// omit the main stage name unless multi-stage is required below
+	mainStage := ""
 
-			if err == nil {
-				CompileStage(buffer, stage, dependency)
-				mainStage = variant
-			}
-		}
+	// write multi-stage sections for each variant dependency
+	for _, stage := range vcfg.VariantDependencies() {
+		dependency, err := config.ExpandVariant(cfg, stage)
 
-		CompileStage(buffer, mainStage, vcfg)
+		if err == nil {
+			CompileStage(buffer, stage, dependency)
+			mainStage = variant
+		}
 	}
 
+	CompileStage(buffer, mainStage, vcfg)
+
 	return buffer
 }