v1.5.0: Permit required ownership of flags (#62)

Add support for tagging of ownership of a specific feature flag.

This works either through the presence of a default ownership file (`testtrack/owners.yml`) or a file specified via an ENV variable (`TESTTRACK_OWNERSHIP_FILE`).

If the file exists, `--owner` is required to be set and to match one of the owners in the file.

If the file does not exist, `--owner` is required to be blank.

Author: Jesse Proudman

Makefile

@@ -1,6 +1,6 @@
 SHELL = /bin/sh
 
-VERSION=1.4.0
+VERSION=1.5.0
 BUILD=`git rev-parse HEAD`
 
 LDFLAGS=-ldflags "-w -s \
@@ -38,7 +38,7 @@ test:
 			then\
 				echo "Style violations found. Run the following command to fix:";\
 				echo;\
-				echo "goimports -w" $$GOIMPORTS_RESULT;\
+				echo "$$(go env GOPATH)/bin/goimports -w" $$GOIMPORTS_RESULT;\
 				echo;\
 				exit 1;\
 			fi

README.md

@@ -88,6 +88,18 @@ Run `testtrack help` for more documentation on how to configure splits and other
 
 Happy TestTracking!
 
+## Additional Configuration
+
+The following configuration options are available:
+
+### Split ownership
+If you have a large organization, you may wish to tag ownership of splits to a specific team to help provide accountability for clean up. This is supported natively in test_track.
+
+1. You must specify an ownership file. The default file exists at `testtrack/owners.yml` though that can be overwritten with the TESTTRACK_OWNERSHIP_FILE environment variable.
+2. This file should contain a list of team names, one per line. Any sub-values of the key names will be ignored for the purposes of test track.
+3. If the test track client is able to find this file, it will require an `--owner` flag be set when creating new splits and experiements.
+4. This data will be passed to the test track server where it can be recorded on the split records
+
 ## How to Contribute
 
 We would love for you to contribute! Anything that benefits the majority of TestTrack users—from a documentation fix to an entirely new feature—is encouraged.

cmds/create_experiment.go

@@ -29,9 +29,10 @@ destroyed split if it was destroyed by mistake, but the migration will fail if
 you attempt to create a new split without a prefix.
 `
 
-var createExperimentWeights string
+var createExperimentWeights, createExperimentOwner string
 
 func init() {
+	createExperimentCmd.Flags().StringVar(&createExperimentOwner, "owner", "", "Who owns this feature flag?")
 	createExperimentCmd.Flags().StringVar(&createExperimentWeights, "weights", "control: 50, treatment: 50", "Variant weights to use")
 	createExperimentCmd.Flags().BoolVar(&noPrefix, "no-prefix", false, "Don't prefix experiment with app_name (supports existing legacy splits)")
 	createCmd.AddCommand(createExperimentCmd)
@@ -43,11 +44,11 @@ var createExperimentCmd = &cobra.Command{
 	Long:  createExperimentDoc,
 	Args:  cobra.ExactArgs(1),
 	RunE: func(cmd *cobra.Command, args []string) error {
-		return createExperiment(args[0], createExperimentWeights)
+		return createExperiment(args[0], createExperimentWeights, createExperimentOwner)
 	},
 }
 
-func createExperiment(name, weights string) error {
+func createExperiment(name, weights string, owner string) error {
 	schema, err := schema.Read()
 	if err != nil {
 		return err
@@ -63,6 +64,11 @@ func createExperiment(name, weights string) error {
 		return err
 	}
 
+	err = validations.ValidateOwnerName(owner)
+	if err != nil {
+		return err
+	}
+
 	err = validations.AutoPrefixAndValidateSplit("name", &name, appName, schema, noPrefix, force)
 	if err != nil {
 		// if this errors, we know this is a create (not an update), so maybe prefix
@@ -76,7 +82,7 @@ func createExperiment(name, weights string) error {
 		return err
 	}
 
-	split, err := splits.New(&name, weightsMap)
+	split, err := splits.New(&name, weightsMap, &owner)
 	if err != nil {
 		return err
 	}

cmds/create_feature_gate.go

@@ -33,9 +33,10 @@ destroyed split if it was destroyed by mistake, but the migration will fail if
 you attempt to create a new split without a prefix.
 `
 
-var createFeatureGateDefault, createFeatureGateWeights string
+var createFeatureGateDefault, createFeatureGateWeights, createFeatureGateOwner string
 
 func init() {
+	createFeatureGateCmd.Flags().StringVar(&createFeatureGateOwner, "owner", "", "Who owns this feature flag?")
 	createFeatureGateCmd.Flags().StringVar(&createFeatureGateDefault, "default", "false", "Default variant for your feature flag")
 	createFeatureGateCmd.Flags().StringVar(&createFeatureGateWeights, "weights", "", "Variant weights to use (overrides default)")
 	createFeatureGateCmd.Flags().BoolVar(&noPrefix, "no-prefix", false, "Don't prefix feature gate with app_name (supports existing legacy splits)")
@@ -48,11 +49,11 @@ var createFeatureGateCmd = &cobra.Command{
 	Long:  createFeatureGateDoc,
 	Args:  cobra.ExactArgs(1),
 	RunE: func(cmd *cobra.Command, args []string) error {
-		return createFeatureGate(args[0], createFeatureGateDefault, createFeatureGateWeights)
+		return createFeatureGate(args[0], createFeatureGateDefault, createFeatureGateWeights, createFeatureGateOwner)
 	},
 }
 
-func createFeatureGate(name, defaultVariant, weights string) error {
+func createFeatureGate(name, defaultVariant, weights string, owner string) error {
 	schema, err := schema.Read()
 	if err != nil {
 		return err
@@ -76,6 +77,11 @@ func createFeatureGate(name, defaultVariant, weights string) error {
 		}
 	}
 
+	err = validations.ValidateOwnerName(owner)
+	if err != nil {
+		return err
+	}
+
 	if len(weights) == 0 {
 		switch defaultVariant {
 		case "true":
@@ -104,7 +110,7 @@ func createFeatureGate(name, defaultVariant, weights string) error {
 		return fmt.Errorf("weights %v are missing false variant", *weightsMap)
 	}
 
-	split, err := splits.New(&name, weightsMap)
+	split, err := splits.New(&name, weightsMap, &createFeatureGateOwner)
 	if err != nil {
 		return err
 	}

cmds/root.go

@@ -16,13 +16,10 @@ var build string
 var arch string = fmt.Sprintf("%s-%s", runtime.GOOS, runtime.GOARCH)
 var noPrefix bool
 var force bool
+var ownershipFilename string
 
 func init() {
-	_, urlSet := os.LookupEnv("TESTTRACK_CLI_URL")
-	_, appNameSet := os.LookupEnv("TESTTRACK_APP_NAME")
-	if !urlSet && !appNameSet {
-		godotenv.Load()
-	}
+	godotenv.Load()
 }
 
 var rootCmd = &cobra.Command{

serializers/serializers.go

@@ -42,6 +42,7 @@ type RemoteKill struct {
 type SplitYAML struct {
 	Name    string        `yaml:"name"`
 	Weights yaml.MapSlice `yaml:"weights"`
+	Owner   string        `yaml:"owner,omitempty"`
 }
 
 // SplitJSON is is the JSON-marshalabe representation of a Split
@@ -72,6 +73,7 @@ type SchemaSplit struct {
 	Name    string        `yaml:"name"`
 	Weights yaml.MapSlice `yaml:"weights"`
 	Decided bool          `yaml:"decided,omitempty"`
+	Owner   string        `yaml:"owner,omitempty"`
 }
 
 // Schema is the YAML-marshalable representation of the TestTrack schema for

splits/splits.go

@@ -25,10 +25,11 @@ type Split struct {
 	migrationVersion *string
 	name             *string
 	weights          *Weights
+	owner            *string
 }
 
 // New returns a migration object
-func New(name *string, weights *Weights) (migrations.IMigration, error) {
+func New(name *string, weights *Weights, owner *string) (migrations.IMigration, error) {
 	migrationVersion, err := migrations.GenerateMigrationVersion()
 	if err != nil {
 		return nil, err
@@ -38,6 +39,7 @@ func New(name *string, weights *Weights) (migrations.IMigration, error) {
 		migrationVersion: migrationVersion,
 		name:             name,
 		weights:          weights,
+		owner:            owner,
 	}, nil
 }
 
@@ -110,6 +112,7 @@ func (s *Split) File() *serializers.MigrationFile {
 		Split: &serializers.SplitYAML{
 			Name:    *s.name,
 			Weights: s.weights.ToYAML(),
+			Owner:   *s.owner,
 		},
 	}
 }
@@ -176,6 +179,7 @@ func (s *Split) ApplyToSchema(schema *serializers.Schema, migrationRepo migratio
 		Name:    *s.name,
 		Weights: s.weights.ToYAML(),
 		Decided: false,
+		Owner:   *s.owner,
 	}
 	schema.Splits = append(schema.Splits, schemaSplit)
 	return nil

validations/validations.go

@@ -2,15 +2,21 @@ package validations
 
 import (
 	"fmt"
+	"io/ioutil"
+	"os"
 	"regexp"
 	"strings"
 
 	"github.com/Betterment/testtrack-cli/serializers"
+	"gopkg.in/yaml.v2"
 )
 
 const appVersionMaxLength = 18 // This conforms to iOS version numering rules
 const splitMaxLength = 128     // This is arbitrary but way bigger than you need and smaller than the column will fit
 
+// DefaultOwnershipFilePath defines the default path to a YML file listing the possible split owners
+const DefaultOwnershipFilePath = "testtrack/owners.yml"
+
 var prefixedSplitRegex = regexp.MustCompile(`^([a-z_\-\d]+)\.[a-z_\d]+$`)
 var nonPrefixedSplitRegex = regexp.MustCompile(`^[a-z_\d]+$`)
 var ambiPrefixedSplitRegex = regexp.MustCompile(`^(?:[a-z_\-\d]+\.)?[a-z_\d]+$`)
@@ -65,6 +71,56 @@ func AutoPrefixAndValidateSplit(paramName string, value *string, currentAppName
 	return SplitExistsInSchema(paramName, value, schema)
 }
 
+// ValidateOwnerName ensures that if a testtrack/owners.yml file is present, the owner matches
+// the list of owners in that file.
+func ValidateOwnerName(owner string) error {
+	ownershipFilePath, ok := os.LookupEnv("TESTTRACK_OWNERSHIP_FILE")
+	if !ok {
+		ownershipFilePath = DefaultOwnershipFilePath
+	}
+
+	// If no ownership file exists, force owner to be empty. Otherwise pass validations.
+	_, err := os.Stat(ownershipFilePath)
+	if os.IsNotExist(err) {
+		if owner != "" {
+			return fmt.Errorf("owner must be blank because ownership file (%s) could not be found", ownershipFilePath)
+		}
+
+		return nil
+	}
+
+	// When the ownership file exists, owner must be specified and must be in the ownership file.
+	if owner == "" {
+		return fmt.Errorf("owner must be specified when ownership file (%s) exists", ownershipFilePath)
+	}
+
+	fileBytes, err := ioutil.ReadFile(ownershipFilePath)
+	if err != nil {
+		return err
+	}
+
+	ownersMap := make(map[string]*struct{})
+	err = yaml.Unmarshal(fileBytes, ownersMap)
+	if err != nil {
+		return err
+	}
+
+	if !mapContainsValue(owner, ownersMap) {
+		return fmt.Errorf("owner '%s' is not defined in ownership file (%s)", owner, ownershipFilePath)
+	}
+
+	return nil
+}
+
+func mapContainsValue(value string, m map[string]*struct{}) bool {
+	for key := range m {
+		if key == value {
+			return true
+		}
+	}
+	return false
+}
+
 // NonPrefixedSplit validates that a split name param is valid with no app prefix
 func NonPrefixedSplit(paramName string, value *string) error {
 	err := Presence(paramName, value)

validations/validations_test.go

@@ -1,6 +1,8 @@
 package validations_test
 
 import (
+	"os"
+	"path/filepath"
 	"testing"
 
 	"github.com/Betterment/testtrack-cli/serializers"
@@ -130,6 +132,94 @@ func TestAutoPrefixAndValidateSplit(t *testing.T) {
 	})
 }
 
+func TestValidateOwnerName(t *testing.T) {
+	t.Run("it succeeds with no owner if ownershipFilename is undefined and the default file does not exist", func(t *testing.T) {
+		err := validations.ValidateOwnerName("")
+		require.NoError(t, err)
+	})
+
+	t.Run("it fails with an owner if ownershipFilename is undefined and the default file does not exist ", func(t *testing.T) {
+		err := validations.ValidateOwnerName("super_owner")
+		require.Error(t, err)
+		require.Contains(t, err.Error(), "owner must be blank because ownership file (testtrack/owners.yml) could not be found")
+	})
+
+	t.Run("it fails if using default ownership file and owner is blank", func(t *testing.T) {
+		WriteOwnershipFile(validations.DefaultOwnershipFilePath)
+
+		err := validations.ValidateOwnerName("")
+		require.Error(t, err)
+		require.Contains(t, err.Error(), "owner must be specified when ownership file (testtrack/owners.yml) exists")
+
+		RemoveOwnershipFile(validations.DefaultOwnershipFilePath)
+	})
+
+	t.Run("it fails if using specified ownership file and owner is blank", func(t *testing.T) {
+		WriteOwnershipFile(".owners.yml")
+		t.Setenv("TESTTRACK_OWNERSHIP_FILE", ".owners.yml")
+
+		err := validations.ValidateOwnerName("")
+		require.Error(t, err)
+		require.Contains(t, err.Error(), "owner must be specified when ownership file (.owners.yml) exists")
+
+		RemoveOwnershipFile(".owners.yml")
+	})
+
+	t.Run("it succeeds if using default ownership file and owner exists", func(t *testing.T) {
+		WriteOwnershipFile(validations.DefaultOwnershipFilePath)
+
+		err := validations.ValidateOwnerName("super_owner")
+		require.NoError(t, err)
+
+		RemoveOwnershipFile(validations.DefaultOwnershipFilePath)
+	})
+
+	t.Run("it succeeds if using specified ownership file and owner exists", func(t *testing.T) {
+		WriteOwnershipFile(".owners.yml")
+		t.Setenv("TESTTRACK_OWNERSHIP_FILE", ".owners.yml")
+
+		err := validations.ValidateOwnerName("super_owner")
+		require.NoError(t, err)
+
+		RemoveOwnershipFile(".owners.yml")
+	})
+
+	t.Run("it fails if using default ownership file and owner does not exist", func(t *testing.T) {
+		WriteOwnershipFile(validations.DefaultOwnershipFilePath)
+
+		err := validations.ValidateOwnerName("superb_owner")
+		require.Error(t, err)
+		require.Contains(t, err.Error(), "owner 'superb_owner' is not defined in ownership file (testtrack/owners.yml)")
+
+		RemoveOwnershipFile(validations.DefaultOwnershipFilePath)
+	})
+
+	t.Run("it fails if using specified ownership file and owner does not exist", func(t *testing.T) {
+		WriteOwnershipFile(".owners.yml")
+		t.Setenv("TESTTRACK_OWNERSHIP_FILE", ".owners.yml")
+
+		err := validations.ValidateOwnerName("superb_owner")
+		require.Error(t, err)
+		require.Contains(t, err.Error(), "owner 'superb_owner' is not defined in ownership file (.owners.yml)")
+
+		RemoveOwnershipFile(".owners.yml")
+	})
+}
+
 func StrPtr(value string) *string {
 	return &value
 }
+
+func WriteOwnershipFile(ownershipFilename string) {
+	if _, err := os.Stat(ownershipFilename); os.IsNotExist(err) {
+		os.MkdirAll(filepath.Dir(ownershipFilename), 0700)
+	}
+
+	ownerContent := []byte("super_owner:\n  delayed_job_alert_slack_channel: '#super_owner'\n")
+	os.WriteFile(ownershipFilename, ownerContent, 0644)
+}
+
+func RemoveOwnershipFile(ownershipFilename string) {
+	os.Remove(ownershipFilename)
+	os.RemoveAll(filepath.Dir(ownershipFilename))
+}