diff --git a/tasks/ps_scale_task.go b/tasks/ps_scale_task.go index 5101aaa..31729cd 100644 --- a/tasks/ps_scale_task.go +++ b/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 } diff --git a/tasks/ps_scale_task_test.go b/tasks/ps_scale_task_test.go index 372181b..35aac28 100644 --- a/tasks/ps_scale_task_test.go +++ b/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: diff --git a/tasks/resource_limit_task_test.go b/tasks/resource_limit_task_test.go index 621392a..99b9921 100644 --- a/tasks/resource_limit_task_test.go +++ b/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: diff --git a/tasks/resource_reserve_task_test.go b/tasks/resource_reserve_task_test.go index 0d03a3f..5cc4674 100644 --- a/tasks/resource_reserve_task_test.go +++ b/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: diff --git a/tasks/resources.go b/tasks/resources.go index 0fb03fe..9735fd0 100644 --- a/tasks/resources.go +++ b/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 +}