Apply suggestions from code review

Co-authored-by: Lili Cosic <cosiclili@gmail.com>

server/storage/schema/actions.go

@@ -73,17 +73,17 @@ func (as ActionList) unsafeExecute(lg *zap.Logger, tx backend.BatchTx) error {
 		revert, err := a.unsafeDo(tx)
 
 		if err != nil {
-			revertActions.unsafeExecuteInReverseOrder(lg, tx)
+			revertActions.unsafeExecuteInReversedOrder(lg, tx)
 			return err
 		}
 		revertActions = append(revertActions, revert)
 	}
 	return nil
 }
 
-// unsafeExecuteInReverseOrder executes actions in revered order. Will panic on
+// unsafeExecuteInReversedOrder executes actions in revered order. Will panic on
 // action error. Should be used when reverting.
-func (as ActionList) unsafeExecuteInReverseOrder(lg *zap.Logger, tx backend.BatchTx) {
+func (as ActionList) unsafeExecuteInReversedOrder(lg *zap.Logger, tx backend.BatchTx) {
 	for j := len(as) - 1; j >= 0; j-- {
 		_, err := as[j].unsafeDo(tx)
 		if err != nil {

server/storage/schema/migration.go

@@ -25,26 +25,28 @@ import (
 type migrationPlan []migrationStep
 
 func newPlan(lg *zap.Logger, current semver.Version, target semver.Version) (p migrationPlan, err error) {
-	if current.Major != target.Major {
-		lg.Error("Changing major storage version is not supported",
-			zap.String("storage-version", current.String()),
-			zap.String("target-storage-version", target.String()),
-		)
-		return nil, fmt.Errorf("Changing major storage version is not supported")
-	}
 	// TODO(serathius): Implement downgrades
-	if current.Minor > target.Minor {
+	if target.LessThan(current) {
 		lg.Error("Target version is lower than the current version, downgrades are not yet supported",
 			zap.String("storage-version", current.String()),
 			zap.String("target-storage-version", target.String()),
 		)
 		return nil, fmt.Errorf("downgrades are not yet supported")
 	}
-	return buildPlan(current, target)
+	return buildPlan(lg, current, target)
 }
 
-func buildPlan(current semver.Version, target semver.Version) (plan migrationPlan, err error) {
-	for current.Minor != target.Minor {
+func buildPlan(lg *zap.Logger, current semver.Version, target semver.Version) (plan migrationPlan, err error) {
+	current = trimToMinor(current)
+	target = trimToMinor(target)
+	if current.Major != target.Major {
+		lg.Error("Changing major storage version is not supported",
+			zap.String("storage-version", current.String()),
+			zap.String("target-storage-version", target.String()),
+		)
+		return plan, fmt.Errorf("changing major storage version is not supported")
+	}
+	for !current.Equal(target) {
 		isUpgrade := current.Minor < target.Minor
 
 		changes, err := schemaChangesForVersion(current, isUpgrade)
@@ -117,3 +119,7 @@ func (s migrationStep) unsafeExecute(lg *zap.Logger, tx backend.BatchTx) error {
 	}
 	return nil
 }
+
+func trimToMinor(ver semver.Version) semver.Version {
+	return semver.Version{Major: ver.Major, Minor: ver.Minor}
+}

server/storage/schema/migration_test.go

@@ -26,6 +26,61 @@ import (
 	"go.uber.org/zap/zaptest"
 )
 
+var (
+	V4_0 = semver.Version{Major: 4, Minor: 0}
+)
+
+func TestNewPlan(t *testing.T) {
+	tcs := []struct {
+		name string
+
+		current semver.Version
+		target  semver.Version
+
+		expectError    bool
+		expectErrorMsg string
+	}{
+		{
+			name:    "Update v3.5 to v3.6 should work",
+			current: V3_5,
+			target:  V3_6,
+		},
+		{
+			name:           "Downgrade v3.6 to v3.5 should fail as downgrades are not yet supported",
+			current:        V3_6,
+			target:         V3_5,
+			expectError:    true,
+			expectErrorMsg: "downgrades are not yet supported",
+		},
+		{
+			name:           "Upgrade v3.6 to v3.7 should fail as v3.7 is unknown",
+			current:        V3_6,
+			target:         V3_7,
+			expectError:    true,
+			expectErrorMsg: `version "3.7.0" is not supported`,
+		},
+		{
+			name:           "Upgrade v3.6 to v4.0 as major version changes are unsupported",
+			current:        V3_6,
+			target:         V4_0,
+			expectError:    true,
+			expectErrorMsg: "changing major storage version is not supported",
+		},
+	}
+	for _, tc := range tcs {
+		t.Run(tc.name, func(t *testing.T) {
+			lg := zaptest.NewLogger(t)
+			_, err := newPlan(lg, tc.current, tc.target)
+			if (err != nil) != tc.expectError {
+				t.Errorf("newPlan(lg, %q, %q) returned unexpected error (or lack thereof), expected: %v, got: %v", tc.current, tc.target, tc.expectError, err)
+			}
+			if err != nil && err.Error() != tc.expectErrorMsg {
+				t.Errorf("newPlan(lg, %q, %q) returned unexpected error message, expected: %q, got: %q", tc.current, tc.target, tc.expectErrorMsg, err.Error())
+			}
+		})
+	}
+}
+
 func TestMigrationStepExecute(t *testing.T) {
 	recorder := &actionRecorder{}
 	errorC := fmt.Errorf("error C")

server/storage/schema/schema.go

@@ -113,14 +113,14 @@ func schemaChangesForVersion(v semver.Version, isUpgrade bool) ([]schemaChange,
 }
 
 var (
-	// schemaChanges list changes that were introduced in perticular version.
+	// schemaChanges list changes that were introduced in a particular version.
 	// schema was introduced in v3.6 as so its changes were not tracked before.
 	schemaChanges = map[semver.Version][]schemaChange{
 		V3_6: {
 			addNewField(Meta, MetaStorageVersionName, emptyStorageVersion),
 		},
 	}
 	// emptyStorageVersion is used for v3.6 Step for the first time, in all other version StoragetVersion should be set by migrator.
-	// Adding a addNewField for StorageVersion we can reuselogic to remove it when downgrading to v3.5
+	// Adding a addNewField for StorageVersion we can reuse logic to remove it when downgrading to v3.5
 	emptyStorageVersion = []byte("")
 )

server/storage/schema/schema_test.go

@@ -130,7 +130,7 @@ func TestMigrate(t *testing.T) {
 			expectErrorMsg: `cannot detect storage schema version: missing term information`,
 		},
 		{
-			name:          `Upgrading v3.5 to v3.6 should be succeed all required fields are set`,
+			name:          `Upgrading v3.5 to v3.6 should succeed; all required fields are set`,
 			version:       V3_5,
 			targetVersion: V3_6,
 			expectVersion: &V3_6,
@@ -268,7 +268,7 @@ func testUnsafeMigrate(lg *zap.Logger, tx backend.BatchTx, target semver.Version
 	if err != nil {
 		return fmt.Errorf("cannot determine storage version: %w", err)
 	}
-	plan, err := buildPlan(current, target)
+	plan, err := buildPlan(lg, current, target)
 	if err != nil {
 		return fmt.Errorf("cannot create migration plan: %w", err)
 	}