diff --git a/Makefile b/Makefile index 943f595..a8d3773 100644 --- a/Makefile +++ b/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 diff --git a/README.md b/README.md index 0004c57..1548240 100644 --- a/README.md +++ b/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. diff --git a/cmds/create_experiment.go b/cmds/create_experiment.go index fbd21d7..f6d0c2d 100644 --- a/cmds/create_experiment.go +++ b/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 } diff --git a/cmds/create_feature_gate.go b/cmds/create_feature_gate.go index d39db5d..be5a2bd 100644 --- a/cmds/create_feature_gate.go +++ b/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 } diff --git a/cmds/root.go b/cmds/root.go index a2b8906..1c9f0fd 100644 --- a/cmds/root.go +++ b/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{ diff --git a/serializers/serializers.go b/serializers/serializers.go index 42f99e5..62fa537 100644 --- a/serializers/serializers.go +++ b/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 diff --git a/splits/splits.go b/splits/splits.go index b13ac7f..703d8ba 100644 --- a/splits/splits.go +++ b/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 diff --git a/validations/validations.go b/validations/validations.go index a568e98..27f5272 100644 --- a/validations/validations.go +++ b/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) diff --git a/validations/validations_test.go b/validations/validations_test.go index 7021d0b..91bb1bb 100644 --- a/validations/validations_test.go +++ b/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)) +}