Merge pull request #1825 from carolynvs/fix-build-driver-cfg

Fix build driver experimental flag checks

pkg/build/dockerfile-generator_test.go

@@ -8,6 +8,7 @@ import (
 	"testing"
 
 	"get.porter.sh/porter/pkg/config"
+	"get.porter.sh/porter/pkg/experimental"
 	"get.porter.sh/porter/pkg/manifest"
 	"get.porter.sh/porter/pkg/mixin"
 	"get.porter.sh/porter/pkg/templates"
@@ -26,6 +27,7 @@ func TestPorter_buildDockerfile(t *testing.T) {
 
 			c := config.NewTestConfig(t)
 			c.Data.BuildDriver = driver
+			c.SetExperimentalFlags(experimental.FlagBuildDrivers)
 			tmpl := templates.NewTemplates(c.Config)
 			configTpl, err := tmpl.GetManifest()
 			require.Nil(t, err)

pkg/cli/config_test.go

@@ -111,7 +111,7 @@ func TestLoadHierarchicalConfig(t *testing.T) {
 
 	t.Run("build-driver from config", func(t *testing.T) {
 		os.Unsetenv("PORTER_BUILD_DRIVER")
-		defer os.Unsetenv("PORTER_EXPERIMENTAL")
+		defer os.Unsetenv("PORTER_BUILD_DRIVER")
 
 		c := config.NewTestConfig(t)
 		c.SetHomeDir("/root/.porter")

pkg/config/config.go

@@ -242,3 +242,13 @@ func (c *Config) IsFeatureEnabled(flag experimental.FeatureFlags) bool {
 func (c *Config) SetExperimentalFlags(flags experimental.FeatureFlags) {
 	c.experimental = &flags
 }
+
+// GetBuildDriver determines the correct build driver to use, taking
+// into account experimental flags.
+// Use this instead of Config.Data.BuildDriver directly.
+func (c *Config) GetBuildDriver() string {
+	if c.IsFeatureEnabled(experimental.FlagBuildDrivers) {
+		return c.Data.BuildDriver
+	}
+	return BuildDriverDocker
+}

pkg/config/config_test.go

@@ -81,3 +81,12 @@ func TestConfigExperimentalFlags(t *testing.T) {
 		assert.True(t, c.IsFeatureEnabled(experimental.FlagBuildDrivers))
 	})
 }
+
+func TestConfig_GetBuildDriver(t *testing.T) {
+	c := NewTestConfig(t)
+	c.Data.BuildDriver = BuildDriverBuildkit
+	require.Equal(t, BuildDriverDocker, c.GetBuildDriver(), "Default to docker when experimental is false, even when a build driver is set")
+
+	c.SetExperimentalFlags(experimental.FlagBuildDrivers)
+	require.Equal(t, BuildDriverBuildkit, c.GetBuildDriver(), "Use the specified driver when the build driver feature is enabled")
+}

pkg/config/datastore.go

@@ -13,6 +13,7 @@ type Data struct {
 
 	// BuildDriver is the driver to use when building bundles.
 	// Available values are: docker, buildkit.
+	// Do not use directly, use Config.GetBuildDriver.
 	BuildDriver string `mapstructure:"build-driver"`
 
 	// RuntimeDriver is the driver to use when executing bundles.

pkg/porter/build.go

@@ -41,7 +41,7 @@ func (o *BuildOptions) Validate(p *Porter) error {
 	}
 
 	if o.Driver == "" {
-		o.Driver = p.Data.BuildDriver
+		o.Driver = p.GetBuildDriver()
 	}
 	if !stringSliceContains(BuildDriverAllowedValues, o.Driver) {
 		return errors.Errorf("invalid --driver value %s", o.Driver)
@@ -68,7 +68,7 @@ func (p *Porter) Build(opts BuildOptions) error {
 	opts.Apply(p.Context)
 
 	if p.Debug {
-		fmt.Fprintf(p.Err, "Using %s build driver\n", p.Data.BuildDriver)
+		fmt.Fprintf(p.Err, "Using %s build driver\n", p.GetBuildDriver())
 	}
 
 	// Start with a fresh .cnab directory before building

pkg/porter/porter.go

@@ -12,7 +12,6 @@ import (
 	cnabprovider "get.porter.sh/porter/pkg/cnab/provider"
 	"get.porter.sh/porter/pkg/config"
 	"get.porter.sh/porter/pkg/credentials"
-	"get.porter.sh/porter/pkg/experimental"
 	"get.porter.sh/porter/pkg/manifest"
 	"get.porter.sh/porter/pkg/mixin"
 	"get.porter.sh/porter/pkg/parameters"
@@ -137,14 +136,11 @@ func (p *Porter) LoadManifestFrom(file string) error {
 // NewBuilder creates a Builder based on the current configuration.
 func (p *Porter) GetBuilder() build.Builder {
 	if p.builder == nil {
-		if p.IsFeatureEnabled(experimental.FlagBuildDrivers) {
-			switch p.Config.Data.BuildDriver {
-			case config.BuildDriverBuildkit:
-				p.builder = buildkit.NewBuilder(p.Context)
-			case config.BuildDriverDocker:
-				p.builder = docker.NewBuilder(p.Context)
-			}
-		} else {
+		switch p.GetBuildDriver() {
+		case config.BuildDriverBuildkit:
+			p.builder = buildkit.NewBuilder(p.Context)
+		case config.BuildDriverDocker:
+		default:
 			p.builder = docker.NewBuilder(p.Context)
 		}
 	}

pkg/templates/templates.go

@@ -58,7 +58,7 @@ func (t *Templates) GetDockerignore() ([]byte, error) {
 
 // GetDockerfileTemplate returns a template.Dockerfile file for use in new bundles.
 func (t *Templates) GetDockerfileTemplate() ([]byte, error) {
-	tmpl := fmt.Sprintf("templates/create/template.%s.Dockerfile", t.Data.BuildDriver)
+	tmpl := fmt.Sprintf("templates/create/template.%s.Dockerfile", t.GetBuildDriver())
 	return t.fs.ReadFile(tmpl)
 }
 
@@ -75,6 +75,6 @@ func (t *Templates) GetSchema() ([]byte, error) {
 
 // GetDockerfile returns the default Dockerfile for invocation images.
 func (t *Templates) GetDockerfile() ([]byte, error) {
-	tmpl := fmt.Sprintf("templates/build/%s.Dockerfile", t.Data.BuildDriver)
+	tmpl := fmt.Sprintf("templates/build/%s.Dockerfile", t.GetBuildDriver())
 	return t.fs.ReadFile(tmpl)
 }

pkg/templates/templates_test.go

@@ -6,6 +6,8 @@ import (
 	"testing"
 
 	"get.porter.sh/porter/pkg/config"
+	"get.porter.sh/porter/pkg/experimental"
+	"get.porter.sh/porter/pkg/test"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
@@ -37,12 +39,24 @@ func TestTemplates_GetDockerfile(t *testing.T) {
 	for _, driver := range testcases {
 		c := config.NewTestConfig(t)
 		c.Data.BuildDriver = driver
+		c.SetExperimentalFlags(experimental.FlagBuildDrivers)
 		tmpl := NewTemplates(c.Config)
 
 		gotTmpl, err := tmpl.GetDockerfile()
 		require.NoError(t, err)
 
-		wantTmpl, _ := ioutil.ReadFile(fmt.Sprintf("./templates/build/%s.Dockerfile", driver))
-		assert.Equal(t, wantTmpl, gotTmpl)
+		test.CompareGoldenFile(t, fmt.Sprintf("./templates/build/%s.Dockerfile", driver), string(gotTmpl))
 	}
+
+	t.Run("experimental flag required to use buildkit", func(t *testing.T) {
+		// Should use the docker template because the experimental feature isn't set
+		c := config.NewTestConfig(t)
+		c.Data.BuildDriver = "buildkit"
+		tmpl := NewTemplates(c.Config)
+
+		gotTmpl, err := tmpl.GetDockerfile()
+		require.NoError(t, err)
+
+		test.CompareGoldenFile(t, "./templates/build/docker.Dockerfile", string(gotTmpl))
+	})
 }