Merge pull request #393 from dokku/341-bug-planned-and-executed-resource-command-argument-order-can-differ

fix: sort resource and ps-scale command arguments

Author: josegonzalez

tasks/ps_scale_task.go

@@ -2,9 +2,11 @@ package tasks
 
 import (
 	"fmt"
-	"github.com/dokku/docket/subprocess"
+	"sort"
 	"strconv"
 	"strings"
+
+	"github.com/dokku/docket/subprocess"
 )
 
 // PsScaleTask manages the process scale for a given dokku application
@@ -100,7 +102,15 @@ func (t PsScaleTask) Plan() PlanResult {
 			}
 			toScale := []string{}
 			mutations := []string{}
-			for proctype, qty := range t.Scale {
+			// Range over sorted process types so the ps:scale args and the
+			// mutation list are deterministic across runs (issue #341).
+			proctypes := make([]string, 0, len(t.Scale))
+			for proctype := range t.Scale {
+				proctypes = append(proctypes, proctype)
+			}
+			sort.Strings(proctypes)
+			for _, proctype := range proctypes {
+				qty := t.Scale[proctype]
 				if cur, ok := existing[proctype]; ok && cur == qty {
 					continue
 				}

tasks/ps_scale_task_test.go

@@ -1,8 +1,10 @@
 package tasks
 
 import (
+	"reflect"
 	"testing"
 
+	"github.com/dokku/docket/subprocess"
 	_ "github.com/gliderlabs/sigil/builtin"
 )
 
@@ -30,6 +32,39 @@ func TestPsScaleTaskNilScale(t *testing.T) {
 	}
 }
 
+func TestPsScaleCommandDeterministicOrder(t *testing.T) {
+	// web and worker both drift, so the ps:scale command lists both (appended
+	// after the app) and both mutations are reported. Sorting the process types
+	// must yield byte-identical output on every run (issue #341).
+	defer subprocess.SetExecRunner(fakeDokku(map[string]string{
+		"--quiet ps:scale test-app": "web: 1\nworker: 1",
+	}))()
+
+	task := PsScaleTask{
+		App:   "test-app",
+		Scale: map[string]int{"worker": 3, "web": 2},
+		State: StatePresent,
+	}
+
+	wantCommands := []string{"dokku ps:scale test-app web=2 worker=3"}
+	wantMutations := []string{"scale web=2 (was 1)", "scale worker=3 (was 1)"}
+
+	// Repeat so a reintroduced map-order bug is caught reliably rather than
+	// passing by chance on a lucky iteration.
+	for i := 0; i < 20; i++ {
+		plan := task.Plan()
+		if plan.Error != nil {
+			t.Fatalf("iteration %d: unexpected plan error: %v", i, plan.Error)
+		}
+		if !reflect.DeepEqual(plan.Commands, wantCommands) {
+			t.Fatalf("iteration %d commands = %v, want %v", i, plan.Commands, wantCommands)
+		}
+		if !reflect.DeepEqual(plan.Mutations, wantMutations) {
+			t.Fatalf("iteration %d mutations = %v, want %v", i, plan.Mutations, wantMutations)
+		}
+	}
+}
+
 func TestGetTasksPsScaleTaskParsedCorrectly(t *testing.T) {
 	data := []byte(`---
 - tasks:

tasks/resource_limit_task_test.go

@@ -1,6 +1,7 @@
 package tasks
 
 import (
+	"reflect"
 	"strings"
 	"testing"
 
@@ -113,6 +114,39 @@ func TestResourceLimitClearBeforeClearsNonDesiredResource(t *testing.T) {
 	}
 }
 
+func TestResourceLimitSetCommandDeterministicOrder(t *testing.T) {
+	// Both cpu and memory drift, so the set command carries both flags and both
+	// mutations are reported. Sorting the resource keys must yield byte-identical,
+	// alphabetical output on every run so plan and apply agree (issue #341).
+	defer subprocess.SetExecRunner(fakeDokku(map[string]string{
+		"--quiet resource:limit test-app": "cpu: 50\nmemory: 256",
+	}))()
+
+	task := ResourceLimitTask{
+		App:       "test-app",
+		Resources: map[string]string{"memory": "512", "cpu": "100"},
+		State:     StatePresent,
+	}
+
+	wantCommands := []string{"dokku resource:limit --cpu 100 --memory 512 test-app"}
+	wantMutations := []string{`set cpu=100 (was "50")`, `set memory=512 (was "256")`}
+
+	// Repeat so a reintroduced map-order bug is caught reliably rather than
+	// passing by chance on a lucky iteration.
+	for i := 0; i < 20; i++ {
+		plan := task.Plan()
+		if plan.Error != nil {
+			t.Fatalf("iteration %d: unexpected plan error: %v", i, plan.Error)
+		}
+		if !reflect.DeepEqual(plan.Commands, wantCommands) {
+			t.Fatalf("iteration %d commands = %v, want %v", i, plan.Commands, wantCommands)
+		}
+		if !reflect.DeepEqual(plan.Mutations, wantMutations) {
+			t.Fatalf("iteration %d mutations = %v, want %v", i, plan.Mutations, wantMutations)
+		}
+	}
+}
+
 func TestGetTasksResourceLimitTaskParsedCorrectly(t *testing.T) {
 	data := []byte(`---
 - tasks:

tasks/resource_reserve_task_test.go

@@ -1,8 +1,10 @@
 package tasks
 
 import (
+	"reflect"
 	"testing"
 
+	"github.com/dokku/docket/subprocess"
 	_ "github.com/gliderlabs/sigil/builtin"
 )
 
@@ -34,6 +36,39 @@ func TestResourceReserveTaskNilResources(t *testing.T) {
 	}
 }
 
+func TestResourceReserveSetCommandDeterministicOrder(t *testing.T) {
+	// Both cpu and memory drift, so the set command carries both flags and both
+	// mutations are reported. Sorting the resource keys must yield byte-identical,
+	// alphabetical output on every run so plan and apply agree (issue #341).
+	defer subprocess.SetExecRunner(fakeDokku(map[string]string{
+		"--quiet resource:reserve test-app": "cpu: 50\nmemory: 256",
+	}))()
+
+	task := ResourceReserveTask{
+		App:       "test-app",
+		Resources: map[string]string{"memory": "512", "cpu": "100"},
+		State:     StatePresent,
+	}
+
+	wantCommands := []string{"dokku resource:reserve --cpu 100 --memory 512 test-app"}
+	wantMutations := []string{`set cpu=100 (was "50")`, `set memory=512 (was "256")`}
+
+	// Repeat so a reintroduced map-order bug is caught reliably rather than
+	// passing by chance on a lucky iteration.
+	for i := 0; i < 20; i++ {
+		plan := task.Plan()
+		if plan.Error != nil {
+			t.Fatalf("iteration %d: unexpected plan error: %v", i, plan.Error)
+		}
+		if !reflect.DeepEqual(plan.Commands, wantCommands) {
+			t.Fatalf("iteration %d commands = %v, want %v", i, plan.Commands, wantCommands)
+		}
+		if !reflect.DeepEqual(plan.Mutations, wantMutations) {
+			t.Fatalf("iteration %d mutations = %v, want %v", i, plan.Mutations, wantMutations)
+		}
+	}
+}
+
 func TestGetTasksResourceReserveTaskParsedCorrectly(t *testing.T) {
 	data := []byte(`---
 - tasks:

tasks/resources.go

@@ -159,11 +159,11 @@ func planSetResource(subcommand string, rctx ResourceContext) PlanResult {
 		return PlanResult{Status: PlanStatusError, Error: err}
 	}
 
-	for k := range rctx.Resources {
+	for _, k := range sortedResourceKeys(rctx.Resources) {
 		if _, ok := currentResources[k]; !ok {
 			return PlanResult{
 				Status: PlanStatusError,
-				Error:  fmt.Errorf("unknown resource %s, valid resources: %v", k, mapKeys(currentResources)),
+				Error:  fmt.Errorf("unknown resource %s, valid resources: %v", k, sortedResourceKeys(currentResources)),
 			}
 		}
 	}
@@ -180,7 +180,8 @@ func planSetResource(subcommand string, rctx ResourceContext) PlanResult {
 	if effective.ClearBefore {
 		mutations = append(mutations, "clear before set")
 	}
-	for k, v := range rctx.Resources {
+	for _, k := range sortedResourceKeys(rctx.Resources) {
+		v := rctx.Resources[k]
 		if currentResources[k] != v {
 			mutations = append(mutations, fmt.Sprintf("set %s=%s (was %q)", k, v, currentResources[k]))
 		}
@@ -269,8 +270,8 @@ func resourceSetInputs(subcommand string, rctx ResourceContext) []subprocess.Exe
 		inputs = append(inputs, resourceClearInput(subcommand, rctx))
 	}
 	args := []string{subcommand}
-	for key, value := range rctx.Resources {
-		args = append(args, fmt.Sprintf("--%s", key), value)
+	for _, key := range sortedResourceKeys(rctx.Resources) {
+		args = append(args, fmt.Sprintf("--%s", key), rctx.Resources[key])
 	}
 	if rctx.ProcessType != "" {
 		args = append(args, "--process-type", rctx.ProcessType)
@@ -306,3 +307,11 @@ func mapKeys(m map[string]string) []string {
 	}
 	return keys
 }
+
+// sortedResourceKeys returns the resource keys in deterministic (sorted) order so
+// plan and apply build byte-identical command args and mutation lists (issue #341).
+func sortedResourceKeys(m map[string]string) []string {
+	keys := mapKeys(m)
+	sort.Strings(keys)
+	return keys
+}