From af82be19eac166144e677f382a13f4eb01bdda29 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Wed, 26 Oct 2016 15:46:22 -0400 Subject: [PATCH] helper/experiment: a helper for setting, making experiments This creates a standard package and interface for defining, querying, setting experiments (`-X` flags). I expect we'll want to continue to introduce various features behind experimental flags. I want to make doing this as easy as possible and I want to make _removing_ experiments as easy as possible as well. The goal with this packge has been to rely on the compiler enforcing our experiment references as much as possible. This means that every experiment is a global variable that must be referenced directly, so when it is removed you'll get compiler errors where the experiment is referenced. This also unifies and makes it easy to grab CLI flags to enable/disable experiments as well as env vars! This way defining an experiment is just a couple lines of code (documented on the package). --- command/apply.go | 5 +- command/meta.go | 4 +- helper/experiment/experiment.go | 162 +++++++++++++++++++++++++++ helper/experiment/experiment_test.go | 117 +++++++++++++++++++ helper/experiment/id.go | 34 ++++++ terraform/context.go | 36 ++---- terraform/terraform_test.go | 16 +-- 7 files changed, 331 insertions(+), 43 deletions(-) create mode 100644 helper/experiment/experiment.go create mode 100644 helper/experiment/experiment_test.go create mode 100644 helper/experiment/id.go diff --git a/command/apply.go b/command/apply.go index 2b40eb947..1f353593a 100644 --- a/command/apply.go +++ b/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." diff --git a/command/meta.go b/command/meta.go index b8c419c1e..bd2016564 100644 --- a/command/meta.go +++ b/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") - f.BoolVar(&terraform.X_newDestroy, "Xnew-destroy", false, "experiment: new destroy") + experiment.Flag(f) // 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 diff --git a/helper/experiment/experiment.go b/helper/experiment/experiment.go new file mode 100644 index 000000000..1a49c4d27 --- /dev/null +++ b/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_` 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 +} diff --git a/helper/experiment/experiment_test.go b/helper/experiment/experiment_test.go new file mode 100644 index 000000000..32055c2c8 --- /dev/null +++ b/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") + } +} diff --git a/helper/experiment/id.go b/helper/experiment/id.go new file mode 100644 index 000000000..8e2f70732 --- /dev/null +++ b/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() {} diff --git a/terraform/context.go b/terraform/context.go index 176c1e182..80bdf2d50 100644 --- a/terraform/context.go +++ b/terraform/context.go @@ -11,20 +11,7 @@ import ( "github.com/hashicorp/hcl" "github.com/hashicorp/terraform/config" "github.com/hashicorp/terraform/config/module" -) - -// 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 + "github.com/hashicorp/terraform/helper/experiment" ) // 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 diff --git a/terraform/terraform_test.go b/terraform/terraform_test.go index bc01b68d6..ae5db9172 100644 --- a/terraform/terraform_test.go +++ b/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 - 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") - + experiment.Flag(flag.CommandLine) 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()) }