Merge pull request #9633 from hashicorp/f-experiment

helper/experiment: a helper for setting, making experiments

command/apply.go

@@ -10,6 +10,7 @@ import (
 	"github.com/hashicorp/go-getter"
 	"github.com/hashicorp/go-multierror"
 	"github.com/hashicorp/terraform/config"
+	"github.com/hashicorp/terraform/helper/experiment"
 	"github.com/hashicorp/terraform/terraform"
 )
 
@@ -96,7 +97,7 @@ func (c *ApplyCommand) Run(args []string) int {
 	}
 
 	// Check for the new apply
-	if terraform.X_newApply {
+	if experiment.Enabled(experiment.X_newApply) && !experiment.Force() {
 		desc := "Experimental new apply graph has been enabled. This may still\n" +
 			"have bugs, and should be used with care. If you'd like to continue,\n" +
 			"you must enter exactly 'yes' as a response."
@@ -116,7 +117,7 @@ func (c *ApplyCommand) Run(args []string) int {
 	}
 
 	// Check for the new destroy
-	if terraform.X_newDestroy {
+	if experiment.Enabled(experiment.X_newDestroy) && !experiment.Force() {
 		desc := "Experimental new destroy graph has been enabled. This may still\n" +
 			"have bugs, and should be used with care. If you'd like to continue,\n" +
 			"you must enter exactly 'yes' as a response."

command/meta.go

@@ -11,6 +11,7 @@ import (
 
 	"github.com/hashicorp/go-getter"
 	"github.com/hashicorp/terraform/config/module"
+	"github.com/hashicorp/terraform/helper/experiment"
 	"github.com/hashicorp/terraform/state"
 	"github.com/hashicorp/terraform/terraform"
 	"github.com/mitchellh/cli"
@@ -336,8 +337,7 @@ func (m *Meta) flagSet(n string) *flag.FlagSet {
 	f.BoolVar(&m.shadow, "shadow", true, "shadow graph")
 
 	// Experimental features
-	f.BoolVar(&terraform.X_newApply, "Xnew-apply", false, "experiment: new apply")
+	experiment.Flag(f)
-	f.BoolVar(&terraform.X_newDestroy, "Xnew-destroy", false, "experiment: new destroy")
 
 	// Create an io.Writer that writes to our Ui properly for errors.
 	// This is kind of a hack, but it does the job. Basically: create

helper/experiment/experiment.go

@@ -0,0 +1,162 @@
+// experiment package contains helper functions for tracking experimental
+// features throughout Terraform.
+//
+// This package should be used for creating, enabling, querying, and deleting
+// experimental features. By unifying all of that onto a single interface,
+// we can have the Go compiler help us by enforcing every place we touch
+// an experimental feature.
+//
+// To create a new experiment:
+//
+//   1. Add the experiment to the global vars list below, prefixed with X_
+//
+//   2. Add the experiment variable to the All listin the init() function
+//
+//   3. Use it!
+//
+// To remove an experiment:
+//
+//   1. Delete the experiment global var.
+//
+//   2. Try to compile and fix all the places where the var was referenced.
+//
+// To use an experiment:
+//
+//   1. Use Flag() if you want the experiment to be available from the CLI.
+//
+//   2. Use Enabled() to check whether it is enabled.
+//
+// As a general user:
+//
+//   1. The `-Xexperiment-name` flag
+//   2. The `TF_X_<experiment-name>` env var.
+//   3. The `TF_X_FORCE` env var can be set to force an experimental feature
+//      without human verifications.
+//
+package experiment
+
+import (
+	"flag"
+	"fmt"
+	"os"
+	"strconv"
+	"strings"
+	"sync"
+)
+
+// The experiments that are available are listed below. Any package in
+// Terraform defining an experiment should define the experiments below.
+// By keeping them all within the experiment package we force a single point
+// of definition and use. This allows the compiler to enforce references
+// so it becomes easy to remove the features.
+var (
+	// New apply graph. This will be removed and be the default in 0.8.0.
+	X_newApply = newBasicID("new-apply", "NEW_APPLY", false)
+
+	// New destroy graph. This will be reomved and be the default in 0.8.0.
+	X_newDestroy = newBasicID("new-destroy", "NEW_DESTROY", false)
+
+	// Shadow graph. This is already on by default. Disabling it will be
+	// allowed for awhile in order for it to not block operations.
+	X_shadow = newBasicID("shadow", "SHADOW", true)
+)
+
+// Global variables this package uses because we are a package
+// with global state.
+var (
+	// all is the list of all experiements. Do not modify this.
+	All []ID
+
+	// enabled keeps track of what flags have been enabled
+	enabled     map[string]bool
+	enabledLock sync.Mutex
+
+	// Hidden "experiment" that forces all others to be on without verification
+	x_force = newBasicID("force", "FORCE", false)
+)
+
+func init() {
+	// The list of all experiments, update this when an experiment is added.
+	All = []ID{
+		X_newApply,
+		X_newDestroy,
+		X_shadow,
+		x_force,
+	}
+
+	// Load
+	reload()
+}
+
+// reload is used by tests to reload the global state. This is called by
+// init publicly.
+func reload() {
+	// Initialize
+	enabledLock.Lock()
+	enabled = make(map[string]bool)
+	enabledLock.Unlock()
+
+	// Set defaults and check env vars
+	for _, id := range All {
+		// Get the default value
+		def := id.Default()
+
+		// If we set it in the env var, default it to true
+		key := fmt.Sprintf("TF_X_%s", strings.ToUpper(id.Env()))
+		if v := os.Getenv(key); v != "" {
+			def = v != "0"
+		}
+
+		// Set the default
+		SetEnabled(id, def)
+	}
+}
+
+// Enabled returns whether an experiment has been enabled or not.
+func Enabled(id ID) bool {
+	enabledLock.Lock()
+	defer enabledLock.Unlock()
+	return enabled[id.Flag()]
+}
+
+// SetEnabled sets an experiment to enabled/disabled. Please check with
+// the experiment docs for when calling this actually affects the experiment.
+func SetEnabled(id ID, v bool) {
+	enabledLock.Lock()
+	defer enabledLock.Unlock()
+	enabled[id.Flag()] = v
+}
+
+// Force returns true if the -Xforce of TF_X_FORCE flag is present, which
+// advises users of this package to not verify with the user that they want
+// experimental behavior and to just continue with it.
+func Force() bool {
+	return Enabled(x_force)
+}
+
+// Flag configures the given FlagSet with the flags to configure
+// all active experiments.
+func Flag(fs *flag.FlagSet) {
+	for _, id := range All {
+		desc := id.Flag()
+		key := fmt.Sprintf("X%s", id.Flag())
+		fs.Var(&idValue{X: id}, key, desc)
+	}
+}
+
+// idValue implements flag.Value for setting the enabled/disabled state
+// of an experiment from the CLI.
+type idValue struct {
+	X ID
+}
+
+func (v *idValue) IsBoolFlag() bool { return true }
+func (v *idValue) String() string   { return strconv.FormatBool(Enabled(v.X)) }
+func (v *idValue) Set(raw string) error {
+	b, err := strconv.ParseBool(raw)
+	if err == nil {
+		SetEnabled(v.X, b)
+	}
+
+	return err
+}

helper/experiment/experiment_test.go

@@ -0,0 +1,117 @@
+package experiment
+
+import (
+	"flag"
+	"fmt"
+	"os"
+	"testing"
+)
+
+// Test experiments
+var (
+	X_test1 = newBasicID("test1", "TEST1", false)
+	X_test2 = newBasicID("test2", "TEST2", true)
+)
+
+// Reinitializes the package to a clean slate
+func testReinit() {
+	All = []ID{X_test1, X_test2, x_force}
+	reload()
+}
+
+func init() {
+	testReinit()
+
+	// Clear all env vars so they don't affect tests
+	for _, id := range All {
+		os.Unsetenv(fmt.Sprintf("TF_X_%s", id.Env()))
+	}
+}
+
+func TestDefault(t *testing.T) {
+	testReinit()
+
+	if Enabled(X_test1) {
+		t.Fatal("test1 should not be enabled")
+	}
+
+	if !Enabled(X_test2) {
+		t.Fatal("test2 should be enabled")
+	}
+}
+
+func TestEnv(t *testing.T) {
+	os.Setenv("TF_X_TEST2", "0")
+	defer os.Unsetenv("TF_X_TEST2")
+
+	testReinit()
+
+	if Enabled(X_test2) {
+		t.Fatal("test2 should be enabled")
+	}
+}
+
+func TestFlag(t *testing.T) {
+	testReinit()
+
+	// Verify default
+	if !Enabled(X_test2) {
+		t.Fatal("test2 should be enabled")
+	}
+
+	// Setup a flag set
+	fs := flag.NewFlagSet("test", flag.ContinueOnError)
+	Flag(fs)
+	fs.Parse([]string{"-Xtest2=false"})
+
+	if Enabled(X_test2) {
+		t.Fatal("test2 should not be enabled")
+	}
+}
+
+func TestFlag_overEnv(t *testing.T) {
+	os.Setenv("TF_X_TEST2", "1")
+	defer os.Unsetenv("TF_X_TEST2")
+
+	testReinit()
+
+	// Verify default
+	if !Enabled(X_test2) {
+		t.Fatal("test2 should be enabled")
+	}
+
+	// Setup a flag set
+	fs := flag.NewFlagSet("test", flag.ContinueOnError)
+	Flag(fs)
+	fs.Parse([]string{"-Xtest2=false"})
+
+	if Enabled(X_test2) {
+		t.Fatal("test2 should not be enabled")
+	}
+}
+
+func TestForce(t *testing.T) {
+	os.Setenv("TF_X_FORCE", "1")
+	defer os.Unsetenv("TF_X_FORCE")
+
+	testReinit()
+
+	if !Force() {
+		t.Fatal("should force")
+	}
+}
+
+func TestForce_flag(t *testing.T) {
+	os.Unsetenv("TF_X_FORCE")
+
+	testReinit()
+
+	// Setup a flag set
+	fs := flag.NewFlagSet("test", flag.ContinueOnError)
+	Flag(fs)
+	fs.Parse([]string{"-Xforce"})
+
+	if !Force() {
+		t.Fatal("should force")
+	}
+}

helper/experiment/id.go

@@ -0,0 +1,34 @@
+package experiment
+
+// ID represents an experimental feature.
+//
+// The global vars defined on this package should be used as ID values.
+// This interface is purposely not implement-able outside of this package
+// so that we can rely on the Go compiler to enforce all experiment references.
+type ID interface {
+	Env() string
+	Flag() string
+	Default() bool
+
+	unexported() // So the ID can't be implemented externally.
+}
+
+// basicID implements ID.
+type basicID struct {
+	EnvValue     string
+	FlagValue    string
+	DefaultValue bool
+}
+
+func newBasicID(flag, env string, def bool) ID {
+	return &basicID{
+		EnvValue:     env,
+		FlagValue:    flag,
+		DefaultValue: def,
+	}
+}
+
+func (id *basicID) Env() string   { return id.EnvValue }
+func (id *basicID) Flag() string  { return id.FlagValue }
+func (id *basicID) Default() bool { return id.DefaultValue }
+func (id *basicID) unexported()   {}

terraform/context.go

@@ -11,20 +11,7 @@ import (
 	"github.com/hashicorp/hcl"
 	"github.com/hashicorp/terraform/config"
 	"github.com/hashicorp/terraform/config/module"
-)
+	"github.com/hashicorp/terraform/helper/experiment"
-
-// Variables prefixed with X_ are experimental features. They can be enabled
-// by setting them to true. This should be done before any API is called.
-// These should be expected to be removed at some point in the future; each
-// option should mention a schedule.
-var (
-	// X_newApply will enable the new apply graph. This will be removed
-	// and be on by default in 0.8.0.
-	X_newApply = false
-
-	// X_newDestroy will enable the new destroy graph. This will be removed
-	// and be on by default in 0.8.0.
-	X_newDestroy = false
 )
 
 // InputMode defines what sort of input will be asked for when Input
@@ -55,11 +42,6 @@ var (
 	// Plan operation, effectively testing the Diff DeepCopy whenever
 	// a Plan occurs. This is enabled for tests.
 	contextTestDeepCopyOnPlan = false
-
-	// contextTestShadow will enable the shadow graph for the new graphs.
-	// This is enabled for tests. This will be removed very shortly and
-	// be enabled by default.
-	contextTestShadow = false
 )
 
 // ContextOpts are the user-configurable options to create a context with
@@ -375,6 +357,8 @@ func (c *Context) Apply() (*State, error) {
 	// Copy our own state
 	c.state = c.state.DeepCopy()
 
+	X_newApply := experiment.Enabled(experiment.X_newApply)
+	X_newDestroy := experiment.Enabled(experiment.X_newDestroy)
 	newGraphEnabled := (c.destroy && X_newDestroy) || (!c.destroy && X_newApply)
 
 	// Build the original graph. This is before the new graph builders
@@ -432,12 +416,6 @@ func (c *Context) Apply() (*State, error) {
 		log.Printf("[WARN] terraform: real graph is original, shadow is experiment")
 	}
 
-	// For now, always shadow with the real graph for verification. We don't
-	// want to shadow yet with the new graphs.
-	if !contextTestShadow {
-		shadow = real
-	}
-
 	// Determine the operation
 	operation := walkApply
 	if c.destroy {
@@ -507,6 +485,9 @@ func (c *Context) Plan() (*Plan, error) {
 	c.diff.init()
 	c.diffLock.Unlock()
 
+	// Used throughout below
+	X_newDestroy := experiment.Enabled(experiment.X_newDestroy)
+
 	// Build the graph. We have a branch here since for the pure-destroy
 	// plan (c.destroy) we use a much simpler graph builder that simply
 	// walks the state and reverses edges.
@@ -714,6 +695,11 @@ func (c *Context) walk(
 	// the real work: talking to real providers, modifying real state, etc.
 	realCtx := c
 
+	// If we don't want shadowing, remove it
+	if !experiment.Enabled(experiment.X_shadow) {
+		shadow = nil
+	}
+
 	// If we have a shadow graph, walk that as well
 	var shadowCtx *Context
 	var shadowCloser Shadow

terraform/terraform_test.go

@@ -15,6 +15,7 @@ import (
 	"github.com/hashicorp/go-getter"
 	"github.com/hashicorp/terraform/config"
 	"github.com/hashicorp/terraform/config/module"
+	"github.com/hashicorp/terraform/helper/experiment"
 	"github.com/hashicorp/terraform/helper/logging"
 )
 
@@ -22,19 +23,9 @@ import (
 const fixtureDir = "./test-fixtures"
 
 func TestMain(m *testing.M) {
-	// Experimental features
+	experiment.Flag(flag.CommandLine)
-	xNewApply := flag.Bool("Xnew-apply", false, "Experiment: new apply graph")
-	xNewDestroy := flag.Bool("Xnew-destroy", false, "Experiment: new destroy graph")
-
-	// Normal features
-	shadow := flag.Bool("shadow", true, "Enable shadow graph")
-
 	flag.Parse()
 
-	// Setup experimental features
-	X_newApply = *xNewApply
-	X_newDestroy = *xNewDestroy
-
 	if testing.Verbose() {
 		// if we're verbose, use the logging requested by TF_LOG
 		logging.SetOutput()
@@ -49,9 +40,6 @@ func TestMain(m *testing.M) {
 	// Always DeepCopy the Diff on every Plan during a test
 	contextTestDeepCopyOnPlan = true
 
-	// Shadow the new graphs
-	contextTestShadow = *shadow
-
 	os.Exit(m.Run())
 }