Repository: helm/helm
Pull request #10085 (2021-08-31 14:30:03)

"... has no deployed releases" error when release history contains only failed releases and history limit reached
Fixed old-releases rotation procedure to not require a deployed release to exists.

An error will arise when there are no successfully deployed release yet, but releases history limit has been reached. In such situation helm will refuse to upgrade release anymore with "... has no deployed releases" error.

Furthermore, release rotation procedure already expecting lastDeployedRelease to be either nil, or not nil. So it is assumed that deployed release may exist or may not and these both outcomes were already expected as a valid situation rather than a failure.


distorhead: Not sure what to do with this test case: https://github.com/helm/helm/commit/8c28da65676a190623ac1d10711780e58e574a04#diff-82890bcea98fd02fb22d71e2add4a8536950a0198fa2e604b73c5882e3ef2d94R300

I see initial motivation to add this test case was to "check that removeLeastRecent error will fail release.Create procedure": https://github.com/helm/helm/commit/00cf10d360de3fbe440789ee51662c2894e041ce#diff-8b98b331c5d4acbeb7274c68973d20900daaed47c8d8f3e62ba39284379166bbL64

So removeLeastRecent procedure could raise any error just for error-handling check. I have removed ErrNoDeployedReleases, which has been used as an example error, which removeLeastRecent function could produce. Not sure yet what "example-error" we could use for this test case as a replacement for ErrNoDeployedReleases.

yxxhero: Can you add some unittest for this change? @distorhead  Thanks very much.
distorhead: > Can you add some unittest for this change? @distorhead Thanks very much.

@yxxhero Fixed test case.
zonggen: It would be great if you create an issue associated to this PR and gets triaged as `bug`([link to contribution procedure](https://github.com/helm/helm/blob/main/CONTRIBUTING.md#how-to-contribute-a-patch)).
distorhead: Added issue: https://github.com/helm/helm/issues/10121
Added new test case for issue.
distorhead: @hickeyma Hello, is there any problem with this PR that should be fixed?
hickeyma: @distorhead Seems to be an issue with circle build. do you mins pouching again to kick off a build?

Patch:
diff --git a/pkg/storage/storage.go b/pkg/storage/storage.go
index 370fec4b4..0a18b34a0 100644
--- a/pkg/storage/storage.go
+++ b/pkg/storage/storage.go
@@ -177,7 +177,7 @@ func (s *Storage) removeLeastRecent(name string, max int) error {
 	relutil.SortByRevision(h)
 
 	lastDeployed, err := s.Deployed(name)
-	if err != nil {
+	if err != nil && !errors.Is(err, driver.ErrNoDeployedReleases) {
 		return err
 	}
 


Test patch:
diff --git a/pkg/storage/storage_test.go b/pkg/storage/storage_test.go
index ac79ca2fd..058b077e8 100644
--- a/pkg/storage/storage_test.go
+++ b/pkg/storage/storage_test.go
@@ -278,8 +278,40 @@ func TestStorageHistory(t *testing.T) {
 	}
 }
 
-func TestStorageRemoveLeastRecentWithError(t *testing.T) {
-	storage := Init(driver.NewMemory())
+var errMaxHistoryMockDriverSomethingHappened = errors.New("something happened")
+
+type MaxHistoryMockDriver struct {
+	Driver driver.Driver
+}
+
+func NewMaxHistoryMockDriver(d driver.Driver) *MaxHistoryMockDriver {
+	return &MaxHistoryMockDriver{Driver: d}
+}
+func (d *MaxHistoryMockDriver) Create(key string, rls *rspb.Release) error {
+	return d.Driver.Create(key, rls)
+}
+func (d *MaxHistoryMockDriver) Update(key string, rls *rspb.Release) error {
+	return d.Driver.Update(key, rls)
+}
+func (d *MaxHistoryMockDriver) Delete(key string) (*rspb.Release, error) {
+	return nil, errMaxHistoryMockDriverSomethingHappened
+}
+func (d *MaxHistoryMockDriver) Get(key string) (*rspb.Release, error) {
+	return d.Driver.Get(key)
+}
+func (d *MaxHistoryMockDriver) List(filter func(*rspb.Release) bool) ([]*rspb.Release, error) {
+	return d.Driver.List(filter)
+}
+func (d *MaxHistoryMockDriver) Query(labels map[string]string) ([]*rspb.Release, error) {
+	return d.Driver.Query(labels)
+}
+func (d *MaxHistoryMockDriver) Name() string {
+	return d.Driver.Name()
+}
+
+func TestMaxHistoryErrorHandling(t *testing.T) {
+	//func TestStorageRemoveLeastRecentWithError(t *testing.T) {
+	storage := Init(NewMaxHistoryMockDriver(driver.NewMemory()))
 	storage.Log = t.Logf
 
 	storage.MaxHistory = 1
@@ -297,7 +329,7 @@ func TestStorageRemoveLeastRecentWithError(t *testing.T) {
 	setup()
 
 	rls2 := ReleaseTestData{Name: name, Version: 2, Status: rspb.StatusSuperseded}.ToRelease()
-	wantErr := driver.ErrNoDeployedReleases
+	wantErr := errMaxHistoryMockDriverSomethingHappened
 	gotErr := storage.Create(rls2)
 	if !errors.Is(gotErr, wantErr) {
 		t.Fatalf("Storing release 'angry-bird' (v2) should return the error %#v, but returned %#v", wantErr, gotErr)
@@ -444,6 +476,65 @@ func TestStorageLast(t *testing.T) {
 	}
 }
 
+// TestUpgradeInitiallyFailedRelease tests a case when there are no deployed release yet, but history limit has been
+// reached: the has-no-deployed-releases error should not occur in such case.
+func TestUpgradeInitiallyFailedReleaseWithHistoryLimit(t *testing.T) {
+	storage := Init(driver.NewMemory())
+	storage.MaxHistory = 4
+
+	const name = "angry-bird"
+
+	// setup storage with test releases
+	setup := func() {
+		// release records
+		rls0 := ReleaseTestData{Name: name, Version: 1, Status: rspb.StatusFailed}.ToRelease()
+		rls1 := ReleaseTestData{Name: name, Version: 2, Status: rspb.StatusFailed}.ToRelease()
+		rls2 := ReleaseTestData{Name: name, Version: 3, Status: rspb.StatusFailed}.ToRelease()
+		rls3 := ReleaseTestData{Name: name, Version: 4, Status: rspb.StatusFailed}.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)")
+
+		hist, err := storage.History(name)
+		if err != nil {
+			t.Fatalf("unexpected error: %s", err)
+		}
+
+		wantHistoryLen := 4
+		if len(hist) != wantHistoryLen {
+			t.Fatalf("expected history of release %q to contain %d releases, got %d", name, wantHistoryLen, len(hist))
+		}
+	}
+
+	setup()
+
+	rls5 := ReleaseTestData{Name: name, Version: 5, Status: rspb.StatusFailed}.ToRelease()
+	err := storage.Create(rls5)
+	if err != nil {
+		t.Fatalf("Failed to create a new release version: %s", err)
+	}
+
+	hist, err := storage.History(name)
+	if err != nil {
+		t.Fatalf("unexpected error: %s", err)
+	}
+
+	for i, rel := range hist {
+		wantVersion := i + 2
+		if rel.Version != wantVersion {
+			t.Fatalf("Expected history release %d version to equal %d, got %d", i+1, wantVersion, rel.Version)
+		}
+
+		wantStatus := rspb.StatusFailed
+		if rel.Info.Status != wantStatus {
+			t.Fatalf("Expected history release %d status to equal %q, got %q", i+1, wantStatus, rel.Info.Status)
+		}
+	}
+}
+
 type ReleaseTestData struct {
 	Name      string
 	Version   int