Enforce hierarchical chart changes ordering (#151)

`groupChangesByChart` was returning a map but we expected the results to be ordered. In particular hierarchical order was preferred.

This change turns `groupChangesByChart` into `orderedChangesByChart` by taking the map generated before and passing it to `orderByChartHierarchy` which builds an array of `ChartChanges` and enforces the expected hierarchical order leveraging the chart full path: least deep paths go first, and at the same depth level we order alphabetically.

Signed-off-by: Jose Luis Vazquez Gonzalez <josvaz@vmware.com>

.github/workflows/golangci-lint.yml

@@ -20,6 +20,6 @@ jobs:
       - name: golangci-lint
         uses: golangci/golangci-lint-action@v2
         with:
-          version: v1.43
+          version: v1.45.2
           # Optional: show only new issues if it's a pull request. The default value is `false`.
           only-new-issues: true

pkg/mover/chart.go

@@ -10,6 +10,8 @@ import (
 	"os"
 	"path/filepath"
 	"regexp"
+	"sort"
+	"strings"
 
 	"github.com/google/go-containerregistry/pkg/authn"
 
@@ -153,6 +155,11 @@ type ChartMover struct {
 	rawHints []byte
 }
 
+type ChartChanges struct {
+	chart   *chart.Chart
+	changes []*internal.RewriteAction
+}
+
 // NewChartMover creates a ChartMover to relocate a chart following the given
 // imagePatters and rules.
 func NewChartMover(req *ChartMoveRequest, opts ...Option) (*ChartMover, error) {
@@ -399,20 +406,20 @@ func (cm *ChartMover) printMove() {
 			src, change.RewrittenReference.Name(), change.Digest, pushRequiredTxt)
 	}
 
-	for chartToModify, changes := range groupChangesByChart(cm.chartChanges, cm.chart) {
-		log.Printf("\nChanges to be applied to %s/values.yaml:\n", chartToModify.ChartFullPath())
-		for _, change := range changes {
+	for _, chartChanges := range orderedChangesByChart(cm.chartChanges, cm.chart) {
+		log.Printf("\nChanges to be applied to %s/values.yaml:\n", chartChanges.chart.ChartFullPath())
+		for _, change := range chartChanges.changes {
 			// Remove chart name from the path since we are already indicating what values.yaml file we are changing
-			log.Printf("  %s: %s\n", namespacedPath(change.Path, chartToModify.Name()), change.Value)
+			log.Printf("  %s: %s\n", namespacedPath(change.Path, chartChanges.chart.Name()), change.Value)
 		}
 
 		log.Println()
 	}
 }
 
-// Return the grouped set of changes by Helm Chart.
+// Return the ordered set of changes grouped by Helm Chart.
 // Meaning that changes to be performed to a given helm chart will be returned under the same map key
-func groupChangesByChart(changes []*internal.RewriteAction, rootChart *chart.Chart) map[*chart.Chart][]*internal.RewriteAction {
+func orderedChangesByChart(changes []*internal.RewriteAction, rootChart *chart.Chart) []ChartChanges {
 	groupedChanges := make(map[*chart.Chart][]*internal.RewriteAction)
 
 	for _, change := range changes {
@@ -424,7 +431,24 @@ func groupChangesByChart(changes []*internal.RewriteAction, rootChart *chart.Cha
 		}
 	}
 
-	return groupedChanges
+	return orderByChartHierarchy(groupedChanges)
+}
+
+func orderByChartHierarchy(groupedChanges map[*chart.Chart][]*internal.RewriteAction) []ChartChanges {
+	allChanges := make([]ChartChanges, 0, len(groupedChanges))
+
+	for chart, changes := range groupedChanges {
+		allChanges = append(allChanges, ChartChanges{chart: chart, changes: changes})
+	}
+
+	sort.Slice(allChanges, func(i, j int) bool {
+		a := allChanges[i].chart.ChartFullPath()
+		b := allChanges[j].chart.ChartFullPath()
+		pathA := strings.Split(a, "/")
+		pathB := strings.Split(b, "/")
+		return len(pathA) < len(pathB) || a < b
+	})
+	return allChanges
 }
 
 // namespacedPath removes the chartName from the beginning of the full path

pkg/mover/chart_test.go

@@ -714,9 +714,9 @@ func TestGroupChangesByChart(t *testing.T) {
 	rewrites := []*internal.RewriteAction{r1, subchart1R1, subchart1R2, subchart1Subchart3, subchart2R1}
 
 	// Expected output
-	want := make(map[*chart.Chart][]*internal.RewriteAction)
+	want := make([]ChartChanges, 4)
 	// parent chart
-	want[rootChart] = []*internal.RewriteAction{r1}
+	want[0] = ChartChanges{chart: rootChart, changes: []*internal.RewriteAction{r1}}
 
 	firstLevelDeps := rootChart.Dependencies()
 	// Sort dependencies since they come in arbitrary order
@@ -725,16 +725,16 @@ func TestGroupChangesByChart(t *testing.T) {
 	})
 
 	// Subchart1
-	want[firstLevelDeps[0]] = []*internal.RewriteAction{subchart1R1, subchart1R2}
+	want[1] = ChartChanges{chart: firstLevelDeps[0], changes: []*internal.RewriteAction{subchart1R1, subchart1R2}}
 
 	// Subchart2
-	want[firstLevelDeps[1]] = []*internal.RewriteAction{subchart2R1}
+	want[2] = ChartChanges{chart: firstLevelDeps[1], changes: []*internal.RewriteAction{subchart2R1}}
 
 	// Subchart1.Subchart3
-	want[firstLevelDeps[0].Dependencies()[0]] = []*internal.RewriteAction{subchart1Subchart3}
+	want[3] = ChartChanges{chart: firstLevelDeps[0].Dependencies()[0], changes: []*internal.RewriteAction{subchart1Subchart3}}
 
 	// Compare output
-	if got := groupChangesByChart(rewrites, rootChart); !reflect.DeepEqual(got, want) {
+	if got := orderedChangesByChart(rewrites, rootChart); !reflect.DeepEqual(got, want) {
 		t.Errorf("got=%v; want=%v", got, want)
 	}
 }