lib/upgrade: Prefer a minor upgrade over a major (fixes #3163)

GitHub-Pull-Request: https://github.com/syncthing/syncthing/pull/3184

lib/upgrade/upgrade_supported.go

@@ -129,11 +129,31 @@ func SelectLatestRelease(version string, rels []Release) (Release, error) {
 		return Release{}, ErrNoVersionToSelect
 	}
 
-	sort.Sort(SortByRelease(rels))
+	// Sort the releases, lowest version number first
+	sort.Sort(sort.Reverse(SortByRelease(rels)))
 	// Check for a beta build
 	beta := strings.Contains(version, "-")
 
+	var selected Release
 	for _, rel := range rels {
+		switch CompareVersions(rel.Tag, version) {
+		case Older, MajorOlder:
+			// This is older than what we're already running
+			continue
+
+		case MajorNewer:
+			// We've found a new major version. That's fine, but if we've
+			// already found a minor upgrade that is acceptable we should go
+			// with that one first and then revisit in the future.
+			if selected.Tag != "" && CompareVersions(selected.Tag, version) == Newer {
+				return selected, nil
+			}
+			// else it may be viable, do the needful below
+
+		default:
+			// New minor release, do the usual processing
+		}
+
 		if rel.Prerelease && !beta {
 			continue
 		}
@@ -144,11 +164,16 @@ func SelectLatestRelease(version string, rels []Release) (Release, error) {
 			l.Debugf("expected release asset %q", expectedRelease)
 			l.Debugln("considering release", assetName)
 			if strings.HasPrefix(assetName, expectedRelease) {
-				return rel, nil
+				selected = rel
 			}
 		}
 	}
-	return Release{}, ErrNoReleaseDownload
+
+	if selected.Tag == "" {
+		return Release{}, ErrNoReleaseDownload
+	}
+
+	return selected, nil
 }
 
 // Upgrade to the given release, saving the previous binary with a ".old" extension.

lib/upgrade/upgrade_test.go

@@ -11,6 +11,7 @@ package upgrade
 import (
 	"encoding/json"
 	"os"
+	"strings"
 	"testing"
 )
 
@@ -58,16 +59,15 @@ func TestCompareVersions(t *testing.T) {
 	}
 }
 
-var upgrades = map[string]string{
-	"v0.10.21":                        "v0.10.30",
-	"v0.10.29":                        "v0.10.30",
-	"v0.10.31":                        "v0.10.30",
-	"v0.10.0-alpha":                   "v0.11.0-beta0",
-	"v0.10.0-beta":                    "v0.11.0-beta0",
-	"v0.11.0-beta0+40-g53cb66e-dirty": "v0.11.0-beta0",
-}
-
 func TestGithubRelease(t *testing.T) {
+	var upgrades = map[string]string{
+		"v0.10.21":                        "v0.10.30",
+		"v0.10.29":                        "v0.10.30",
+		"v0.10.0-alpha":                   "v0.10.30",
+		"v0.10.0-beta":                    "v0.10.30",
+		"v0.11.0-beta0+40-g53cb66e-dirty": "v0.11.0-beta0",
+	}
+
 	fd, err := os.Open("testdata/github-releases.json")
 	if err != nil {
 		t.Errorf("Missing github-release test data")
@@ -94,3 +94,48 @@ func TestErrorRelease(t *testing.T) {
 		t.Error("Should return an error when no release were available")
 	}
 }
+
+func TestSelectedRelease(t *testing.T) {
+	testcases := []struct {
+		current    string
+		candidates []string
+		selected   string
+	}{
+		// Within the same "major" (minor, in this case) select the newest
+		{"v0.12.24", []string{"v0.12.23", "v0.12.24", "v0.12.25", "v0.12.26"}, "v0.12.26"},
+		// Do no select beta versions when we are not a beta
+		{"v0.12.24", []string{"v0.12.26", "v0.12.27-beta.42"}, "v0.12.26"},
+		// Do select beta versions when we are a beta
+		{"v0.12.24-beta.0", []string{"v0.12.26", "v0.12.27-beta.42"}, "v0.12.27-beta.42"},
+		// Select the best within the current major when there is a minor upgrade available
+		{"v0.12.24", []string{"v0.12.23", "v0.12.24", "v0.12.25", "v0.13.0"}, "v0.12.25"},
+		{"v1.12.24", []string{"v1.12.23", "v1.12.24", "v1.14.2", "v2.0.0"}, "v1.14.2"},
+		// Select the next major when we are at the best minor
+		{"v0.12.25", []string{"v0.12.23", "v0.12.24", "v0.12.25", "v0.13.0"}, "v0.13.0"},
+		{"v1.14.2", []string{"v0.12.23", "v0.12.24", "v1.14.2", "v2.0.0"}, "v2.0.0"},
+	}
+
+	for i, tc := range testcases {
+		// Prepare a list of candidate releases
+		var rels []Release
+		for _, c := range tc.candidates {
+			rels = append(rels, Release{
+				Tag:        c,
+				Prerelease: strings.Contains(c, "-"),
+				Assets: []Asset{
+					// There must be a matching asset or it will not get selected
+					{Name: releaseName(c)},
+				},
+			})
+		}
+
+		// Check the selection
+		sel, err := SelectLatestRelease(tc.current, rels)
+		if err != nil {
+			t.Fatal("Unexpected error:", err)
+		}
+		if sel.Tag != tc.selected {
+			t.Errorf("Test case %d: expected %s to be selected, but got %s", i, tc.selected, sel.Tag)
+		}
+	}
+}