diff --git a/Makefile b/Makefile index 8ce3795..3daa91b 100644 --- a/Makefile +++ b/Makefile @@ -1,6 +1,6 @@ SHELL = /bin/sh -VERSION=1.8.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..4be7f87 --- /dev/null +++ b/UPGRADING.md @@ -0,0 +1,91 @@ +# 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: "20200115000000" +``` + +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: +- "20200601000000" +- "20200115000000" +``` + +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 the next time it regenerates the +schema it rewrites the file back to the v1 format. 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 reverts 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 back to v1. 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 reverts it to v1. + +### 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/migrationmanagers/migrationmanagers.go b/migrationmanagers/migrationmanagers.go index 2d43455..d7b83ba 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,7 @@ 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 - } + m.schema.AddVersion(*m.migration.MigrationVersion()) return nil } diff --git a/schema/schema.go b/schema/schema.go index 9916da8..bbca00c 100644 --- a/schema/schema.go +++ b/schema/schema.go @@ -1,6 +1,8 @@ package schema import ( + "bytes" + "crypto/sha1" "encoding/json" "errors" "fmt" @@ -42,6 +44,65 @@ func Read() (*serializers.Schema, error) { if err != nil { return nil, err } + if schema.SerializerVersion > serializers.SerializerVersion { + return nil, fmt.Errorf( + "%s was written by a newer testtrack CLI (serializer_version %d, this CLI supports %d). Please upgrade your testtrack CLI", + schemaPath, schema.SerializerVersion, serializers.SerializerVersion, + ) + } + 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, + ) + } + 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) { + schemaPath, exists := findSchemaPath() + if !exists { + return nil, errors.New("no testtrack schema file to upgrade. Run testtrack schema generate to create one") + } + schemaBytes, err := os.ReadFile(schemaPath) + if err != nil { + return nil, err + } + var schema serializers.Schema + err = yaml.Unmarshal(schemaBytes, &schema) + if err != nil { + return nil, err + } + if schema.SerializerVersion > serializers.SerializerVersion { + return nil, fmt.Errorf( + "%s was written by a newer testtrack CLI (serializer_version %d, this CLI writes %d). Please upgrade your testtrack CLI", + schemaPath, schema.SerializerVersion, serializers.SerializerVersion, + ) + } + + migrationRepo, err := migrationloaders.Load() + if err != nil { + return nil, err + } + schema.SchemaVersions = migrationRepo.SortedVersions() + schema.SerializerVersion = serializers.SerializerVersion + + err = Write(&schema) + if err != nil { + return nil, err + } return &schema, nil } @@ -63,9 +124,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,12 +266,28 @@ 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 { diff --git a/schema/schema_test.go b/schema/schema_test.go new file mode 100644 index 0000000..a9a8810 --- /dev/null +++ b/schema/schema_test.go @@ -0,0 +1,144 @@ +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 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 (the retail 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 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.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)) +} diff --git a/schemaloaders/schemaloaders.go b/schemaloaders/schemaloaders.go index cbf5d22..9f1d891 100644 --- a/schemaloaders/schemaloaders.go +++ b/schemaloaders/schemaloaders.go @@ -71,10 +71,19 @@ func (s *SchemaLoader) Load() error { } } + appliedVersions := make(map[string]bool, len(s.schema.SchemaVersions)) + for _, version := range s.schema.SchemaVersions { + appliedVersions[version] = true + } + + warnedOfUnrecordedMigrations := 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] { + if !warnedOfUnrecordedMigrations { + fmt.Println("Schema load complete, but there are migrations on disk not recorded in the schema file - run testtrack schema generate to update it.") + warnedOfUnrecordedMigrations = true + } + 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..272993f --- /dev/null +++ b/schemaloaders/schemaloaders_test.go @@ -0,0 +1,96 @@ +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 }() + + 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..b72e120 100644 --- a/serializers/serializers.go +++ b/serializers/serializers.go @@ -4,8 +4,14 @@ 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 migration file format so we can evolve 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 +96,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"`