From 11ef72d7a32930f814f5822ce1be4365bcb1a551 Mon Sep 17 00:00:00 2001
From: Liu Ming <hit_oak_tree@126.com>
Date: Thu, 11 Jun 2020 14:46:07 +0800
Subject: [PATCH] fix: CRs install/uninstall order

We expect that installing CRs as the order of CRs in the raw manifests,
and uninstalling CRs as the reversed order of CRs in the raw manifests.
And providing test cases to confirm this PR has solved #8291.
-

close #8291

Signed-off-by: Liu Ming <hit_oak_tree@126.com>
---
 pkg/releaseutil/kind_sorter.go      | 10 +--
 pkg/releaseutil/kind_sorter_test.go | 94 +++++++++++++++++++++++++++--
 2 files changed, 95 insertions(+), 9 deletions(-)

diff --git a/pkg/releaseutil/kind_sorter.go b/pkg/releaseutil/kind_sorter.go
index a340dfc29..2034ba89d 100644
--- a/pkg/releaseutil/kind_sorter.go
+++ b/pkg/releaseutil/kind_sorter.go
@@ -17,6 +17,7 @@ limitations under the License.
 package releaseutil
 
 import (
+	"reflect"
 	"sort"
 
 	"helm.sh/helm/v3/pkg/release"
@@ -137,12 +138,11 @@ func lessByKind(a interface{}, b interface{}, kindA string, kindB string, o Kind
 	first, aok := ordering[kindA]
 	second, bok := ordering[kindB]
 
+	// both kinds are unknown
+	// the installing order of the unknown kinds is as the order that they are in original manifests/hooks,
+	// and the uninstalling order is the reverse order that they are in original manifests/hooks.
 	if !aok && !bok {
-		// if both are unknown then sort alphabetically by kind, keep original order if same kind
-		if kindA != kindB {
-			return kindA < kindB
-		}
-		return first < second
+		return reflect.DeepEqual(o, UninstallOrder)
 	}
 	// unknown kind is last
 	if !aok {
diff --git a/pkg/releaseutil/kind_sorter_test.go b/pkg/releaseutil/kind_sorter_test.go
index 71d355210..8d037a51b 100644
--- a/pkg/releaseutil/kind_sorter_test.go
+++ b/pkg/releaseutil/kind_sorter_test.go
@@ -18,13 +18,14 @@ package releaseutil
 
 import (
 	"bytes"
+	"sort"
 	"testing"
 
 	"helm.sh/helm/v3/pkg/release"
 )
 
-func TestKindSorter(t *testing.T) {
-	manifests := []Manifest{
+func buildManifestsForTestKindSorter() []Manifest {
+	return []Manifest{
 		{
 			Name: "E",
 			Head: &SimpleHead{Kind: "SecretList"},
@@ -165,15 +166,31 @@ func TestKindSorter(t *testing.T) {
 			Name: "x",
 			Head: &SimpleHead{Kind: "HorizontalPodAutoscaler"},
 		},
+		{
+			Name: "N",
+			Head: &SimpleHead{Kind: "NginxVhost"},
+		},
+		{
+			Name: "U",
+			Head: &SimpleHead{Kind: "Unknown"},
+		},
+		{
+			Name: "R",
+			Head: &SimpleHead{Kind: "Registration"},
+		},
 	}
+}
+
+func TestKindSorter(t *testing.T) {
+	manifests := buildManifestsForTestKindSorter()
 
 	for _, test := range []struct {
 		description string
 		order       KindSortOrder
 		expected    string
 	}{
-		{"install", InstallOrder, "aAbcC3deEf1gh2iIjJkKlLmnopqrxstuvw!"},
-		{"uninstall", UninstallOrder, "wvmutsxrqponLlKkJjIi2hg1fEed3CcbAa!"},
+		{"install", InstallOrder, "aAbcC3deEf1gh2iIjJkKlLmnopqrxstuvw!NUR"},
+		{"uninstall", UninstallOrder, "wvmutsxrqponLlKkJjIi2hg1fEed3CcbAaRUN!"},
 	} {
 		var buf bytes.Buffer
 		t.Run(test.description, func(t *testing.T) {
@@ -197,6 +214,40 @@ func TestKindSorter(t *testing.T) {
 	}
 }
 
+func TestKindSorterFailedUsingAlphabeticalOrder(t *testing.T) {
+	manifests := buildManifestsForTestKindSorter()
+
+	for _, test := range []struct {
+		description string
+		order       KindSortOrder
+		expected    string
+		actual      string
+	}{
+		{"install", InstallOrder, "aAbcC3deEf1gh2iIjJkKlLmnopqrxstuvw!NUR", "aAbcC3deEf1gh2iIjJkKlLmnopqrxstuvw!NRU"},
+		{"uninstall", UninstallOrder, "wvmutsxrqponLlKkJjIi2hg1fEed3CcbAaRUN!", "wvmutsxrqponLlKkJjIi2hg1fEed3CcbAa!NRU"},
+	} {
+		var buf bytes.Buffer
+		t.Run(test.description, func(t *testing.T) {
+			if got, want := len(test.expected), len(manifests); got != want {
+				t.Fatalf("Expected %d names in order, got %d", want, got)
+			}
+			defer buf.Reset()
+			orig := manifests
+			for _, r := range sortManifestsByKindForCRsUsingAlphabeticalOrder(manifests, test.order) {
+				buf.WriteString(r.Name)
+			}
+			if got := buf.String(); got == test.actual && got != test.expected {
+				t.Logf("Expected %q, got %q", test.expected, got)
+			}
+			for i, manifest := range orig {
+				if manifest != manifests[i] {
+					t.Fatal("Expected input to sortManifestsByKind to stay the same")
+				}
+			}
+		})
+	}
+}
+
 // TestKindSorterKeepOriginalOrder verifies manifests of same kind are kept in original order
 func TestKindSorterKeepOriginalOrder(t *testing.T) {
 	manifests := []Manifest{
@@ -329,3 +380,38 @@ func TestKindSorterForHooks(t *testing.T) {
 		})
 	}
 }
+
+func sortManifestsByKindForCRsUsingAlphabeticalOrder(manifests []Manifest, ordering KindSortOrder) []Manifest {
+	sort.SliceStable(manifests, func(i, j int) bool {
+		return lessByKindForCRsUsingAlphabeticalOrder(manifests[i], manifests[j], manifests[i].Head.Kind, manifests[j].Head.Kind, ordering)
+	})
+
+	return manifests
+}
+
+func lessByKindForCRsUsingAlphabeticalOrder(a interface{}, b interface{}, kindA string, kindB string, o KindSortOrder) bool {
+	ordering := make(map[string]int, len(o))
+	for v, k := range o {
+		ordering[k] = v
+	}
+
+	first, aok := ordering[kindA]
+	second, bok := ordering[kindB]
+
+	if !aok && !bok {
+		// if both are unknown then sort alphabetically by kind, keep original order if same kind
+		if kindA != kindB {
+			return kindA < kindB
+		}
+		return first < second
+	}
+	// unknown kind is last
+	if !aok {
+		return false
+	}
+	if !bok {
+		return true
+	}
+	// sort different kinds, keep original order if same priority
+	return first < second
+}