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/UPGRADING.md b/UPGRADING.md new file mode 100644 index 0000000..c269488 --- /dev/null +++ b/UPGRADING.md @@ -0,0 +1,100 @@ +# Upgrading testtrack-cli + +## 1.x to 2.0.0 + +2.0.0 changes the on-disk schema format (`testtrack/schema.{yml,json}`). + +### What changed + +The schema's applied-migration high-water mark used to be a single scalar +field: + +```yaml +serializer_version: 1 +schema_version: "2020011543200" +``` + +Every new migration rewrote that one line, so any two branches that added +migrations conflicted on it. 2.0.0 replaces the scalar with a list of every +applied migration version (`serializer_version` bumps to `2`): + +```yaml +serializer_version: 2 +schema_versions: +- "2020060151730" +- "2020011543200" +``` + +The list is ordered by a hash of each version (not chronologically), so +concurrently added migrations scatter through the file instead of clustering +on one line. The ordering is otherwise meaningless — don't rely on it. + +### This is a breaking format change + +A 2.0.0 CLI **refuses to read** an old (`serializer_version: 1`) schema file. +Any command that reads the schema errors and tells you to run `testtrack schema +upgrade` — `create`, `migrate`, `sync`, `decide`, the `destroy` commands, and +`schema load` (essentially everything except `assign`/`unassign`, which use a +different read path). This is deliberate: the old format can't be losslessly +read into the new struct, so the CLI makes you convert it explicitly rather than +silently round-tripping a lossy upgrade. + +Conversely, a pre-2.0.0 CLI does **not** understand `schema_versions`. If it +reads a v2 schema it ignores the field, and any command that rewrites the +schema produces a hybrid: the file keeps `serializer_version: 2` (the old CLI +round-trips whatever number it read) but is otherwise back in the v1 shape — a +scalar `schema_version`, no `schema_versions` list. A 2.0.0 CLI detects the +leftover `schema_version` field, refuses to read the hybrid, and tells you to +run `testtrack schema upgrade`, which repairs it by rebuilding the version list +from `testtrack/migrate`. So the whole team — and every CI/build environment — +has to be on 2.0.0 before you commit a v2 schema, or the format will flip-flop +between commits. + +Because a 2.0.0 CLI rejects a v1 schema and a 1.x CLI mangles a v2 schema, the +switch is a flag day: the moment the v2 schema lands in the repo, every consumer +of it must already be on 2.0.0. Order the rollout so the CLI bump and the schema +commit line up, not in separate steps. + +1. **Make 2.0.0 available, but don't switch anything to it yet.** Cut the + `v2.0.0` release, update the brew formula, and make the pinned binary + available to CI — without yet changing the CI pin or asking devs to upgrade. + +2. **Land the schema conversion and the CI bump together, in one change.** In + your app root: + + ```sh + testtrack schema upgrade + ``` + + This converts `testtrack/schema.{yml,json}` to the v2 format **in place**: it + keeps the materialized state already in the file (splits, decisions, + retirements, etc.) and only rebuilds the `schema_versions` list from the + migration filenames in `testtrack/migrate`. In the **same** PR, bump the + CI/build pin to `v2.0.0`. They must be atomic: if CI runs 2.0.0 against a + still-v1 schema, `testtrack migrate` errors; if the v2 schema lands while CI + still runs 1.x, the old binary rewrites it into the hybrid described above, + and every 2.0.0 user is blocked until someone re-runs `schema upgrade`. + Expect a large + one-time diff — the new `schema_versions` block lists every migration — but + the splits body is left untouched. + +3. **Developers upgrade as that change lands** (`brew upgrade testtrack-cli`). A + developer must be on 2.0.0 before running any testtrack command against the + upgraded schema — an older binary silently rewrites it into the hybrid, + which 2.0.0 CLIs then refuse to read until it's repaired with + `schema upgrade`. + +### Notes + +- **Use `schema upgrade`, not `schema generate`, to convert.** `generate` + rebuilds the schema by replaying every migration from scratch, which fails on + many long-lived apps — e.g. when `testtrack/migrate` predates some splits, or + contains a decision/retirement for a split that was created out of band in the + TestTrack admin (so there's no create migration to replay). `upgrade` doesn't + replay; it preserves the already-correct materialized state and just restamps + the format, so it works regardless. +- `schema upgrade` reads the old file directly (it's the one command that + bypasses the version guard), so it's always the recovery path if you hit the + "older schema format" error. +- No server-side change is required. The TestTrack server tracks applied + migrations itself; `schema_version`/`schema_versions` is a CLI-only artifact. diff --git a/cmds/schema_upgrade.go b/cmds/schema_upgrade.go new file mode 100644 index 0000000..de83502 --- /dev/null +++ b/cmds/schema_upgrade.go @@ -0,0 +1,40 @@ +package cmds + +import ( + "github.com/Betterment/testtrack-cli/schema" + "github.com/spf13/cobra" +) + +var schemaUpgradeDoc = ` +Upgrades testtrack/schema.{json,yml} to the schema format this CLI writes, +in place, without replaying migrations. + +It keeps the materialized state already in the file (splits, decisions, +retirements, feature completions, identifier types) and only rebuilds the +applied-migration version list from the files in testtrack/migrate. This is the +upgrade path to use when a newer CLI refuses to read an older-format schema. + +Unlike 'schema generate', upgrade does not rebuild the schema from scratch, so +it works on apps whose migrations can't be replayed cleanly - e.g. when +testtrack/migrate predates some splits, or references splits that were created +out of band in the TestTrack admin. +` + +func init() { + schemaCmd.AddCommand(schemaUpgradeCmd) +} + +var schemaUpgradeCmd = &cobra.Command{ + Use: "upgrade", + Short: "Upgrade schema.{json,yml} to the current format in place", + Long: schemaUpgradeDoc, + Args: cobra.NoArgs, + RunE: func(cmd *cobra.Command, _ []string) error { + return schemaUpgrade() + }, +} + +func schemaUpgrade() error { + _, err := schema.Upgrade() + return err +} 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/featurecompletions/featurecompletions.go b/featurecompletions/featurecompletions.go index 6131059..ae5d01f 100644 --- a/featurecompletions/featurecompletions.go +++ b/featurecompletions/featurecompletions.go @@ -67,7 +67,7 @@ func (f *FeatureCompletion) Filename() *string { // File returns a serializable MigrationFile for this migration func (f *FeatureCompletion) File() *serializers.MigrationFile { return &serializers.MigrationFile{ - SerializerVersion: serializers.SerializerVersion, + SerializerVersion: serializers.MigrationSerializerVersion, FeatureCompletion: f.serializable(), } } diff --git a/identifiertypes/identifiertypes.go b/identifiertypes/identifiertypes.go index 106398a..bee0190 100644 --- a/identifiertypes/identifiertypes.go +++ b/identifiertypes/identifiertypes.go @@ -54,7 +54,7 @@ func (i *IdentifierType) Filename() *string { // File returns a serializable MigrationFile for this migration func (i *IdentifierType) File() *serializers.MigrationFile { return &serializers.MigrationFile{ - SerializerVersion: serializers.SerializerVersion, + SerializerVersion: serializers.MigrationSerializerVersion, IdentifierType: i.serializable(), } } diff --git a/migrationloaders/migrationloaders.go b/migrationloaders/migrationloaders.go index 8310ec5..033b474 100644 --- a/migrationloaders/migrationloaders.go +++ b/migrationloaders/migrationloaders.go @@ -17,25 +17,65 @@ import ( "gopkg.in/yaml.v2" ) -// Load loads a set of migrations -func Load() (migrations.Repository, error) { +// Filenames lists the migration filenames in testtrack/migrate, skipping +// hidden files and directories. It is the single source of truth for which +// directory entries count as migrations — Load and `schema upgrade` must +// agree on that, or the schema's recorded versions drift from what the +// loader sees. +func Filenames() ([]string, error) { files, err := os.ReadDir("testtrack/migrate") if err != nil { return nil, err } - - migrationRepo := make(migrations.Repository) + filenames := make([]string, 0, len(files)) for _, file := range files { - if strings.HasPrefix(file.Name(), ".") { - continue // Skip hidden files + if file.IsDir() || strings.HasPrefix(file.Name(), ".") { + continue + } + filenames = append(filenames, file.Name()) + } + return filenames, nil +} + +// Versions returns the unique migration versions recorded in the filenames in +// testtrack/migrate, without reading file contents — usable even on repos +// whose migrations can't be parsed or replayed. +func Versions() ([]string, error) { + filenames, err := Filenames() + if err != nil { + return nil, err + } + seen := make(map[string]bool, len(filenames)) + versions := make([]string, 0, len(filenames)) + for _, filename := range filenames { + version, err := migrations.ExtractVersionFromFilename(filename) + if err != nil { + return nil, fmt.Errorf("%w - delete or rename it if it isn't a testtrack migration", err) + } + if seen[version] { + continue } + seen[version] = true + versions = append(versions, version) + } + return versions, nil +} - migrationVersion, err := migrations.ExtractVersionFromFilename(file.Name()) +// Load loads a set of migrations +func Load() (migrations.Repository, error) { + filenames, err := Filenames() + if err != nil { + return nil, err + } + + migrationRepo := make(migrations.Repository) + for _, filename := range filenames { + migrationVersion, err := migrations.ExtractVersionFromFilename(filename) if err != nil { return nil, err } - fileBytes, err := os.ReadFile(path.Join("testtrack/migrate", file.Name())) + fileBytes, err := os.ReadFile(path.Join("testtrack/migrate", filename)) if err != nil { return nil, err } @@ -62,7 +102,7 @@ func Load() (migrations.Repository, error) { } else if migrationFile.IdentifierType != nil { migrationRepo[migrationVersion] = identifiertypes.FromFile(&migrationVersion, migrationFile.IdentifierType) } else { - return nil, fmt.Errorf("testtrack/migrate/%s didn't match a known migration type", file.Name()) + return nil, fmt.Errorf("testtrack/migrate/%s didn't match a known migration type", filename) } } return migrationRepo, nil 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/remotekills/remotekills.go b/remotekills/remotekills.go index d921b0e..e7ddf29 100644 --- a/remotekills/remotekills.go +++ b/remotekills/remotekills.go @@ -91,7 +91,7 @@ func (r *RemoteKill) Filename() *string { // File returns a serializable MigrationFile for this migration func (r *RemoteKill) File() *serializers.MigrationFile { return &serializers.MigrationFile{ - SerializerVersion: serializers.SerializerVersion, + SerializerVersion: serializers.MigrationSerializerVersion, RemoteKill: r.serializable(), } } diff --git a/schema/schema.go b/schema/schema.go index 9916da8..6219ed9 100644 --- a/schema/schema.go +++ b/schema/schema.go @@ -1,6 +1,8 @@ package schema import ( + "bytes" + "crypto/sha1" "encoding/json" "errors" "fmt" @@ -27,22 +29,117 @@ func findSchemaPath() (string, bool) { return "testtrack/schema.json", false } +// readSchemaFile locates and unmarshals the schema file, rejecting files +// written by a newer CLI. exists is false (with a nil schema and nil error) +// when no schema file is present; callers decide how to handle that. +func readSchemaFile() (schema *serializers.Schema, schemaPath string, exists bool, err error) { + schemaPath, exists = findSchemaPath() + if !exists { + return nil, schemaPath, false, nil + } + schemaBytes, err := os.ReadFile(schemaPath) + if err != nil { + return nil, schemaPath, true, err + } + var s serializers.Schema + err = yaml.Unmarshal(schemaBytes, &s) + if err != nil { + return nil, schemaPath, true, err + } + if s.SerializerVersion > serializers.SerializerVersion { + return nil, schemaPath, true, fmt.Errorf( + "%s was written by a newer testtrack CLI (serializer_version %d, this CLI supports %d). Please upgrade your testtrack CLI", + schemaPath, s.SerializerVersion, serializers.SerializerVersion, + ) + } + return &s, schemaPath, true, nil +} + // Read a schema from disk or generate one func Read() (*serializers.Schema, error) { - schemaPath, exists := findSchemaPath() + schema, schemaPath, exists, err := readSchemaFile() + if err != nil { + return nil, err + } if !exists { return Generate() } - schemaBytes, err := os.ReadFile(schemaPath) + if schema.SerializerVersion < serializers.SerializerVersion { + // An older file predates a format change and may be missing data that + // can't be reconstructed by re-reading it (e.g. the v1 scalar + // schema_version carried no per-migration list). Refuse to read it + // rather than silently round-trip a lossy upgrade; `schema upgrade` + // converts it in place, preserving the materialized state. + return nil, fmt.Errorf( + "%s uses an older schema format (serializer_version %d, this CLI writes %d). Run `testtrack schema upgrade` to upgrade it", + schemaPath, schema.SerializerVersion, serializers.SerializerVersion, + ) + } + if schema.LegacySchemaVersion != nil { + // A pre-2.0 CLI rewriting a v2 schema produces a hybrid: it round-trips + // serializer_version: 2 but writes the v1 shape (scalar schema_version, + // no schema_versions list), so the version check above can't catch it. + // Reading it as-is would silently continue with an empty applied-version + // list. Key presence, not value, is the tell: 1.x write paths that never + // set the scalar (e.g. `sync`) emit schema_version: "". + return nil, fmt.Errorf( + "%s has serializer_version %d but contains the legacy schema_version field - it was likely rewritten by a pre-2.0 testtrack CLI. Run `testtrack schema upgrade` to repair it, and make sure no older CLI touches it again", + schemaPath, schema.SerializerVersion, + ) + } + if len(schema.SchemaVersions) == 0 { + // No machine write produces an empty version list alongside migrations + // on disk — that state means the schema_versions block was lost to a + // hand-edit or merge resolution (or the legacy scalar was nulled out, + // which also evades the presence check above). Reading it as-is would + // ratify the truncated list on the next write. A missing or unreadable + // migrate dir counts as no migrations. + if filenames, err := migrationloaders.Filenames(); err == nil && len(filenames) > 0 { + return nil, fmt.Errorf( + "%s has no schema_versions but testtrack/migrate contains migrations - the applied-version list was likely lost in a merge. Run `testtrack schema upgrade` to rebuild it", + schemaPath, + ) + } + } + return schema, nil +} + +// Upgrade converts an existing schema file to the current serializer format in +// place. Unlike Generate it does not replay migrations, so it preserves the +// already-materialized state (splits, decisions, retirements, etc.) and works +// even on schemas that can't be rebuilt from scratch — e.g. apps whose +// testtrack/migrate predates some splits or references ones created out of +// band. The only thing it rebuilds is the applied-version list, which it reads +// from the migration filenames on disk. +func Upgrade() (*serializers.Schema, error) { + schema, _, exists, err := readSchemaFile() if err != nil { return nil, err } - var schema serializers.Schema - err = yaml.Unmarshal(schemaBytes, &schema) + if !exists { + return nil, errors.New("no testtrack schema file to upgrade. Run testtrack schema generate to create one") + } + + versions, err := migrationloaders.Versions() + if err != nil { + if os.IsNotExist(err) { + // Legacy repos can have a schema with no testtrack/migrate dir at + // all (git doesn't track empty dirs); there are no versions to + // record, which is a valid v2 state. + versions = nil + } else { + return nil, err + } + } + schema.SchemaVersions = versions + schema.SerializerVersion = serializers.SerializerVersion + schema.LegacySchemaVersion = nil + + err = Write(schema) if err != nil { return nil, err } - return &schema, nil + return schema, nil } // Generate a schema from migrations on the filesystem and write it to disk @@ -63,9 +160,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() @@ -115,7 +213,16 @@ func Link(force bool) error { return os.Symlink(dir+"/"+schemaPath, path) } -// ReadMerged merges schemas linked at ~/testtrack/schemas into a single virtual schema +// ReadMerged merges schemas linked at ~/testtrack/schemas into a single virtual schema. +// +// It deliberately bypasses Read's serializer-version and hybrid guards: the +// linked schemas belong to *other* apps that upgrade on their own schedule, +// and hard-failing on a neighbor's stale schema would break assign/fakeserver +// for every app on the machine. This tolerance is safe because the body +// fields consumed here (splits, identifier_types, remote_kills, +// feature_completions) have never changed shape across serializer versions — +// a future version that reshapes them must add per-file version handling +// here. func ReadMerged() (*serializers.Schema, error) { configDir, err := paths.FakeServerConfigDir() if err != nil { @@ -204,17 +311,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..08fe648 --- /dev/null +++ b/schema/schema_test.go @@ -0,0 +1,313 @@ +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 TestReadRejectsNewerSerializerVersion(t *testing.T) { + withSchemaFile(t, ` +serializer_version: 99 +schema_versions: +- "2020011712345" +`) + + _, err := Read() + require.Error(t, err) + require.Contains(t, err.Error(), "upgrade your testtrack CLI") +} + +func TestReadRejectsOlderSerializerVersion(t *testing.T) { + withSchemaFile(t, ` +serializer_version: 1 +schema_version: "2020011712345" +`) + + _, err := Read() + require.Error(t, err) + require.Contains(t, err.Error(), "testtrack schema upgrade") +} + +func TestReadRejectsHybridSchemaFromPre2CLIRewrite(t *testing.T) { + // A pre-2.0 CLI rewriting a v2 schema round-trips serializer_version: 2 but + // writes the v1 shape: scalar schema_version, no schema_versions list. + withSchemaFile(t, ` +serializer_version: 2 +schema_version: "2020011712345" +splits: +- name: some_split + weights: + "false": 100 + "true": 0 +`) + + _, err := Read() + require.Error(t, err) + require.Contains(t, err.Error(), "pre-2.0 testtrack CLI") + require.Contains(t, err.Error(), "testtrack schema upgrade") +} + +func TestReadRejectsHybridSchemaWithEmptyScalar(t *testing.T) { + // 1.x write paths that never set the scalar (e.g. `sync`) emit + // schema_version: "" — the v1 field has no omitempty. Detection must be by + // key presence, not non-empty value, or this hybrid silently reads as a v2 + // schema with an empty applied-version list. + withSchemaFile(t, ` +serializer_version: 2 +schema_version: "" +splits: +- name: some_split + weights: + "false": 100 + "true": 0 +`) + + _, err := Read() + require.Error(t, err) + require.Contains(t, err.Error(), "pre-2.0 testtrack CLI") + require.Contains(t, err.Error(), "testtrack schema upgrade") +} + +func TestReadRejectsEmptyVersionListWithMigrationsOnDisk(t *testing.T) { + // A merge resolution that deletes the schema_versions block (or nulls the + // legacy scalar, which evades the presence guard) leaves a v2 file with no + // applied-version list. Reading it as-is would ratify the truncated list on + // the next write. + for name, contents := range map[string]string{ + "missing list": ` +serializer_version: 2 +splits: +- name: some_split + weights: + "false": 100 + "true": 0 +`, + "nulled legacy scalar": ` +serializer_version: 2 +schema_version: +splits: +- name: some_split + weights: + "false": 100 + "true": 0 +`, + } { + t.Run(name, func(t *testing.T) { + withSchemaFile(t, contents) + require.NoError(t, os.MkdirAll("testtrack/migrate", 0755)) + writeSplitMigration(t, "2020011712345", "some_split") + + _, err := Read() + require.Error(t, err) + require.Contains(t, err.Error(), "no schema_versions but testtrack/migrate contains migrations") + require.Contains(t, err.Error(), "testtrack schema upgrade") + }) + } +} + +func TestReadAcceptsEmptyVersionListWithoutMigrations(t *testing.T) { + // A schema with no recorded versions is valid when there are no migrations + // on disk (fresh projects, legacy repos with no migrate dir). + withSchemaFile(t, ` +serializer_version: 2 +splits: +- name: some_split + weights: + "false": 100 + "true": 0 +`) + + _, err := Read() + require.NoError(t, err) +} + +func TestUpgradeRepairsHybridSchema(t *testing.T) { + dir := t.TempDir() + t.Chdir(dir) + require.NoError(t, os.MkdirAll("testtrack/migrate", 0755)) + require.NoError(t, os.WriteFile(filepath.Join("testtrack", "schema.yml"), []byte(` +serializer_version: 2 +schema_version: "2020011712345" +splits: +- name: some_split + weights: + "false": 100 + "true": 0 +`), 0644)) + writeSplitMigration(t, "2020011712345", "some_split") + + schema, err := Upgrade() + require.NoError(t, err) + require.Equal(t, []string{"2020011712345"}, schema.SchemaVersions) + + // The legacy scalar must not survive the rewrite, so the repaired file + // passes Read's hybrid guard. + contents, err := os.ReadFile(filepath.Join("testtrack", "schema.yml")) + require.NoError(t, err) + require.NotContains(t, string(contents), "schema_version:") + + reread, err := Read() + require.NoError(t, err) + require.Equal(t, []string{"2020011712345"}, reread.SchemaVersions) +} + +func TestUpgradeConvertsInPlacePreservingBodyWithoutReplaying(t *testing.T) { + dir := t.TempDir() + t.Chdir(dir) + require.NoError(t, os.MkdirAll("testtrack/migrate", 0755)) + + // A v1 schema whose body references a split that has NO create migration on + // disk — i.e. it could not be rebuilt by replaying (a common real-world case). + require.NoError(t, os.WriteFile(filepath.Join("testtrack", "schema.yml"), []byte(` +serializer_version: 1 +schema_version: "2020011712346" +splits: +- name: legacy_split_created_out_of_band + weights: + "false": 100 + "true": 0 +`), 0644)) + + // Migration files on disk decide that orphan split — replaying these from + // scratch would fail, but upgrade doesn't replay. + writeSplitMigration(t, "2020011712345", "some_other_split") + require.NoError(t, os.WriteFile(filepath.Join("testtrack", "migrate", "2020011712346_create_split_decision_legacy_split_created_out_of_band.yml"), + []byte("serializer_version: 1\nsplit_decision:\n split: legacy_split_created_out_of_band\n variant: \"false\"\n"), 0644)) + + schema, err := Upgrade() + require.NoError(t, err) + + require.Equal(t, serializers.SerializerVersion, schema.SerializerVersion) + // Version list rebuilt from the migration filenames. + require.ElementsMatch(t, []string{"2020011712345", "2020011712346"}, schema.SchemaVersions) + // Materialized body preserved as-is (not replayed/rebuilt). + require.Len(t, schema.Splits, 1) + require.Equal(t, "legacy_split_created_out_of_band", schema.Splits[0].Name) + + // And the file is now readable by the normal guarded Read(). + reread, err := Read() + require.NoError(t, err) + require.Equal(t, serializers.SerializerVersion, reread.SerializerVersion) +} + +func TestUpgradeUsesFilenamesOnlyAndDedupes(t *testing.T) { + dir := t.TempDir() + t.Chdir(dir) + require.NoError(t, os.MkdirAll("testtrack/migrate", 0755)) + require.NoError(t, os.WriteFile(filepath.Join("testtrack", "schema.yml"), []byte(` +serializer_version: 1 +schema_version: "2020011712347" +`), 0644)) + + // Unparsable contents must not break upgrade — it only needs filenames. + require.NoError(t, os.WriteFile(filepath.Join("testtrack", "migrate", "2020011712345_create_split_broken.yml"), + []byte("{{{ not yaml"), 0644)) + // Two files sharing a version (it happens in real repos) record it once. + require.NoError(t, os.WriteFile(filepath.Join("testtrack", "migrate", "2020011712346_create_split_twin_a.yml"), + []byte("also garbage"), 0644)) + require.NoError(t, os.WriteFile(filepath.Join("testtrack", "migrate", "2020011712346_create_split_twin_b.yml"), + []byte("also garbage"), 0644)) + // Hidden files are skipped, matching the migration loader. + require.NoError(t, os.WriteFile(filepath.Join("testtrack", "migrate", ".DS_Store"), + []byte{0}, 0644)) + + schema, err := Upgrade() + require.NoError(t, err) + require.ElementsMatch(t, []string{"2020011712345", "2020011712346"}, schema.SchemaVersions) +} + +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) +} + +func TestReadAcceptsCurrentSerializerVersion(t *testing.T) { + withSchemaFile(t, ` +serializer_version: 2 +schema_versions: +- "2020011712345" +- "2020011712346" +`) + + schema, err := Read() + require.NoError(t, err) + require.Equal(t, []string{"2020011712345", "2020011712346"}, schema.SchemaVersions) +} + +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") +} + +// withSchemaFile writes a schema file into a temp testtrack dir and chdirs there. +func withSchemaFile(t *testing.T, contents string) { + t.Helper() + dir := t.TempDir() + t.Chdir(dir) + require.NoError(t, os.MkdirAll("testtrack", 0755)) + require.NoError(t, os.WriteFile(filepath.Join("testtrack", "schema.yml"), []byte(contents), 0644)) +} + +func writeSplitMigration(t *testing.T, version, name string) { + t.Helper() + contents := "serializer_version: " + strconv.Itoa(serializers.MigrationSerializerVersion) + "\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)) +} diff --git a/schemaloaders/schemaloaders.go b/schemaloaders/schemaloaders.go index cbf5d22..26ad5e3 100644 --- a/schemaloaders/schemaloaders.go +++ b/schemaloaders/schemaloaders.go @@ -71,10 +71,36 @@ func (s *SchemaLoader) Load() error { } } + appliedVersions := make(map[string]bool, len(s.schema.SchemaVersions)) + for _, version := range s.schema.SchemaVersions { + appliedVersions[version] = true + } + + var unrecorded []string + for _, version := range s.migrationRepo.SortedVersions() { + if !appliedVersions[version] { + unrecorded = append(unrecorded, version) + } + } + if len(unrecorded) > 0 { + // An unrecorded version can mean two different states, and the safe + // remedy differs: if only the version list was damaged (the schema body + // already reflects the migration), `schema upgrade` records it; but if + // the body change never landed (hand-copied file, merge dropped the + // schema hunk), upgrade would mark it applied WITHOUT applying it and + // permanently silence this warning. Enumerate the versions so the user + // can check, and print before syncing so the hint isn't lost if a + // SyncVersion call fails below. + fmt.Println("Warning: there are migrations on disk not recorded in the schema file:") + for _, version := range unrecorded { + fmt.Printf(" %s\n", version) + } + fmt.Println("If the schema file already reflects their changes (e.g. a merge lost only the version list), run `testtrack schema upgrade` to record them. If it does not, `schema upgrade` would mark them applied WITHOUT applying them - instead run `testtrack schema generate` to rebuild the schema from migrations (if your history replays cleanly), or re-create the change with the CLI.") + } + 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] { + continue } err := migrationmanagers.NewWithServer((*s.migrationRepo)[version], s.server).SyncVersion() if err != nil { diff --git a/schemaloaders/schemaloaders_test.go b/schemaloaders/schemaloaders_test.go new file mode 100644 index 0000000..7737351 --- /dev/null +++ b/schemaloaders/schemaloaders_test.go @@ -0,0 +1,132 @@ +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 succeeds unless +// failAfter is set, in which case Posts beyond that count return a 500. +type fakeServer struct { + postedPaths []string + failAfter int +} + +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) + if f.failAfter > 0 && len(f.postedPaths) > f.failAfter { + return &http.Response{StatusCode: 500, Body: io.NopCloser(strings.NewReader(""))}, nil + } + 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") + // The warning enumerates exactly the unrecorded versions so the user can + // inspect them, and explains both repair paths. + require.Contains(t, out, "2020011712347") + require.Contains(t, out, "2020011712348") + require.NotContains(t, out, " 2020011712345\n") + require.Contains(t, out, "testtrack schema upgrade") + require.Contains(t, out, "WITHOUT applying them") + // 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 TestLoadWarningSurvivesSyncVersionFailure(t *testing.T) { + // The first SyncVersion succeeds, the second gets a 500 — the warning must + // already be on stdout, not swallowed by the error return. + server := &fakeServer{failAfter: 1} + schema := &serializers.Schema{SchemaVersions: []string{"2020011712345", "2020011712346"}} + repo := migrations.Repository{ + "2020011712345": newSplitMigration(t, "2020011712345"), + "2020011712346": newSplitMigration(t, "2020011712346"), + "2020011712347": newSplitMigration(t, "2020011712347"), + } + loader := &SchemaLoader{server: server, schema: schema, migrationRepo: &repo} + + out := captureStdout(t, func() { + require.Error(t, loader.Load()) + }) + + require.Equal(t, 1, strings.Count(out, unrecordedWarning), + "warning must print even when a later SyncVersion fails") + require.Contains(t, out, "2020011712347") +} + +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..b0591be 100644 --- a/serializers/serializers.go +++ b/serializers/serializers.go @@ -4,8 +4,21 @@ 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 schema file format so we +// can evolve it 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 + +// MigrationSerializerVersion is the current version of the migration file +// format. It is versioned independently of the schema file: the schema format +// changed in v2 but migration files didn't, and stamping them with the schema's +// version would burn version numbers for a format that hasn't evolved. +const MigrationSerializerVersion = 1 // MigrationVersion is a JSON-marshalable representation of migration version (timestamp) type MigrationVersion struct { @@ -89,12 +102,32 @@ type SchemaSplit struct { // Schema is the YAML-marshalable representation of the TestTrack schema for // 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"` - 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"` + SerializerVersion int `yaml:"serializer_version" json:"serializer_version"` + SchemaVersions []string `yaml:"schema_versions,omitempty" json:"schema_versions,omitempty"` + // LegacySchemaVersion is the v1 scalar high-water mark. It only exists so + // reads can detect a file that carries it — either a plain v1 schema, or a + // hybrid produced by a pre-2.0 CLI rewriting a v2 schema (those round-trip + // serializer_version: 2 while writing the v1 shape). It's a pointer because + // detection is by key presence, not value: 1.x always writes the key (no + // omitempty), and rewrite paths that never set the scalar (e.g. 1.x `sync`) + // emit schema_version: "", which must still trip the guard. It is never + // written: `schema upgrade` clears it and omitempty drops nil from output. + LegacySchemaVersion *string `yaml:"schema_version,omitempty" json:"schema_version,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 diff --git a/splitdecisions/splitdecisions.go b/splitdecisions/splitdecisions.go index 3d9fb8d..4d5df8e 100644 --- a/splitdecisions/splitdecisions.go +++ b/splitdecisions/splitdecisions.go @@ -53,7 +53,7 @@ func (s *SplitDecision) Filename() *string { // File returns a serializable MigrationFile for this migration func (s *SplitDecision) File() *serializers.MigrationFile { return &serializers.MigrationFile{ - SerializerVersion: serializers.SerializerVersion, + SerializerVersion: serializers.MigrationSerializerVersion, SplitDecision: &serializers.SplitDecision{ Split: *s.split, Variant: *s.variant, diff --git a/splitretirements/splitretirements.go b/splitretirements/splitretirements.go index c9c5b2e..eeb5b55 100644 --- a/splitretirements/splitretirements.go +++ b/splitretirements/splitretirements.go @@ -53,7 +53,7 @@ func (s *SplitRetirement) Filename() *string { // File returns a serializable MigrationFile for this migration func (s *SplitRetirement) File() *serializers.MigrationFile { return &serializers.MigrationFile{ - SerializerVersion: serializers.SerializerVersion, + SerializerVersion: serializers.MigrationSerializerVersion, SplitRetirement: &serializers.SplitRetirement{ Split: *s.split, Decision: *s.decision, diff --git a/splits/splits.go b/splits/splits.go index ddd6316..49e576f 100644 --- a/splits/splits.go +++ b/splits/splits.go @@ -109,7 +109,7 @@ func (s *Split) Filename() *string { // File returns a serializable MigrationFile for this migration func (s *Split) File() *serializers.MigrationFile { return &serializers.MigrationFile{ - SerializerVersion: serializers.SerializerVersion, + SerializerVersion: serializers.MigrationSerializerVersion, Split: &serializers.SplitYAML{ Name: *s.name, Weights: *s.weights,