From 82fbdfac5f4866e61dc86326d09c7e2f2f8ac6ea Mon Sep 17 00:00:00 2001 From: Yuval Shavit <110620369+yuval-klotho@users.noreply.github.com> Date: Tue, 21 Feb 2023 09:57:00 -0500 Subject: [PATCH] fix pin-and-downgrade Previously, if you tried to pin to a lower version, we would treat it as a no-op (and keep you at your current version). Now, we'll perform the update ("downdate"). This resolves #187. We've had a few back-and-forths in this code, most recently #137. I added unit tests to try and put this to bed for good, or at least prevent further regressions. I found myself getting kinda confused about the various situations, so I wrote a novel to explain it to myself. I figured I'd keep it around for the next person. --- pkg/updater/updater.go | 30 ++++- pkg/updater/updater_test.go | 237 ++++++++++++++++++++++++++++++++++++ 2 files changed, 265 insertions(+), 2 deletions(-) create mode 100644 pkg/updater/updater_test.go diff --git a/pkg/updater/updater.go b/pkg/updater/updater.go index f4beaeae0..1b59a3173 100644 --- a/pkg/updater/updater.go +++ b/pkg/updater/updater.go @@ -20,7 +20,7 @@ var ( Arch string = runtime.GOARCH ) -var ( +const ( DefaultServer string = "http://srv.klo.dev" ) @@ -75,7 +75,33 @@ func (u *Updater) CheckUpdate(currentVersion string) (bool, error) { return false, fmt.Errorf("invalid version %s: %v", currentVersion, err) } - return currVersion.LessThan(*latestVersion) || strings.Split(u.CurrentStream, ":")[0] != strings.Split(u.Stream, ":")[0], nil + // Given a stream "xxx:yyyy", the qualifier is the "xxx" and the tag is the "yyyy". + // + // (1) If the qualifiers are different, always update (this is to handle open <--> pro) + // Otherwise, check the cli's version against latest. This is a bit trickier: + // + // (2a) If the tags are the same, then either it's a specific version or it's a monotonic tag like "latest". + // • If it's a monotonic tag, we only want to perform upgrades. A downgrade would be a situation like if we gave + // someone a pre-release, in which case we don't want to downgrade them. + // • If it's a specific version, we can assume that the version will never change. + // • So in either case, we want to only perform upgrades. + // (2b) If the tags are different, then someone is either pinning to a specific version, or going from a pinned + // version to a monotonic version. In either case, we should allow downgrades. (Going from pinned to monotonic + // *may* be an incorrect downgrade, with a similar pre-release reason. But if someone has a pre-release, they + // shouldn't be worrying about any upgrade stuff, including not changing their update stream from pinned to + // monotonic.) + + // case (1): different qualifiers always update + if strings.Split(u.CurrentStream, ":")[0] != strings.Split(u.Stream, ":")[0] { + return true, nil + } + + // the qualifiers are the same, so the tags are the same iff the full stream strings are the same + if u.CurrentStream == u.Stream { + return currVersion.LessThan(*latestVersion), nil // case (2a): only upgrades + } else { + return !currVersion.Equal(*latestVersion), nil // case (2b): upgrades or downgrades + } } // Update performs an update if a newer version is diff --git a/pkg/updater/updater_test.go b/pkg/updater/updater_test.go new file mode 100644 index 000000000..4f9b6abc8 --- /dev/null +++ b/pkg/updater/updater_test.go @@ -0,0 +1,237 @@ +package updater + +import ( + "encoding/json" + "github.com/stretchr/testify/assert" + "net/http" + "net/http/httptest" + "testing" +) + +func TestCheckUpdate(t *testing.T) { + type updateInputs struct { + buildStream string + currentVersion string + checkStream string + } + cases := []struct { + name string + cli updateInputs + // serverResponse is the server's answer for the latest version, or empty string to represent an error + serverResponse string + expectUpdate bool + expectError bool + }{ + { + name: `same stream, server has same version`, + cli: updateInputs{ + buildStream: `open:latest`, + currentVersion: `0.5.0`, + checkStream: `open:latest`, + }, + serverResponse: `0.5.0`, + expectUpdate: false, + }, + { + name: `same stream, server has newer version`, + cli: updateInputs{ + buildStream: `open:latest`, + currentVersion: `0.5.0`, + checkStream: `open:latest`, + }, + serverResponse: `0.5.1`, + expectUpdate: true, + }, + { + name: `same stream, server has older version`, + cli: updateInputs{ + buildStream: `open:latest`, + currentVersion: `0.5.1`, + checkStream: `open:latest`, + }, + serverResponse: `0.5.0`, + // This is typically unexpected, but can happen in development, or if we give someone a pre-release. + // We should not downgrade in either of those cases. + expectUpdate: false, + }, + { + name: `different stream, server has newer version`, + cli: updateInputs{ + buildStream: `open:latest`, + currentVersion: `0.5.0`, + checkStream: `pro:latest`, + }, + serverResponse: `0.5.1`, + expectUpdate: true, + }, + { + name: `different stream, server has same version`, + cli: updateInputs{ + buildStream: `open:latest`, + currentVersion: `0.5.0`, + checkStream: `pro:latest`, + }, + serverResponse: `0.5.0`, + expectUpdate: true, // both are 0.5.0, but we want to update to switch editions + }, + { + name: `different stream, server has older version`, + cli: updateInputs{ + buildStream: `open:latest`, + currentVersion: `0.5.1`, + checkStream: `pro:latest`, + }, + serverResponse: `0.5.1`, + expectUpdate: true, + }, + { + name: `pin to same stream, same version`, + cli: updateInputs{ + buildStream: `open:latest`, + currentVersion: `0.5.0`, + checkStream: `open:v0.5.0`, + }, + serverResponse: `0.5.0`, + // it's important that this is false, or else the user will keep getting messages to the effect of + // "you're on v0.1.2, but there's a new version, v0.1.2". + expectUpdate: false, + }, + { + name: `pin to same stream, newer version`, + cli: updateInputs{ + buildStream: `open:latest`, + currentVersion: `0.5.0`, + checkStream: `open:v0.5.1`, + }, + serverResponse: `0.5.1`, + expectUpdate: true, + }, + { + name: `pin to same stream, older version`, + cli: updateInputs{ + buildStream: `open:latest`, + currentVersion: `0.5.1`, + checkStream: `open:v0.5.0`, + }, + serverResponse: `0.5.0`, + expectUpdate: true, + }, + { + name: `pin to different stream, same version`, + cli: updateInputs{ + buildStream: `open:latest`, + currentVersion: `0.5.0`, + checkStream: `pro:v0.5.0`, + }, + serverResponse: `0.5.0`, + expectUpdate: true, + }, + { + name: `pin to different stream, newer version`, + cli: updateInputs{ + buildStream: `open:latest`, + currentVersion: `0.5.0`, + checkStream: `pro:v0.5.1`, + }, + serverResponse: `0.5.1`, + expectUpdate: true, + }, + { + name: `pin to different stream, older version`, + cli: updateInputs{ + buildStream: `open:latest`, + currentVersion: `0.5.1`, + checkStream: `pro:v0.5.0`, + }, + serverResponse: `0.5.0`, + expectUpdate: true, + }, + { + name: `pin to non-existent version`, + cli: updateInputs{ + buildStream: `open:latest`, + currentVersion: `0.5.1`, + checkStream: `open:v0.5.0`, + }, + expectError: true, + }, + } + for _, tt := range cases { + t.Run(tt.name, func(t *testing.T) { + assert := assert.New(t) + + handler := interactions{assert: assert} + rr := requestResponse{ + inUri: `/update/check-latest-version?stream=` + tt.cli.checkStream, + inMethod: http.MethodGet, + outStatus: http.StatusInternalServerError, // will be overwritten if serverResponse != "" + } + if tt.serverResponse != "" { + rr.outStatus = http.StatusOK + rr.outBody = map[string]string{`latest_version`: tt.serverResponse} + } + handler.interactions = append(handler.interactions, rr) + server := httptest.NewServer(&handler) + defer server.Close() + + updater := Updater{ + ServerURL: server.URL, + Stream: tt.cli.checkStream, + CurrentStream: tt.cli.buildStream, + } + needsUpdate, e := updater.CheckUpdate(tt.cli.currentVersion) + + if t.Failed() { + return // this means the mocked server failed (ie ran out of request-responses) + } + if !assert.Empty(handler.interactions, "didn't see all expected interactions") { + return + } + if tt.expectError { + assert.Error(e) + } else { + if assert.NoError(e) { + assert.Equal(needsUpdate, tt.expectUpdate) + } + } + }) + } +} + +type ( + requestResponse struct { + inMethod string + inUri string + outStatus int + outBody any + } + + interactions struct { + assert *assert.Assertions + interactions []requestResponse + } +) + +func (s *interactions) ServeHTTP(w http.ResponseWriter, r *http.Request) { + if len(s.interactions) == 0 { + s.assert.Fail("no interactions left") + w.WriteHeader(http.StatusInternalServerError) + return + } + curr := s.interactions[0] + s.interactions = s.interactions[1:] + + if s.assert.Equal(curr.inMethod, r.Method) && s.assert.Equal(curr.inUri, r.URL.RequestURI()) { + body, err := json.Marshal(curr.outBody) + if !s.assert.NoError(err) { + w.WriteHeader(http.StatusInternalServerError) + return + } + w.WriteHeader(curr.outStatus) + _, err = w.Write(body) + s.assert.NoError(err) + } else { + s.assert.Fail("no interactions left") + w.WriteHeader(http.StatusInternalServerError) + } +}