From 5b89458503e15651b2db99789119cdad66f14e8b Mon Sep 17 00:00:00 2001 From: John Ryan Date: Fri, 29 Apr 2022 11:14:23 -0700 Subject: [PATCH 1/3] Introduce feature flags to control experiments --- cmd/ytt/ytt.go | 12 ++++++ pkg/feature/flags.go | 81 +++++++++++++++++++++++++++++++++++++++ pkg/feature/flags_test.go | 29 ++++++++++++++ 3 files changed, 122 insertions(+) create mode 100644 pkg/feature/flags.go create mode 100644 pkg/feature/flags_test.go diff --git a/cmd/ytt/ytt.go b/cmd/ytt/ytt.go index ab0eb966..63f188fd 100644 --- a/cmd/ytt/ytt.go +++ b/cmd/ytt/ytt.go @@ -7,14 +7,17 @@ import ( "fmt" "math/rand" "os" + "strings" "time" uierrs "github.com/cppforlife/go-cli-ui/errors" "github.com/vmware-tanzu/carvel-ytt/pkg/cmd" + "github.com/vmware-tanzu/carvel-ytt/pkg/feature" ) func main() { rand.Seed(time.Now().UTC().UnixNano()) + enableExperimentalFeatures() command := cmd.NewDefaultYttCmd() @@ -24,3 +27,12 @@ func main() { os.Exit(1) } } + +func enableExperimentalFeatures() { + if experiments, isSet := os.LookupEnv("YTTEXPERIMENTS"); isSet { + for _, experiment := range strings.Split(experiments, ",") { + feature.Flags().Enable(experiment) + fmt.Fprintf(os.Stderr, "Experimental feature %q enabled.\n", experiment) + } + } +} diff --git a/pkg/feature/flags.go b/pkg/feature/flags.go new file mode 100644 index 00000000..433eb10f --- /dev/null +++ b/pkg/feature/flags.go @@ -0,0 +1,81 @@ +// Copyright 2022 VMware, Inc. +// SPDX-License-Identifier: Apache-2.0 + +/* +Package feature provides a global "Feature Flag" facility used to toggle entire features on or off. + +To "register" a new feature, +1. add a new constant +2. add that constant to the `allFeatures` slice + +Initialize the flags, enabling desired features: + + feature.Flags().Enable() + +To then circuit-break functionality behind a feature flag. + + if feature.Flags().IsEnabled() { + ... + } +*/ +package feature + +import "fmt" + +// Names of features that can be toggled +const ( + Noop = "noop" +) + +// allFeatures is the total list of features. It must contain all the constants defined, above. +var allFeatures = []string{Noop} + +// Flags returns the singleton instance of feature flags. +func Flags() *Flagset { + // NOT thread-safe. + if instance == nil { + instance = newFlagSet() + } + return instance +} + +// Flagset is a collection of flags. +type Flagset struct { + flags map[string]bool +} + +// Enable toggles the named feature on. +// Subsequent calls to IsEnabled() for that same named feature will return true. +// +// Once this Flagset has been frozen, panics. +func (f *Flagset) Enable(name string) *Flagset { + f.ensureExists(name) + f.flags[name] = true + return f +} + +// IsEnabled reports whether the named feature has been enabled. +// +// If the feature has been explicitly enabled, returns true. +// Otherwise, returns false. +func (f *Flagset) IsEnabled(name string) bool { + f.ensureExists(name) + return f.flags[name] +} + +func (f *Flagset) ensureExists(name string) { + _, ok := f.flags[name] + if !ok { + panic(fmt.Sprintf("Unknown feature %q; known features are %q", name, allFeatures)) + } +} + +func newFlagSet() *Flagset { + flagset := &Flagset{flags: make(map[string]bool, len(allFeatures))} + for _, feature := range allFeatures { + flagset.flags[feature] = false + } + return flagset +} + +var instance = newFlagSet() diff --git a/pkg/feature/flags_test.go b/pkg/feature/flags_test.go new file mode 100644 index 00000000..bd12fea4 --- /dev/null +++ b/pkg/feature/flags_test.go @@ -0,0 +1,29 @@ +// Copyright 2022 VMware, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package feature + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +/* +At runtime, there is a singleton instance of feature flags. +This test suite verifies the behavior of of such an instance. +To avoid test pollution, a fresh instance is created in each test. +*/ + +func TestFeaturesAreDisabledByDefault(t *testing.T) { + assert.False(t, newFlagSet().IsEnabled(Noop)) +} + +func TestFeaturesCanBeEnabled(t *testing.T) { + flagset := newFlagSet().Enable(Noop) + assert.True(t, flagset.IsEnabled(Noop)) +} + +func TestPanicsWhenFeatureIsUnknown(t *testing.T) { + assert.Panics(t, func() { newFlagSet().Enable("not-a-feature") }) +} From d739fccfb50f569c16e8eda16d31f722c0eef2f8 Mon Sep 17 00:00:00 2001 From: John Ryan Date: Tue, 3 May 2022 08:16:16 -0700 Subject: [PATCH 2/3] Feature flag assertion validations --- cmd/ytt/ytt.go | 3 ++- pkg/cmd/template/cmd_test.go | 13 +++++++++++++ pkg/feature/flags.go | 5 +++-- pkg/validations/assert.go | 5 +++++ 4 files changed, 23 insertions(+), 3 deletions(-) diff --git a/cmd/ytt/ytt.go b/cmd/ytt/ytt.go index 63f188fd..9cf1b3dd 100644 --- a/cmd/ytt/ytt.go +++ b/cmd/ytt/ytt.go @@ -29,7 +29,8 @@ func main() { } func enableExperimentalFeatures() { - if experiments, isSet := os.LookupEnv("YTTEXPERIMENTS"); isSet { + if experiments, isSet := os.LookupEnv("YTTEXPERIMENTS"); isSet && experiments != "" { + // if we didn't ignore empty string, we'd attempt to Enable(""); that would panic (rather than ignore). for _, experiment := range strings.Split(experiments, ",") { feature.Flags().Enable(experiment) fmt.Fprintf(os.Stderr, "Experimental feature %q enabled.\n", experiment) diff --git a/pkg/cmd/template/cmd_test.go b/pkg/cmd/template/cmd_test.go index 9965afe3..ceed03b0 100644 --- a/pkg/cmd/template/cmd_test.go +++ b/pkg/cmd/template/cmd_test.go @@ -4,15 +4,28 @@ package template_test import ( + "os" "testing" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" cmdtpl "github.com/vmware-tanzu/carvel-ytt/pkg/cmd/template" "github.com/vmware-tanzu/carvel-ytt/pkg/cmd/ui" + "github.com/vmware-tanzu/carvel-ytt/pkg/feature" "github.com/vmware-tanzu/carvel-ytt/pkg/files" ) +// TestMain is invoked when any tests are run in this package, *instead of* those tests being run directly. +// This allows for setup to occur before *any* test is run. +func TestMain(m *testing.M) { + // Enable experimental code + feature.Flags().Enable(feature.Validations) + + exitVal := m.Run() // execute the specified tests + + os.Exit(exitVal) // required in order to properly report the error level when tests fail. +} + func TestLoad(t *testing.T) { yamlTplData := []byte(` #@ load("@ytt:data", "data") diff --git a/pkg/feature/flags.go b/pkg/feature/flags.go index 433eb10f..68d9ff56 100644 --- a/pkg/feature/flags.go +++ b/pkg/feature/flags.go @@ -24,11 +24,12 @@ import "fmt" // Names of features that can be toggled const ( - Noop = "noop" + Noop = "noop" + Validations = "validations" ) // allFeatures is the total list of features. It must contain all the constants defined, above. -var allFeatures = []string{Noop} +var allFeatures = []string{Noop, Validations} // Flags returns the singleton instance of feature flags. func Flags() *Flagset { diff --git a/pkg/validations/assert.go b/pkg/validations/assert.go index 57d301d8..d8871d09 100644 --- a/pkg/validations/assert.go +++ b/pkg/validations/assert.go @@ -7,6 +7,7 @@ import ( "fmt" "github.com/k14s/starlark-go/starlark" + "github.com/vmware-tanzu/carvel-ytt/pkg/feature" "github.com/vmware-tanzu/carvel-ytt/pkg/template" "github.com/vmware-tanzu/carvel-ytt/pkg/yamlmeta" ) @@ -23,9 +24,13 @@ const ( // When a Node's value is invalid, the errors are collected and returned in an AssertCheck. // Otherwise, returns empty AssertCheck and nil error. func ProcessAndRunValidations(n yamlmeta.Node, threadName string) (AssertCheck, error) { + if !feature.Flags().IsEnabled(feature.Validations) { + return AssertCheck{}, nil + } if n == nil { return AssertCheck{}, nil } + err := yamlmeta.Walk(n, &convertAssertAnnsToValidations{}) if err != nil { return AssertCheck{}, err From 5cd8e2e1497be4df213d7e60453e7e52843f676b Mon Sep 17 00:00:00 2001 From: John Ryan Date: Tue, 10 May 2022 16:30:54 -0700 Subject: [PATCH 3/3] Only allow enabling experiments through env var - rename package from "feature" to "experiments" - report enabled experiments in "version" command output --- cmd/ytt/ytt.go | 13 ---- pkg/cmd/template/cmd_test.go | 6 +- pkg/cmd/version.go | 5 +- pkg/experiments/flags.go | 88 ++++++++++++++++++++++ pkg/{feature => experiments}/flags_test.go | 18 ++--- pkg/feature/flags.go | 82 -------------------- pkg/validations/assert.go | 4 +- 7 files changed, 106 insertions(+), 110 deletions(-) create mode 100644 pkg/experiments/flags.go rename pkg/{feature => experiments}/flags_test.go (53%) delete mode 100644 pkg/feature/flags.go diff --git a/cmd/ytt/ytt.go b/cmd/ytt/ytt.go index 9cf1b3dd..ab0eb966 100644 --- a/cmd/ytt/ytt.go +++ b/cmd/ytt/ytt.go @@ -7,17 +7,14 @@ import ( "fmt" "math/rand" "os" - "strings" "time" uierrs "github.com/cppforlife/go-cli-ui/errors" "github.com/vmware-tanzu/carvel-ytt/pkg/cmd" - "github.com/vmware-tanzu/carvel-ytt/pkg/feature" ) func main() { rand.Seed(time.Now().UTC().UnixNano()) - enableExperimentalFeatures() command := cmd.NewDefaultYttCmd() @@ -27,13 +24,3 @@ func main() { os.Exit(1) } } - -func enableExperimentalFeatures() { - if experiments, isSet := os.LookupEnv("YTTEXPERIMENTS"); isSet && experiments != "" { - // if we didn't ignore empty string, we'd attempt to Enable(""); that would panic (rather than ignore). - for _, experiment := range strings.Split(experiments, ",") { - feature.Flags().Enable(experiment) - fmt.Fprintf(os.Stderr, "Experimental feature %q enabled.\n", experiment) - } - } -} diff --git a/pkg/cmd/template/cmd_test.go b/pkg/cmd/template/cmd_test.go index ceed03b0..42539140 100644 --- a/pkg/cmd/template/cmd_test.go +++ b/pkg/cmd/template/cmd_test.go @@ -11,15 +11,15 @@ import ( "github.com/stretchr/testify/require" cmdtpl "github.com/vmware-tanzu/carvel-ytt/pkg/cmd/template" "github.com/vmware-tanzu/carvel-ytt/pkg/cmd/ui" - "github.com/vmware-tanzu/carvel-ytt/pkg/feature" + "github.com/vmware-tanzu/carvel-ytt/pkg/experiments" "github.com/vmware-tanzu/carvel-ytt/pkg/files" ) // TestMain is invoked when any tests are run in this package, *instead of* those tests being run directly. // This allows for setup to occur before *any* test is run. func TestMain(m *testing.M) { - // Enable experimental code - feature.Flags().Enable(feature.Validations) + experiments.ResetForTesting() + os.Setenv(experiments.Env, "validations") exitVal := m.Run() // execute the specified tests diff --git a/pkg/cmd/version.go b/pkg/cmd/version.go index d2f4eb8b..6a700791 100644 --- a/pkg/cmd/version.go +++ b/pkg/cmd/version.go @@ -7,6 +7,7 @@ import ( "fmt" "github.com/spf13/cobra" + "github.com/vmware-tanzu/carvel-ytt/pkg/experiments" "github.com/vmware-tanzu/carvel-ytt/pkg/version" ) @@ -27,6 +28,8 @@ func NewVersionCmd(o *VersionOptions) *cobra.Command { func (o *VersionOptions) Run() error { fmt.Printf("ytt version %s\n", version.Version) - + for _, experiment := range experiments.GetEnabled() { + fmt.Printf("- experiment %q enabled.\n", experiment) + } return nil } diff --git a/pkg/experiments/flags.go b/pkg/experiments/flags.go new file mode 100644 index 00000000..a72ac108 --- /dev/null +++ b/pkg/experiments/flags.go @@ -0,0 +1,88 @@ +// Copyright 2022 VMware, Inc. +// SPDX-License-Identifier: Apache-2.0 + +/* +Package experiments provides a global "Feature Flag" facility for +circuit-breaking pre-GA code. + +The intent is to provide a means of selecting a flavor of executable at +boot; not to toggle experiments on and off. Once settings are loaded from +the environment variable experiments.Env, they are fixed. + +Registering a New Experiment + +1. implement a getter on this package `IsEnabled()` and add the experiment to GetEnabled() + +2. circuit-break functionality behind that check: + + if experiments.IsEnabled() { + ... + } + +3. in tests, enable experiment(s) by setting the environment variable: + + experiments.ResetForTesting() + os.Setenv(experiments.Env, ",,...") + +*/ +package experiments + +import ( + "os" + "strings" +) + +// Env is the OS environment variable with comma-separated names of experiments to enable. +const Env = "YTTEXPERIMENTS" + +// IsValidationsEnabled reports whether the "validations" experiment was enabled by the user (via the Env). +func IsValidationsEnabled() bool { + return isSet("validations") +} + +// GetEnabled reports the name of all enabled experiments. +// +// An experiment is enabled by including its name in the OS environment variable named Env. +func GetEnabled() []string { + experiments := []string{} + if IsValidationsEnabled() { + experiments = append(experiments, "validations") + } + + return experiments +} + +func isSet(flag string) bool { + for _, setting := range getSettings() { + if setting == flag { + return true + } + } + return false +} + +func getSettings() []string { + if settings == nil { + for _, setting := range strings.Split(os.Getenv(Env), ",") { + settings = append(settings, strings.ToLower(strings.TrimSpace(setting))) + } + } + return settings +} + +// settings cached copy of name of experiments that are enabled (cleaned up). +var settings []string + +// isNoopEnabled reports whether the "noop" experiment was enabled. +// +// This is for testing purposes only. +func isNoopEnabled() bool { + return isSet("noop") +} + +// ResetForTesting clears the experiment flag settings, forcing reload from the Env on next use. +// +// This is for testing purposes only. +func ResetForTesting() { + settings = nil +} diff --git a/pkg/feature/flags_test.go b/pkg/experiments/flags_test.go similarity index 53% rename from pkg/feature/flags_test.go rename to pkg/experiments/flags_test.go index bd12fea4..2b7eab6d 100644 --- a/pkg/feature/flags_test.go +++ b/pkg/experiments/flags_test.go @@ -1,29 +1,29 @@ // Copyright 2022 VMware, Inc. // SPDX-License-Identifier: Apache-2.0 -package feature +package experiments import ( + "os" "testing" "github.com/stretchr/testify/assert" ) /* -At runtime, there is a singleton instance of feature flags. +At runtime, there is a singleton instance of experiments flags. This test suite verifies the behavior of of such an instance. To avoid test pollution, a fresh instance is created in each test. */ func TestFeaturesAreDisabledByDefault(t *testing.T) { - assert.False(t, newFlagSet().IsEnabled(Noop)) + ResetForTesting() + os.Setenv(Env, "") + assert.False(t, isNoopEnabled()) } func TestFeaturesCanBeEnabled(t *testing.T) { - flagset := newFlagSet().Enable(Noop) - assert.True(t, flagset.IsEnabled(Noop)) -} - -func TestPanicsWhenFeatureIsUnknown(t *testing.T) { - assert.Panics(t, func() { newFlagSet().Enable("not-a-feature") }) + ResetForTesting() + os.Setenv(Env, "noop") + assert.True(t, isNoopEnabled()) } diff --git a/pkg/feature/flags.go b/pkg/feature/flags.go deleted file mode 100644 index 68d9ff56..00000000 --- a/pkg/feature/flags.go +++ /dev/null @@ -1,82 +0,0 @@ -// Copyright 2022 VMware, Inc. -// SPDX-License-Identifier: Apache-2.0 - -/* -Package feature provides a global "Feature Flag" facility used to toggle entire features on or off. - -To "register" a new feature, -1. add a new constant -2. add that constant to the `allFeatures` slice - -Initialize the flags, enabling desired features: - - feature.Flags().Enable() - -To then circuit-break functionality behind a feature flag. - - if feature.Flags().IsEnabled() { - ... - } -*/ -package feature - -import "fmt" - -// Names of features that can be toggled -const ( - Noop = "noop" - Validations = "validations" -) - -// allFeatures is the total list of features. It must contain all the constants defined, above. -var allFeatures = []string{Noop, Validations} - -// Flags returns the singleton instance of feature flags. -func Flags() *Flagset { - // NOT thread-safe. - if instance == nil { - instance = newFlagSet() - } - return instance -} - -// Flagset is a collection of flags. -type Flagset struct { - flags map[string]bool -} - -// Enable toggles the named feature on. -// Subsequent calls to IsEnabled() for that same named feature will return true. -// -// Once this Flagset has been frozen, panics. -func (f *Flagset) Enable(name string) *Flagset { - f.ensureExists(name) - f.flags[name] = true - return f -} - -// IsEnabled reports whether the named feature has been enabled. -// -// If the feature has been explicitly enabled, returns true. -// Otherwise, returns false. -func (f *Flagset) IsEnabled(name string) bool { - f.ensureExists(name) - return f.flags[name] -} - -func (f *Flagset) ensureExists(name string) { - _, ok := f.flags[name] - if !ok { - panic(fmt.Sprintf("Unknown feature %q; known features are %q", name, allFeatures)) - } -} - -func newFlagSet() *Flagset { - flagset := &Flagset{flags: make(map[string]bool, len(allFeatures))} - for _, feature := range allFeatures { - flagset.flags[feature] = false - } - return flagset -} - -var instance = newFlagSet() diff --git a/pkg/validations/assert.go b/pkg/validations/assert.go index d8871d09..16a13ee3 100644 --- a/pkg/validations/assert.go +++ b/pkg/validations/assert.go @@ -7,7 +7,7 @@ import ( "fmt" "github.com/k14s/starlark-go/starlark" - "github.com/vmware-tanzu/carvel-ytt/pkg/feature" + "github.com/vmware-tanzu/carvel-ytt/pkg/experiments" "github.com/vmware-tanzu/carvel-ytt/pkg/template" "github.com/vmware-tanzu/carvel-ytt/pkg/yamlmeta" ) @@ -24,7 +24,7 @@ const ( // When a Node's value is invalid, the errors are collected and returned in an AssertCheck. // Otherwise, returns empty AssertCheck and nil error. func ProcessAndRunValidations(n yamlmeta.Node, threadName string) (AssertCheck, error) { - if !feature.Flags().IsEnabled(feature.Validations) { + if !experiments.IsValidationsEnabled() { return AssertCheck{}, nil } if n == nil {