diff --git a/Makefile b/Makefile index 48d6742..725cd85 100644 --- a/Makefile +++ b/Makefile @@ -1,6 +1,6 @@ SHELL = /bin/sh -VERSION=1.9.0 +VERSION=2.0.0 BUILD=`git rev-parse HEAD` LDFLAGS=-ldflags "-w -s \ diff --git a/fakeserver/server_test.go b/fakeserver/server_test.go index a0a554a..de5147a 100644 --- a/fakeserver/server_test.go +++ b/fakeserver/server_test.go @@ -20,8 +20,9 @@ import ( ) var testSchema = ` -serializer_version: 1 -schema_version: "2020011774023" +serializer_version: 2 +schema_versions: +- "2020011774023" splits: - name: test.test_experiment weights: @@ -34,8 +35,8 @@ splits: ` var otherTestSchema = `{ - "serializer_version": 1, - "schema_version": "2020011774023", + "serializer_version": 2, + "schema_versions": ["2020011774023"], "splits": [ { "name": "test.json_experiment", diff --git a/migrationmanagers/migrationmanagers.go b/migrationmanagers/migrationmanagers.go index 2d43455..b4b9131 100644 --- a/migrationmanagers/migrationmanagers.go +++ b/migrationmanagers/migrationmanagers.go @@ -94,8 +94,8 @@ func (m *MigrationManager) ApplyToSchema(migrationRepo migrations.Repository, id } appliedVersion := m.migration.MigrationVersion() - if appliedVersion != nil && m.schema.SchemaVersion < *appliedVersion { - m.schema.SchemaVersion = *appliedVersion + if appliedVersion != nil { + m.schema.AddVersion(*appliedVersion) } return nil } @@ -160,10 +160,5 @@ func (m *MigrationManager) SyncVersion() error { return fmt.Errorf("got %d status code", resp.StatusCode) } - appliedVersion := m.migration.MigrationVersion() - if m.schema.SchemaVersion < *appliedVersion { - m.schema.SchemaVersion = *appliedVersion - } - return nil } diff --git a/schema/schema.go b/schema/schema.go index 9916da8..f9c2c83 100644 --- a/schema/schema.go +++ b/schema/schema.go @@ -1,6 +1,8 @@ package schema import ( + "bytes" + "crypto/sha1" "encoding/json" "errors" "fmt" @@ -63,9 +65,10 @@ func Generate() (*serializers.Schema, error) { return schema, nil } -// Write a schema to disk after alpha-sorting its resources +// Write a schema to disk after sorting its resources into a stable order func Write(schema *serializers.Schema) error { SortAlphabetically(schema) + sortSchemaVersions(schema) schemaPath, _ := findSchemaPath() @@ -204,17 +207,35 @@ func applyAllMigrationsToSchema(schema *serializers.Schema) error { return err } } - if len(versions) != 0 { - schema.SchemaVersion = versions[len(versions)-1] - } + schema.SchemaVersions = versions return nil } +// sortSchemaVersions orders the applied-version list by the SHA-1 of each +// version. The order is otherwise meaningless; hashing scatters newly added +// versions through the list instead of clustering them by timestamp, so two +// branches that each append a migration rarely touch the same lines. Hashes +// are precomputed so each version is hashed once rather than on every +// comparison. +func sortSchemaVersions(schema *serializers.Schema) { + hashes := make(map[string][sha1.Size]byte, len(schema.SchemaVersions)) + for _, version := range schema.SchemaVersions { + hashes[version] = sha1.Sum([]byte(version)) + } + sort.Slice(schema.SchemaVersions, func(i, j int) bool { + a := hashes[schema.SchemaVersions[i]] + b := hashes[schema.SchemaVersions[j]] + return bytes.Compare(a[:], b[:]) < 0 + }) +} + // SortAlphabetically sorts the schema's resource slices by their natural keys func SortAlphabetically(schema *serializers.Schema) { sort.Slice(schema.RemoteKills, func(i, j int) bool { - return schema.RemoteKills[i].Split < schema.RemoteKills[j].Split && - schema.RemoteKills[i].Reason < schema.RemoteKills[j].Reason + if schema.RemoteKills[i].Split != schema.RemoteKills[j].Split { + return schema.RemoteKills[i].Split < schema.RemoteKills[j].Split + } + return schema.RemoteKills[i].Reason < schema.RemoteKills[j].Reason }) sort.Slice(schema.FeatureCompletions, func(i, j int) bool { return schema.FeatureCompletions[i].FeatureGate < schema.FeatureCompletions[j].FeatureGate diff --git a/schema/schema_test.go b/schema/schema_test.go new file mode 100644 index 0000000..dc12a8e --- /dev/null +++ b/schema/schema_test.go @@ -0,0 +1,80 @@ +package schema + +import ( + "os" + "path/filepath" + "strconv" + "testing" + + "github.com/Betterment/testtrack-cli/serializers" + "github.com/stretchr/testify/require" +) + +func TestSortSchemaVersionsByHash(t *testing.T) { + schema := &serializers.Schema{SchemaVersions: []string{ + "2020011712345", + "2020011712346", + "2020011712347", + "2020011712348", + "2020011712349", + }} + sortSchemaVersions(schema) + + expected := []string{ + "2020011712349", // 0dfad45993f8e2122515814d8e62f676fcb33841 + "2020011712347", // 13af4b988d91f57f71abeb0d657c24a18fba27d7 + "2020011712346", // 529bb729c31584a72d98669cf1daa0b9509379ce + "2020011712348", // a467c4bdcdbdac7030e77951932029e03a8d0920 + "2020011712345", // bef98a4d8faa7aea1b347fb834a25c01be3c44d2 + } + require.Equal(t, expected, schema.SchemaVersions, "versions should be ordered by sha1 of the version") +} + +func TestGenerateRecordsAllMigrationVersions(t *testing.T) { + dir := t.TempDir() + t.Chdir(dir) + require.NoError(t, os.MkdirAll("testtrack/migrate", 0755)) + + versions := []string{"2020011712345", "2020011712346", "2020011712347"} + for i, version := range versions { + writeSplitMigration(t, version, []string{"alpha_experiment", "bravo_experiment", "charlie_experiment"}[i]) + } + + schema, err := Generate() + require.NoError(t, err) + + require.ElementsMatch(t, versions, schema.SchemaVersions, + "every migration version should be recorded in schema_versions") +} + +func writeSplitMigration(t *testing.T, version, name string) { + t.Helper() + contents := "serializer_version: " + strconv.Itoa(serializers.SerializerVersion) + "\n" + + "split:\n" + + " name: " + name + "\n" + + " weights:\n" + + " control: 50\n" + + " treatment: 50\n" + path := filepath.Join("testtrack", "migrate", version+"_"+name+".yml") + require.NoError(t, os.WriteFile(path, []byte(contents), 0644)) +} + +func TestSortAlphabeticallyRemoteKillsIsDeterministic(t *testing.T) { + // These three kills are mutually incomparable under the old buggy + // comparator (Split < Split && Reason < Reason), which made the written + // order depend on input order. + kills := []serializers.RemoteKill{ + {Split: "a_split", Reason: "z_reason"}, + {Split: "b_split", Reason: "m_reason"}, + {Split: "c_split", Reason: "a_reason"}, + } + forward := &serializers.Schema{RemoteKills: []serializers.RemoteKill{kills[0], kills[1], kills[2]}} + reversed := &serializers.Schema{RemoteKills: []serializers.RemoteKill{kills[2], kills[1], kills[0]}} + + SortAlphabetically(forward) + SortAlphabetically(reversed) + + require.Equal(t, forward.RemoteKills, reversed.RemoteKills, + "remote kills must sort to the same order regardless of input order") + require.Equal(t, []serializers.RemoteKill{kills[0], kills[1], kills[2]}, forward.RemoteKills) +} diff --git a/schemaloaders/schemaloaders.go b/schemaloaders/schemaloaders.go index cbf5d22..16172ba 100644 --- a/schemaloaders/schemaloaders.go +++ b/schemaloaders/schemaloaders.go @@ -71,10 +71,16 @@ func (s *SchemaLoader) Load() error { } } + appliedVersions := make(map[string]bool, len(s.schema.SchemaVersions)) + for _, version := range s.schema.SchemaVersions { + appliedVersions[version] = true + } + + unrecorded := false for _, version := range s.migrationRepo.SortedVersions() { - if version > s.schema.SchemaVersion { - fmt.Println("Schema load complete, but there are migrations newer than the schema file - run testtrack migrate to apply them.") - break + if !appliedVersions[version] { + unrecorded = true + continue } err := migrationmanagers.NewWithServer((*s.migrationRepo)[version], s.server).SyncVersion() if err != nil { @@ -82,6 +88,10 @@ func (s *SchemaLoader) Load() error { } } + if unrecorded { + fmt.Println("Schema load complete, but there are migrations on disk not recorded in the schema file - run testtrack schema generate to update it.") + } + return nil } diff --git a/schemaloaders/schemaloaders_test.go b/schemaloaders/schemaloaders_test.go new file mode 100644 index 0000000..c6da141 --- /dev/null +++ b/schemaloaders/schemaloaders_test.go @@ -0,0 +1,99 @@ +package schemaloaders + +import ( + "io" + "net/http" + "os" + "strings" + "testing" + + "github.com/Betterment/testtrack-cli/migrations" + "github.com/Betterment/testtrack-cli/serializers" + "github.com/Betterment/testtrack-cli/splits" + "github.com/stretchr/testify/require" +) + +const unrecordedWarning = "migrations on disk not recorded in the schema file" + +// fakeServer records the paths it's asked to POST and always succeeds. +type fakeServer struct { + postedPaths []string +} + +func (f *fakeServer) Get(string, interface{}) error { return nil } + +func (f *fakeServer) Delete(string) error { return nil } + +func (f *fakeServer) Post(path string, _ interface{}) (*http.Response, error) { + f.postedPaths = append(f.postedPaths, path) + return &http.Response{StatusCode: 204, Body: io.NopCloser(strings.NewReader(""))}, nil +} + +func TestLoadSyncsOnlyRecordedVersionsAndWarnsOnce(t *testing.T) { + server := &fakeServer{} + // Two versions are recorded in the schema; two migration files on disk are not. + schema := &serializers.Schema{SchemaVersions: []string{"2020011712345", "2020011712346"}} + repo := migrations.Repository{ + "2020011712345": newSplitMigration(t, "2020011712345"), + "2020011712346": newSplitMigration(t, "2020011712346"), + "2020011712347": newSplitMigration(t, "2020011712347"), + "2020011712348": newSplitMigration(t, "2020011712348"), + } + loader := &SchemaLoader{server: server, schema: schema, migrationRepo: &repo} + + out := captureStdout(t, func() { + require.NoError(t, loader.Load()) + }) + + require.Equal(t, 1, strings.Count(out, unrecordedWarning), + "warning should print exactly once no matter how many unrecorded migrations exist") + // Only the two recorded versions get marked applied on the server; the + // schema here has no splits/etc., so these are the only posts. + require.Equal(t, []string{"api/v2/migrations", "api/v2/migrations"}, server.postedPaths) +} + +func TestLoadDoesNotWarnWhenEveryMigrationIsRecorded(t *testing.T) { + server := &fakeServer{} + schema := &serializers.Schema{SchemaVersions: []string{"2020011712345", "2020011712346"}} + repo := migrations.Repository{ + "2020011712345": newSplitMigration(t, "2020011712345"), + "2020011712346": newSplitMigration(t, "2020011712346"), + } + loader := &SchemaLoader{server: server, schema: schema, migrationRepo: &repo} + + out := captureStdout(t, func() { + require.NoError(t, loader.Load()) + }) + + require.NotContains(t, out, unrecordedWarning) + require.Equal(t, []string{"api/v2/migrations", "api/v2/migrations"}, server.postedPaths) +} + +func newSplitMigration(t *testing.T, version string) migrations.IMigration { + t.Helper() + migration, err := splits.FromFile(&version, &serializers.SplitYAML{ + Name: "split_" + version, + Weights: map[string]int{"control": 50, "treatment": 50}, + }) + require.NoError(t, err) + return migration +} + +func captureStdout(t *testing.T, fn func()) string { + t.Helper() + original := os.Stdout + r, w, err := os.Pipe() + require.NoError(t, err) + os.Stdout = w + defer func() { + os.Stdout = original + r.Close() + }() + + fn() + require.NoError(t, w.Close()) + + out, err := io.ReadAll(r) + require.NoError(t, err) + return string(out) +} diff --git a/serializers/serializers.go b/serializers/serializers.go index 34f06ae..17dd7cc 100644 --- a/serializers/serializers.go +++ b/serializers/serializers.go @@ -4,8 +4,15 @@ import ( "gopkg.in/yaml.v2" ) -// SerializerVersion is the current version of the migration file format so we can evolve over time -const SerializerVersion = 1 +// SerializerVersion is the current version of the file formats so we can +// evolve them over time. +// +// Version 2 replaced the single scalar schema_version high-water-mark with a +// schema_versions list of every applied migration version, eliminating the +// merge-conflict hotspot that the scalar created (every new migration rewrote +// the same line). The list is ordered by a hash of each version so concurrently +// added migrations scatter through the file instead of clustering. +const SerializerVersion = 2 // MigrationVersion is a JSON-marshalable representation of migration version (timestamp) type MigrationVersion struct { @@ -90,13 +97,24 @@ type SchemaSplit struct { // migration validation and bootstrapping of new ecosystems type Schema struct { SerializerVersion int `yaml:"serializer_version" json:"serializer_version"` - SchemaVersion string `yaml:"schema_version" json:"schema_version"` + SchemaVersions []string `yaml:"schema_versions,omitempty" json:"schema_versions,omitempty"` Splits []SchemaSplit `yaml:"splits,omitempty" json:"splits,omitempty"` IdentifierTypes []IdentifierType `yaml:"identifier_types,omitempty" json:"identifier_types,omitempty"` RemoteKills []RemoteKill `yaml:"remote_kills,omitempty" json:"remote_kills,omitempty"` FeatureCompletions []FeatureCompletion `yaml:"feature_completions,omitempty" json:"feature_completions,omitempty"` } +// AddVersion records a migration version as applied in the schema, ignoring +// duplicates. Ordering is handled at write time, so callers needn't sort. +func (s *Schema) AddVersion(version string) { + for _, v := range s.SchemaVersions { + if v == version { + return + } + } + s.SchemaVersions = append(s.SchemaVersions, version) +} + // LegacySchema represents the Rails migration-piggybacked testtrack schema files of old type LegacySchema struct { IdentifierTypes []string `yaml:"identifier_types"`