[featureflags] Simplify feature flags (#325)

mikeland73 · web-flow · commit 3c2a20f43e9c · 2022-11-30T14:06:10.000-08:00
## Summary

* Simplifies feature flag lib by using a single const as the name and the feature itself. 
* Removes the need for `init`
* Makes callsites more concise
* Remove `NixpkgVersion` flag

## How was it tested?

unit tests
diff --git a/boxcli/featureflag/feature.go b/boxcli/featureflag/feature.go
@@ -14,15 +14,19 @@ type feature struct {
 
 var features = map[string]*feature{}
 
-func disabled(name string) {
-	features[name] = &feature{name: name}
-}
-
-func enabled(name string) {
-	features[name] = &feature{name: name, enabled: true}
+func disabled(name string) *feature {
+	if features[name] == nil {
+		features[name] = &feature{name: name}
+	}
+	features[name].enabled = false
+	return features[name]
 }
 
-func Get(name string) *feature {
+func enabled(name string) *feature {
+	if features[name] == nil {
+		features[name] = &feature{name: name}
+	}
+	features[name].enabled = true
 	return features[name]
 }
 
diff --git a/boxcli/featureflag/feature_test.go b/boxcli/featureflag/feature_test.go
@@ -3,29 +3,33 @@ package featureflag
 import "testing"
 
 func TestEnabledFeature(t *testing.T) {
-	enabled("TEST")
-	if !Get("TEST").Enabled() {
-		t.Errorf("got Get(%q).Enabled() = false, want true.", "TEST")
+	name := "TestEnabledFeature"
+	enabled(name)
+	if !features[name].Enabled() {
+		t.Errorf("got %s.Enabled() = false, want true.", name)
 	}
 }
 
 func TestDisabledFeature(t *testing.T) {
-	disabled("TEST")
-	if Get("TEST").Enabled() {
-		t.Errorf("got Get(%q).Enabled() = true, want false.", "TEST")
+	name := "TestDisabledFeature"
+	disabled(name)
+	if features[name].Enabled() {
+		t.Errorf("got %s.Enabled() = true, want false.", name)
 	}
 }
 
 func TestEnabledFeatureEnv(t *testing.T) {
-	disabled("TEST")
-	t.Setenv("DEVBOX_FEATURE_TEST", "1")
-	if !Get("TEST").Enabled() {
-		t.Errorf("got Get(%q).Enabled() = false, want true.", "TEST")
+	name := "TestEnabledFeatureEnv"
+	disabled(name)
+	t.Setenv("DEVBOX_FEATURE_"+name, "1")
+	if !features[name].Enabled() {
+		t.Errorf("got %s.Enabled() = false, want true.", name)
 	}
 }
 
 func TestNonExistentFeature(t *testing.T) {
-	if Get("TEST").Enabled() {
-		t.Errorf("got Get(%q).Enabled() = true, want false.", "TEST")
+	name := "TestNonExistentFeature"
+	if features[name].Enabled() {
+		t.Errorf("got %s.Enabled() = true, want false.", name)
 	}
 }
diff --git a/boxcli/featureflag/nixpkgversion.go b/boxcli/featureflag/nixpkgversion.go
diff --git a/boxcli/featureflag/pkgconfig.go b/boxcli/featureflag/pkgconfig.go
@@ -1,7 +1,3 @@
 package featureflag
 
-const PKGConfig = "PKG_CONFIG" // DEVBOX_FEATURE_PKG_CONFIG
-
-func init() {
-	disabled(PKGConfig)
-}
+var PKGConfig = disabled("PKG_CONFIG") // DEVBOX_FEATURE_PKG_CONFIG
diff --git a/boxcli/info.go b/boxcli/info.go
@@ -18,10 +18,9 @@ type infoCmdFlags struct {
 
 func InfoCmd() *cobra.Command {
 	flags := infoCmdFlags{}
-
 	command := &cobra.Command{
 		Use:               "info <pkg>",
-		Hidden:            !featureflag.Get(featureflag.PKGConfig).Enabled(),
+		Hidden:            !featureflag.PKGConfig.Enabled(),
 		Short:             "Display package info",
 		Args:              cobra.ExactArgs(1),
 		PersistentPreRunE: nixShellPersistentPreRunE,
diff --git a/config.go b/config.go
@@ -12,7 +12,6 @@ import (
 	"unicode"
 
 	"github.com/pkg/errors"
-	"go.jetpack.io/devbox/boxcli/featureflag"
 	"go.jetpack.io/devbox/boxcli/usererr"
 	"go.jetpack.io/devbox/cuecfg"
 	"go.jetpack.io/devbox/debug"
@@ -76,7 +75,7 @@ func ReadConfig(path string) (*Config, error) {
 }
 
 func upgradeConfig(cfg *Config, absFilePath string) error {
-	if cfg.Nixpkgs.Commit == "" && featureflag.Get(featureflag.NixpkgVersion).Enabled() {
+	if cfg.Nixpkgs.Commit == "" {
 		// For now, we add the hardcoded value corresponding to the commit hash as of 2022-08-16 in:
 		// `git ls-remote https://github.com/nixos/nixpkgs nixos-unstable`
 		// In the near future, this will be changed to the commit-hash of the unstable tag in nixpkgs github repository
diff --git a/devbox.go b/devbox.go
@@ -40,11 +40,10 @@ const (
 func InitConfig(dir string) (created bool, err error) {
 	cfgPath := filepath.Join(dir, configFilename)
 
-	config := &Config{}
-	if featureflag.Get(featureflag.NixpkgVersion).Enabled() {
-		config.Nixpkgs = NixpkgsConfig{
+	config := &Config{
+		Nixpkgs: NixpkgsConfig{
 			Commit: plansdk.DefaultNixpkgsCommit,
-		}
+		},
 	}
 	return cuecfg.InitFile(cfgPath, config)
 }
@@ -115,7 +114,7 @@ func (d *Devbox) Add(pkgs ...string) error {
 	if err := d.ensurePackagesAreInstalled(install); err != nil {
 		return err
 	}
-	if featureflag.Get(featureflag.PKGConfig).Enabled() {
+	if featureflag.PKGConfig.Enabled() {
 		for _, pkg := range pkgs {
 			if err := pkgcfg.PrintReadme(pkg, d.configDir, d.writer, IsDevboxShellEnabled()); err != nil {
 				return err
@@ -147,7 +146,7 @@ func (d *Devbox) Remove(pkgs ...string) error {
 		return err
 	}
 
-	if featureflag.Get(featureflag.PKGConfig).Enabled() {
+	if featureflag.PKGConfig.Enabled() {
 		if err := pkgcfg.Remove(d.configDir, uninstalledPackages); err != nil {
 			return err
 		}
@@ -248,7 +247,7 @@ func (d *Devbox) Shell() error {
 		nix.WithConfigDir(d.configDir),
 	}
 
-	if featureflag.Get(featureflag.PKGConfig).Enabled() {
+	if featureflag.PKGConfig.Enabled() {
 		env, err := pkgcfg.Env(plan.DevPackages, d.configDir)
 		if err != nil {
 			return err
@@ -458,7 +457,7 @@ func (d *Devbox) ensurePackagesAreInstalled(mode installMode) error {
 	}
 	fmt.Println("done.")
 
-	if featureflag.Get(featureflag.PKGConfig).Enabled() {
+	if featureflag.PKGConfig.Enabled() {
 		if err := pkgcfg.RemoveInvalidSymlinks(d.configDir); err != nil {
 			return err
 		}
diff --git a/generate.go b/generate.go
@@ -50,7 +50,7 @@ func generateForShell(rootPath string, plan *plansdk.ShellPlan) error {
 		}
 	}
 
-	if featureflag.Get(featureflag.PKGConfig).Enabled() {
+	if featureflag.PKGConfig.Enabled() {
 		for _, pkg := range plan.DevPackages {
 			if err := pkgcfg.CreateFilesAndShowReadme(pkg, rootPath); err != nil {
 				return err
diff --git a/planner/plansdk/plansdk.go b/planner/plansdk/plansdk.go
@@ -10,7 +10,6 @@ import (
 
 	"github.com/imdario/mergo"
 	"github.com/pkg/errors"
-	"go.jetpack.io/devbox/boxcli/featureflag"
 	"go.jetpack.io/devbox/pkgslice"
 )
 
@@ -223,18 +222,6 @@ type NixpkgsInfo struct {
 const DefaultNixpkgsCommit = "af9e00071d0971eb292fd5abef334e66eda3cb69"
 
 func GetNixpkgsInfo(commitHash string) (*NixpkgsInfo, error) {
-
-	// If the featureflag is OFF, then we fallback to the hardcoded commit
-	// and ignore any value set in the devbox.json
-	if !featureflag.Get(featureflag.NixpkgVersion).Enabled() {
-		// sha256 from:
-		// nix-prefetch-url --unpack  https://github.com/nixos/nixpkgs/archive/<commit-hash>.tar.gz
-		return &NixpkgsInfo{
-			URL:    fmt.Sprintf("https://github.com/nixos/nixpkgs/archive/%s.tar.gz", DefaultNixpkgsCommit),
-			Sha256: "1mdwy0419m5i9ss6s5frbhgzgyccbwycxm5nal40c8486bai0hwy",
-		}, nil
-	}
-
 	return &NixpkgsInfo{
 		URL: fmt.Sprintf("https://github.com/nixos/nixpkgs/archive/%s.tar.gz", commitHash),
 	}, nil

Original file line number	Diff line number	Diff line change
`@@ -3,29 +3,33 @@ package featureflag`
`3`	`3`	`import "testing"`
`4`	`4`
`5`	`5`	`func TestEnabledFeature(t *testing.T) {`
`6`		`- enabled("TEST")`
`7`		`- if !Get("TEST").Enabled() {`
`8`		`- t.Errorf("got Get(%q).Enabled() = false, want true.", "TEST")`
	`6`	`+ name := "TestEnabledFeature"`
	`7`	`+ enabled(name)`
	`8`	`+ if !features[name].Enabled() {`
	`9`	`+ t.Errorf("got %s.Enabled() = false, want true.", name)`
`9`	`10`	`}`
`10`	`11`	`}`
`11`	`12`
`12`	`13`	`func TestDisabledFeature(t *testing.T) {`
`13`		`- disabled("TEST")`
`14`		`- if Get("TEST").Enabled() {`
`15`		`- t.Errorf("got Get(%q).Enabled() = true, want false.", "TEST")`
	`14`	`+ name := "TestDisabledFeature"`
	`15`	`+ disabled(name)`
	`16`	`+ if features[name].Enabled() {`
	`17`	`+ t.Errorf("got %s.Enabled() = true, want false.", name)`
`16`	`18`	`}`
`17`	`19`	`}`
`18`	`20`
`19`	`21`	`func TestEnabledFeatureEnv(t *testing.T) {`
`20`		`- disabled("TEST")`
`21`		`- t.Setenv("DEVBOX_FEATURE_TEST", "1")`
`22`		`- if !Get("TEST").Enabled() {`
`23`		`- t.Errorf("got Get(%q).Enabled() = false, want true.", "TEST")`
	`22`	`+ name := "TestEnabledFeatureEnv"`
	`23`	`+ disabled(name)`
	`24`	`+ t.Setenv("DEVBOX_FEATURE_"+name, "1")`
	`25`	`+ if !features[name].Enabled() {`
	`26`	`+ t.Errorf("got %s.Enabled() = false, want true.", name)`
`24`	`27`	`}`
`25`	`28`	`}`
`26`	`29`
`27`	`30`	`func TestNonExistentFeature(t *testing.T) {`
`28`		`- if Get("TEST").Enabled() {`
`29`		`- t.Errorf("got Get(%q).Enabled() = true, want false.", "TEST")`
	`31`	`+ name := "TestNonExistentFeature"`
	`32`	`+ if features[name].Enabled() {`
	`33`	`+ t.Errorf("got %s.Enabled() = true, want false.", name)`
`30`	`34`	`}`
`31`	`35`	`}`
Original file line number	Diff line number	Diff line change
`@@ -40,11 +40,10 @@ const (`
`40`	`40`	`func InitConfig(dir string) (created bool, err error) {`
`41`	`41`	`cfgPath := filepath.Join(dir, configFilename)`
`42`	`42`
`43`		`- config := &Config{}`
`44`		`- if featureflag.Get(featureflag.NixpkgVersion).Enabled() {`
`45`		`- config.Nixpkgs = NixpkgsConfig{`
	`43`	`+ config := &Config{`
	`44`	`+ Nixpkgs: NixpkgsConfig{`
`46`	`45`	`Commit: plansdk.DefaultNixpkgsCommit,`
`47`		`- }`
	`46`	`+ },`
`48`	`47`	`}`
`49`	`48`	`return cuecfg.InitFile(cfgPath, config)`
`50`	`49`	`}`
`@@ -115,7 +114,7 @@ func (d *Devbox) Add(pkgs ...string) error {`
`115`	`114`	`if err := d.ensurePackagesAreInstalled(install); err != nil {`
`116`	`115`	`return err`
`117`	`116`	`}`
`118`		`- if featureflag.Get(featureflag.PKGConfig).Enabled() {`
	`117`	`+ if featureflag.PKGConfig.Enabled() {`
`119`	`118`	`for _, pkg := range pkgs {`
`120`	`119`	`if err := pkgcfg.PrintReadme(pkg, d.configDir, d.writer, IsDevboxShellEnabled()); err != nil {`
`121`	`120`	`return err`
`@@ -147,7 +146,7 @@ func (d *Devbox) Remove(pkgs ...string) error {`
`147`	`146`	`return err`
`148`	`147`	`}`
`149`	`148`
`150`		`- if featureflag.Get(featureflag.PKGConfig).Enabled() {`
	`149`	`+ if featureflag.PKGConfig.Enabled() {`
`151`	`150`	`if err := pkgcfg.Remove(d.configDir, uninstalledPackages); err != nil {`
`152`	`151`	`return err`
`153`	`152`	`}`
`@@ -248,7 +247,7 @@ func (d *Devbox) Shell() error {`
`248`	`247`	`nix.WithConfigDir(d.configDir),`
`249`	`248`	`}`
`250`	`249`
`251`		`- if featureflag.Get(featureflag.PKGConfig).Enabled() {`
	`250`	`+ if featureflag.PKGConfig.Enabled() {`
`252`	`251`	`env, err := pkgcfg.Env(plan.DevPackages, d.configDir)`
`253`	`252`	`if err != nil {`
`254`	`253`	`return err`
`@@ -458,7 +457,7 @@ func (d *Devbox) ensurePackagesAreInstalled(mode installMode) error {`
`458`	`457`	`}`
`459`	`458`	`fmt.Println("done.")`
`460`	`459`
`461`		`- if featureflag.Get(featureflag.PKGConfig).Enabled() {`
	`460`	`+ if featureflag.PKGConfig.Enabled() {`
`462`	`461`	`if err := pkgcfg.RemoveInvalidSymlinks(d.configDir); err != nil {`
`463`	`462`	`return err`
`464`	`463`	`}`
Original file line number	Diff line number	Diff line change
`@@ -50,7 +50,7 @@ func generateForShell(rootPath string, plan *plansdk.ShellPlan) error {`
`50`	`50`	`}`
`51`	`51`	`}`
`52`	`52`
`53`		`- if featureflag.Get(featureflag.PKGConfig).Enabled() {`
	`53`	`+ if featureflag.PKGConfig.Enabled() {`
`54`	`54`	`for _, pkg := range plan.DevPackages {`
`55`	`55`	`if err := pkgcfg.CreateFilesAndShowReadme(pkg, rootPath); err != nil {`
`56`	`56`	`return err`