Merge pull request #4978 from ultimateboy/dont-delete-deployed

fix(storage): when pruning release versions, never delete the last deployed revision

pkg/storage/storage.go

@@ -181,21 +181,37 @@ func (s *Storage) removeLeastRecent(name string, max int) error {
 	if len(h) <= max {
 		return nil
 	}
-	overage := len(h) - max
 
 	// We want oldest to newest
 	relutil.SortByRevision(h)
 
+	lastDeployed, err := s.Deployed(name)
+	if err != nil {
+		return err
+	}
+
+	var toDelete []*rspb.Release
+	for _, rel := range h {
+		// once we have enough releases to delete to reach the max, stop
+		if len(h)-len(toDelete) == max {
+			break
+		}
+		if lastDeployed != nil {
+			if rel.GetVersion() != lastDeployed.GetVersion() {
+				toDelete = append(toDelete, rel)
+			}
+		} else {
+			toDelete = append(toDelete, rel)
+		}
+	}
+
 	// Delete as many as possible. In the case of API throughput limitations,
 	// multiple invocations of this function will eventually delete them all.
-	toDelete := h[0:overage]
 	errors := []error{}
 	for _, rel := range toDelete {
-		key := makeKey(name, rel.Version)
+		err = s.deleteReleaseVersion(name, rel.GetVersion())
-		_, innerErr := s.Delete(name, rel.Version)
+		if err != nil {
-		if innerErr != nil {
+			errors = append(errors, err)
-			s.Log("error pruning %s from release history: %s", key, innerErr)
-			errors = append(errors, innerErr)
 		}
 	}
 
@@ -210,6 +226,16 @@ func (s *Storage) removeLeastRecent(name string, max int) error {
 	}
 }
 
+func (s *Storage) deleteReleaseVersion(name string, version int32) error {
+	key := makeKey(name, version)
+	_, err := s.Delete(name, version)
+	if err != nil {
+		s.Log("error pruning %s from release history: %s", key, err)
+		return err
+	}
+	return nil
+}
+
 // Last fetches the last revision of the named release.
 func (s *Storage) Last(name string) (*rspb.Release, error) {
 	s.Log("getting last revision of %q", name)

pkg/storage/storage_test.go

@@ -293,6 +293,57 @@ func TestStorageRemoveLeastRecent(t *testing.T) {
 	}
 }
 
+func TestStorageDontDeleteDeployed(t *testing.T) {
+	storage := Init(driver.NewMemory())
+	storage.Log = t.Logf
+	storage.MaxHistory = 3
+
+	const name = "angry-bird"
+
+	// setup storage with test releases
+	setup := func() {
+		// release records
+		rls0 := ReleaseTestData{Name: name, Version: 1, Status: rspb.Status_SUPERSEDED}.ToRelease()
+		rls1 := ReleaseTestData{Name: name, Version: 2, Status: rspb.Status_DEPLOYED}.ToRelease()
+		rls2 := ReleaseTestData{Name: name, Version: 3, Status: rspb.Status_FAILED}.ToRelease()
+		rls3 := ReleaseTestData{Name: name, Version: 4, Status: rspb.Status_FAILED}.ToRelease()
+
+		// create the release records in the storage
+		assertErrNil(t.Fatal, storage.Create(rls0), "Storing release 'angry-bird' (v1)")
+		assertErrNil(t.Fatal, storage.Create(rls1), "Storing release 'angry-bird' (v2)")
+		assertErrNil(t.Fatal, storage.Create(rls2), "Storing release 'angry-bird' (v3)")
+		assertErrNil(t.Fatal, storage.Create(rls3), "Storing release 'angry-bird' (v4)")
+	}
+	setup()
+
+	rls5 := ReleaseTestData{Name: name, Version: 5, Status: rspb.Status_FAILED}.ToRelease()
+	assertErrNil(t.Fatal, storage.Create(rls5), "Storing release 'angry-bird' (v5)")
+
+	// On inserting the 5th record, we expect a total of 3 releases, but we expect version 2
+	// (the only deployed release), to still exist
+	hist, err := storage.History(name)
+	if err != nil {
+		t.Fatal(err)
+	} else if len(hist) != storage.MaxHistory {
+		for _, item := range hist {
+			t.Logf("%s %v", item.Name, item.Version)
+		}
+		t.Fatalf("expected %d items in history, got %d", storage.MaxHistory, len(hist))
+	}
+
+	expectedVersions := map[int32]bool{
+		2: true,
+		4: true,
+		5: true,
+	}
+
+	for _, item := range hist {
+		if !expectedVersions[item.GetVersion()] {
+			t.Errorf("Release version %d, found when not expected", item.GetVersion())
+		}
+	}
+}
+
 func TestStorageLast(t *testing.T) {
 	storage := Init(driver.NewMemory())