From a09ea8050bb786d99cb325b69ea16f8fc6f1fdc5 Mon Sep 17 00:00:00 2001 From: Jose Diaz-Gonzalez Date: Thu, 16 Jul 2026 03:45:39 -0400 Subject: [PATCH] fix: honor explicit restart: false in dokku_config An explicit `restart: false` in a `dokku_config` recipe was silently overridden back to `true`, so `dokku config:set` and `config:unset` restarted the app despite the opt-out and the documented no-restart example restarted the app anyway. The explicit value is now honored on both the set and unset paths, and the other optional boolean task flags that carry a default are made consistent so a future default value cannot regress the same way. --- docs/tasks/dokku_http_auth_user.md | 3 - docs/tasks/dokku_ps_scale.md | 1 - docs/tasks/dokku_resource_limit.md | 3 - docs/tasks/dokku_resource_reserve.md | 3 - generate/docs.go | 6 +- tasks/bool_ptr.go | 17 ++++ tasks/config_task.go | 18 ++-- tasks/config_task_integration_test.go | 10 +- tasks/config_task_test.go | 99 ++++++++++++++++++- tasks/export_integration_test.go | 2 +- tasks/http_auth_user_task.go | 9 +- tasks/http_auth_user_task_integration_test.go | 2 +- tasks/http_auth_user_task_test.go | 3 +- tasks/plan_integration_test.go | 2 +- tasks/ps_scale_task.go | 9 +- tasks/ps_scale_task_integration_test.go | 2 +- tasks/ps_scale_task_test.go | 3 +- tasks/resource_limit_task.go | 7 +- tasks/resource_limit_task_integration_test.go | 2 +- tasks/resource_limit_task_test.go | 13 ++- tasks/resource_reserve_task.go | 7 +- .../resource_reserve_task_integration_test.go | 2 +- tasks/resource_reserve_task_test.go | 4 +- tests/bats/json.bats | 55 +++++++++++ 24 files changed, 225 insertions(+), 57 deletions(-) create mode 100644 tasks/bool_ptr.go diff --git a/docs/tasks/dokku_http_auth_user.md b/docs/tasks/dokku_http_auth_user.md index 71a5f4e..0bd6673 100644 --- a/docs/tasks/dokku_http_auth_user.md +++ b/docs/tasks/dokku_http_auth_user.md @@ -33,7 +33,6 @@ dokku_http_auth_user: password: secret - username: ops password: hunter2 - update_password: false ``` ### Rotate an existing user's password @@ -54,7 +53,6 @@ dokku_http_auth_user: app: hello-world users: - username: ops - update_password: false state: absent ``` @@ -64,7 +62,6 @@ dokku_http_auth_user: dokku_http_auth_user: app: hello-world users: [] - update_password: false state: absent ``` diff --git a/docs/tasks/dokku_ps_scale.md b/docs/tasks/dokku_ps_scale.md index 4df4c35..fb799c8 100644 --- a/docs/tasks/dokku_ps_scale.md +++ b/docs/tasks/dokku_ps_scale.md @@ -27,7 +27,6 @@ dokku_ps_scale: scale: web: 2 worker: 1 - skip_deploy: false ``` ### Scale web and worker processes without deploy diff --git a/docs/tasks/dokku_resource_limit.md b/docs/tasks/dokku_resource_limit.md index 886be3c..59ee1cb 100644 --- a/docs/tasks/dokku_resource_limit.md +++ b/docs/tasks/dokku_resource_limit.md @@ -28,7 +28,6 @@ dokku_resource_limit: resources: cpu: "100" memory: "256" - clear_before: false ``` ### Set memory limit for web process type @@ -39,7 +38,6 @@ dokku_resource_limit: process_type: web resources: memory: "512" - clear_before: false ``` ### Clear all resource limits @@ -48,7 +46,6 @@ dokku_resource_limit: dokku_resource_limit: app: hello-world resources: {} - clear_before: false state: absent ``` diff --git a/docs/tasks/dokku_resource_reserve.md b/docs/tasks/dokku_resource_reserve.md index c1efd9c..e1c8ba7 100644 --- a/docs/tasks/dokku_resource_reserve.md +++ b/docs/tasks/dokku_resource_reserve.md @@ -28,7 +28,6 @@ dokku_resource_reserve: resources: cpu: "100" memory: "256" - clear_before: false ``` ### Set memory reservation for web process type @@ -39,7 +38,6 @@ dokku_resource_reserve: process_type: web resources: memory: "512" - clear_before: false ``` ### Clear all resource reservations @@ -48,7 +46,6 @@ dokku_resource_reserve: dokku_resource_reserve: app: hello-world resources: {} - clear_before: false state: absent ``` diff --git a/generate/docs.go b/generate/docs.go index e6b0256..5351d3e 100644 --- a/generate/docs.go +++ b/generate/docs.go @@ -38,8 +38,12 @@ type param struct { // displayType maps a Go field type to the doc-facing type name. Named string // types such as State render as "string"; slices render as "list" and maps as -// "dict", mirroring Ansible's type vocabulary. +// "dict", mirroring Ansible's type vocabulary. Pointer fields (e.g. an optional +// *bool) render as their element type so they read as "bool" rather than "*bool". func displayType(t reflect.Type) string { + if t.Kind() == reflect.Ptr { + return displayType(t.Elem()) + } switch t.Kind() { case reflect.String: return "string" diff --git a/tasks/bool_ptr.go b/tasks/bool_ptr.go new file mode 100644 index 0000000..885534a --- /dev/null +++ b/tasks/bool_ptr.go @@ -0,0 +1,17 @@ +package tasks + +// boolPtr returns a pointer to b. It is used when building task structs whose +// optional bool fields are declared as *bool so an explicit false is +// distinguishable from an omitted key. +func boolPtr(b bool) *bool { return &b } + +// boolValue dereferences p, returning def when p is nil. Optional bool task +// fields are declared as *bool so `restart: false` (and friends) survive +// decoding: go-defaults leaves pointer fields untouched, so the `default:` tag +// only drives the generated docs table and the real default is applied here. +func boolValue(p *bool, def bool) bool { + if p == nil { + return def + } + return *p +} diff --git a/tasks/config_task.go b/tasks/config_task.go index 5f77378..f784684 100644 --- a/tasks/config_task.go +++ b/tasks/config_task.go @@ -14,8 +14,12 @@ type ConfigTask struct { // App is the name of the app App string `required:"true" yaml:"app" description:"Name of the app"` - // Restart is a flag indicating if the app should be restarted - Restart bool `yaml:"restart" default:"true" description:"Flag indicating if the app should be restarted"` + // Restart is a flag indicating if the app should be restarted. It is a + // *bool so an explicit `restart: false` is distinguishable from an omitted + // key: nil defaults to true via boolValue(t.Restart, true). The default:"true" + // tag only drives the generated docs table; go-defaults leaves pointer fields + // untouched, which is what fixes the silent override of an explicit false. + Restart *bool `yaml:"restart,omitempty" default:"true" description:"Flag indicating if the app should be restarted"` // Config is a map of configuration key-value pairs Config map[string]string `yaml:"config" description:"Map of configuration key-value pairs"` @@ -55,7 +59,7 @@ func (t ConfigTask) Examples() ([]Doc, error) { Name: "set KEY=VALUE", ConfigTask: ConfigTask{ App: "hello-world", - Restart: true, + Restart: boolPtr(true), Config: map[string]string{ "KEY": "VALUE_1", }, @@ -65,7 +69,7 @@ func (t ConfigTask) Examples() ([]Doc, error) { Name: "set KEY=VALUE without restart", ConfigTask: ConfigTask{ App: "hello-world", - Restart: false, + Restart: boolPtr(false), Config: map[string]string{ "KEY": "VALUE_1", }, @@ -156,7 +160,7 @@ func planConfigSet(t ConfigTask) PlanResult { status = PlanStatusCreate } args := []string{"--quiet", "config:set", "--encoded"} - if !t.Restart { + if !boolValue(t.Restart, true) { args = append(args, "--no-restart") } args = append(args, t.App) @@ -192,7 +196,7 @@ func planConfigUnset(t ConfigTask) PlanResult { mutations = append(mutations, fmt.Sprintf("unset %s", k)) } args := []string{"--quiet", "config:unset"} - if !t.Restart { + if !boolValue(t.Restart, true) { args = append(args, "--no-restart") } args = append(args, t.App) @@ -242,7 +246,7 @@ func (t ConfigTask) ExportApp(app string) ([]interface{}, error) { if len(config) == 0 { return nil, nil } - return []interface{}{ConfigTask{App: app, Restart: true, Config: config}}, nil + return []interface{}{ConfigTask{App: app, Restart: boolPtr(true), Config: config}}, nil } // getConfig retrieves the current configuration for a given dokku application diff --git a/tasks/config_task_integration_test.go b/tasks/config_task_integration_test.go index 1f1d463..2af5fa5 100644 --- a/tasks/config_task_integration_test.go +++ b/tasks/config_task_integration_test.go @@ -17,7 +17,7 @@ func TestIntegrationConfigSetAndUnset(t *testing.T) { // set config setTask := ConfigTask{ App: appName, - Restart: false, + Restart: boolPtr(false), Config: map[string]string{"TEST_KEY": "test_value"}, State: StatePresent, } @@ -44,7 +44,7 @@ func TestIntegrationConfigSetAndUnset(t *testing.T) { // unset config unsetTask := ConfigTask{ App: appName, - Restart: false, + Restart: boolPtr(false), Config: map[string]string{"TEST_KEY": ""}, State: StateAbsent, } @@ -81,7 +81,7 @@ func TestIntegrationConfigMultipleKeys(t *testing.T) { // set 3 keys setTask := ConfigTask{ App: appName, - Restart: false, + Restart: boolPtr(false), Config: map[string]string{"KEY_A": "val_a", "KEY_B": "val_b", "KEY_C": "val_c"}, State: StatePresent, } @@ -96,7 +96,7 @@ func TestIntegrationConfigMultipleKeys(t *testing.T) { // update one key, keep others the same updateTask := ConfigTask{ App: appName, - Restart: false, + Restart: boolPtr(false), Config: map[string]string{"KEY_A": "val_a", "KEY_B": "val_b_updated", "KEY_C": "val_c"}, State: StatePresent, } @@ -120,7 +120,7 @@ func TestIntegrationConfigMultipleKeys(t *testing.T) { // unset all keys unsetTask := ConfigTask{ App: appName, - Restart: false, + Restart: boolPtr(false), Config: map[string]string{"KEY_A": "", "KEY_B": "", "KEY_C": ""}, State: StateAbsent, } diff --git a/tasks/config_task_test.go b/tasks/config_task_test.go index 3df592f..9f45bbc 100644 --- a/tasks/config_task_test.go +++ b/tasks/config_task_test.go @@ -1,8 +1,10 @@ package tasks import ( + "strings" "testing" + "github.com/dokku/docket/subprocess" _ "github.com/gliderlabs/sigil/builtin" ) @@ -54,10 +56,14 @@ func TestGetTasksConfigTaskParsedCorrectly(t *testing.T) { if configTask.App != "test-app" { t.Errorf("App = %q, want %q", configTask.App, "test-app") } - // Note: defaults.SetDefaults overrides restart=false with the default tag value "true" - // because false is the zero value for bool. This documents the actual behavior. - if !configTask.Restart { - t.Error("Restart = false, want true (defaults.SetDefaults overrides zero-value bool)") + // Restart is a *bool, so an explicit restart: false survives decoding: it is + // no longer clobbered back to true by defaults.SetDefaults (go-defaults + // leaves pointer fields untouched). + if configTask.Restart == nil { + t.Fatal("Restart is nil, want an explicit false") + } + if *configTask.Restart { + t.Error("Restart = true, want false (explicit restart: false must be preserved)") } if len(configTask.Config) != 2 { t.Fatalf("expected 2 config keys, got %d", len(configTask.Config)) @@ -69,3 +75,88 @@ func TestGetTasksConfigTaskParsedCorrectly(t *testing.T) { t.Errorf("Config[KEY2] = %q, want %q", configTask.Config["KEY2"], "val2") } } + +// TestConfigTaskRestartFlag pins the --no-restart flag to the Restart pointer +// across both config:set and config:unset. An explicit restart: false must emit +// --no-restart; an omitted restart (nil) defaults to true and must not. +func TestConfigTaskRestartFlag(t *testing.T) { + cases := []struct { + name string + restart *bool + state State + current string // stdout of the config:export probe + wantVerb string + wantNoRestrt bool + }{ + {"set explicit false emits --no-restart", boolPtr(false), StatePresent, "{}", "config:set", true}, + {"set omitted defaults to restart", nil, StatePresent, "{}", "config:set", false}, + {"set explicit true keeps restart", boolPtr(true), StatePresent, "{}", "config:set", false}, + {"unset explicit false emits --no-restart", boolPtr(false), StateAbsent, `{"KEY":"val"}`, "config:unset", true}, + {"unset omitted defaults to restart", nil, StateAbsent, `{"KEY":"val"}`, "config:unset", false}, + {"unset explicit true keeps restart", boolPtr(true), StateAbsent, `{"KEY":"val"}`, "config:unset", false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + defer subprocess.SetExecRunner(fakeDokku(map[string]string{ + "--quiet config:export --format json test-app": tc.current, + }))() + + plan := ConfigTask{ + App: "test-app", + Restart: tc.restart, + Config: map[string]string{"KEY": "val"}, + State: tc.state, + }.Plan() + if plan.Error != nil { + t.Fatalf("unexpected plan error: %v", plan.Error) + } + if len(plan.Commands) == 0 { + t.Fatalf("expected a command, got none (plan=%#v)", plan) + } + cmd := plan.Commands[0] + if !strings.Contains(cmd, tc.wantVerb) { + t.Errorf("command %q does not contain %q", cmd, tc.wantVerb) + } + if got := strings.Contains(cmd, "--no-restart"); got != tc.wantNoRestrt { + t.Errorf("command %q: --no-restart present = %v, want %v", cmd, got, tc.wantNoRestrt) + } + }) + } +} + +// TestConfigTaskRestartFalseEndToEnd exercises the full parse + SetDefaults +// pipeline: a recipe with restart: false must still yield a --no-restart +// command. This is the regression the go-defaults clobber produced. +func TestConfigTaskRestartFalseEndToEnd(t *testing.T) { + data := []byte(`--- +- tasks: + - name: set config + dokku_config: + app: test-app + restart: false + config: + KEY: val +`) + defer subprocess.SetExecRunner(fakeDokku(map[string]string{ + "--quiet config:export --format json test-app": "{}", + }))() + + parsed, err := GetTasks(data, map[string]interface{}{}) + if err != nil { + t.Fatalf("GetTasks failed: %v", err) + } + task := parsed.Get("set config") + if task == nil { + t.Fatal("task 'set config' not found") + } + plan := task.Plan() + if plan.Error != nil { + t.Fatalf("unexpected plan error: %v", plan.Error) + } + if len(plan.Commands) == 0 { + t.Fatalf("expected a command, got none") + } + if !strings.Contains(plan.Commands[0], "--no-restart") { + t.Errorf("restart: false should emit --no-restart, got %q", plan.Commands[0]) + } +} diff --git a/tasks/export_integration_test.go b/tasks/export_integration_test.go index db2f713..ec86d5c 100644 --- a/tasks/export_integration_test.go +++ b/tasks/export_integration_test.go @@ -16,7 +16,7 @@ func TestIntegrationExportConfigRoundTrip(t *testing.T) { createApp(app) defer destroyApp(app) - set := ConfigTask{App: app, Restart: false, Config: map[string]string{"EXPORT_KEY": "export_value"}, State: StatePresent} + set := ConfigTask{App: app, Restart: boolPtr(false), Config: map[string]string{"EXPORT_KEY": "export_value"}, State: StatePresent} if r := set.Execute(); r.Error != nil { t.Fatalf("set config: %v", r.Error) } diff --git a/tasks/http_auth_user_task.go b/tasks/http_auth_user_task.go index 8260423..01e709c 100644 --- a/tasks/http_auth_user_task.go +++ b/tasks/http_auth_user_task.go @@ -20,8 +20,9 @@ type HttpAuthUserTask struct { // UpdatePassword re-issues http-auth:add-user for users that already exist // so their password converges. Passwords are not exposed in the report, so - // a rotation cannot be drift-detected; enable this to force convergence. - UpdatePassword bool `required:"false" yaml:"update_password" default:"false" description:"Re-issue add-user for users that already exist so their password converges"` + // a rotation cannot be drift-detected; enable this to force convergence. It + // is a *bool so the value survives decoding unchanged; nil defaults to false. + UpdatePassword *bool `required:"false" yaml:"update_password,omitempty" default:"false" description:"Re-issue add-user for users that already exist so their password converges"` // State is the desired state of the HTTP auth users State State `required:"false" yaml:"state,omitempty" default:"present" options:"present,absent" description:"Desired state of the HTTP auth users"` @@ -99,7 +100,7 @@ func (t HttpAuthUserTask) Examples() ([]Doc, error) { DokkuHttpAuthUser: HttpAuthUserTask{ App: "hello-world", Users: []HttpAuthUser{{Username: "admin", Password: "new-secret"}}, - UpdatePassword: true, + UpdatePassword: boolPtr(true), }, }, { @@ -166,7 +167,7 @@ func (t HttpAuthUserTask) Plan() PlanResult { case !current[u.Username]: toApply = append(toApply, u) mutations = append(mutations, "add "+u.Username) - case t.UpdatePassword: + case boolValue(t.UpdatePassword, false): toApply = append(toApply, u) mutations = append(mutations, "update "+u.Username) } diff --git a/tasks/http_auth_user_task_integration_test.go b/tasks/http_auth_user_task_integration_test.go index b784554..6df0624 100644 --- a/tasks/http_auth_user_task_integration_test.go +++ b/tasks/http_auth_user_task_integration_test.go @@ -75,7 +75,7 @@ func TestIntegrationHttpAuthUser(t *testing.T) { } // update_password re-issues add-user for an existing user - rotate := HttpAuthUserTask{App: appName, Users: []HttpAuthUser{{Username: "alice", Password: "new-pass"}}, UpdatePassword: true, State: StatePresent} + rotate := HttpAuthUserTask{App: appName, Users: []HttpAuthUser{{Username: "alice", Password: "new-pass"}}, UpdatePassword: boolPtr(true), State: StatePresent} result = rotate.Execute() if result.Error != nil { t.Fatalf("failed to rotate password: %v", result.Error) diff --git a/tasks/http_auth_user_task_test.go b/tasks/http_auth_user_task_test.go index 983d150..cfcbb56 100644 --- a/tasks/http_auth_user_task_test.go +++ b/tasks/http_auth_user_task_test.go @@ -126,7 +126,8 @@ func TestGetTasksHttpAuthUserTaskParsedCorrectly(t *testing.T) { if hauTask.App != "test-app" { t.Errorf("App = %q, want %q", hauTask.App, "test-app") } - if !hauTask.UpdatePassword { + // UpdatePassword is a *bool, so an explicit update_password: true survives decoding. + if hauTask.UpdatePassword == nil || !*hauTask.UpdatePassword { t.Error("UpdatePassword = false, want true") } if len(hauTask.Users) != 2 { diff --git a/tasks/plan_integration_test.go b/tasks/plan_integration_test.go index 3b56ad8..d0f2957 100644 --- a/tasks/plan_integration_test.go +++ b/tasks/plan_integration_test.go @@ -85,7 +85,7 @@ func TestIntegrationPlanConfigItemizes(t *testing.T) { task := ConfigTask{ App: appName, - Restart: false, + Restart: boolPtr(false), Config: map[string]string{"DOCKET_TEST_KEY_A": "1", "DOCKET_TEST_KEY_B": "2"}, State: StatePresent, } diff --git a/tasks/ps_scale_task.go b/tasks/ps_scale_task.go index 001ab5f..5101aaa 100644 --- a/tasks/ps_scale_task.go +++ b/tasks/ps_scale_task.go @@ -15,8 +15,9 @@ type PsScaleTask struct { // Scale is a map of process types to quantities Scale map[string]int `required:"true" yaml:"scale" description:"Map of process types to quantities"` - // SkipDeploy skips the corresponding deploy - SkipDeploy bool `yaml:"skip_deploy" default:"false" description:"Skip the corresponding deploy"` + // SkipDeploy skips the corresponding deploy. It is a *bool so the value + // survives decoding unchanged; nil defaults to false. + SkipDeploy *bool `yaml:"skip_deploy,omitempty" default:"false" description:"Skip the corresponding deploy"` // State is the desired state of the process scale State State `required:"false" yaml:"state,omitempty" default:"present" options:"present" description:"Desired state of the process scale"` @@ -63,7 +64,7 @@ func (t PsScaleTask) Examples() ([]Doc, error) { Name: "Scale web and worker processes without deploy", PsScaleTask: PsScaleTask{ App: "hello-world", - SkipDeploy: true, + SkipDeploy: boolPtr(true), Scale: map[string]int{ "web": 4, "worker": 4, @@ -114,7 +115,7 @@ func (t PsScaleTask) Plan() PlanResult { return PlanResult{InSync: true, Status: PlanStatusOK} } args := []string{"ps:scale"} - if t.SkipDeploy { + if boolValue(t.SkipDeploy, false) { args = append(args, "--skip-deploy") } args = append(args, t.App) diff --git a/tasks/ps_scale_task_integration_test.go b/tasks/ps_scale_task_integration_test.go index 9bdf763..a36ef59 100644 --- a/tasks/ps_scale_task_integration_test.go +++ b/tasks/ps_scale_task_integration_test.go @@ -146,7 +146,7 @@ func TestIntegrationPsScaleSkipDeploy(t *testing.T) { scaleTask := PsScaleTask{ App: appName, Scale: map[string]int{"web": 2, "worker": 1}, - SkipDeploy: true, + SkipDeploy: boolPtr(true), State: StatePresent, } result := scaleTask.Execute() diff --git a/tasks/ps_scale_task_test.go b/tasks/ps_scale_task_test.go index 50b1883..372181b 100644 --- a/tasks/ps_scale_task_test.go +++ b/tasks/ps_scale_task_test.go @@ -74,7 +74,8 @@ func TestGetTasksPsScaleTaskParsedCorrectly(t *testing.T) { if psTask.Scale["worker"] != 1 { t.Errorf("Scale[worker] = %d, want %d", psTask.Scale["worker"], 1) } - if !psTask.SkipDeploy { + // SkipDeploy is a *bool, so an explicit skip_deploy: true survives decoding. + if psTask.SkipDeploy == nil || !*psTask.SkipDeploy { t.Error("SkipDeploy = false, want true (YAML value should be preserved)") } } diff --git a/tasks/resource_limit_task.go b/tasks/resource_limit_task.go index 9713260..744055e 100644 --- a/tasks/resource_limit_task.go +++ b/tasks/resource_limit_task.go @@ -11,8 +11,9 @@ type ResourceLimitTask struct { // Resources is a map of resource type to quantity Resources map[string]string `yaml:"resources" description:"Map of resource type to quantity"` - // ClearBefore clears all resource limits before applying new ones - ClearBefore bool `yaml:"clear_before" default:"false" description:"ClearBefore clears all resource limits before applying new ones"` + // ClearBefore clears all resource limits before applying new ones. It is a + // *bool so the value survives decoding unchanged; nil defaults to false. + ClearBefore *bool `yaml:"clear_before,omitempty" default:"false" description:"ClearBefore clears all resource limits before applying new ones"` // State is the desired state of the resource limits State State `required:"false" yaml:"state,omitempty" default:"present" options:"present,absent" description:"Desired state of the resource limits"` @@ -94,7 +95,7 @@ func (t ResourceLimitTask) Validate() error { // Plan reports the drift the ResourceLimitTask would produce. func (t ResourceLimitTask) Plan() PlanResult { - return planResource(t.State, t.App, t.ProcessType, t.Resources, t.ClearBefore, "resource:limit") + return planResource(t.State, t.App, t.ProcessType, t.Resources, boolValue(t.ClearBefore, false), "resource:limit") } // init registers the ResourceLimitTask with the task registry diff --git a/tasks/resource_limit_task_integration_test.go b/tasks/resource_limit_task_integration_test.go index 394b541..018892e 100644 --- a/tasks/resource_limit_task_integration_test.go +++ b/tasks/resource_limit_task_integration_test.go @@ -103,7 +103,7 @@ func TestIntegrationResourceLimitProcessType(t *testing.T) { App: appName, ProcessType: "web", Resources: map[string]string{"cpu": "200"}, - ClearBefore: true, + ClearBefore: boolPtr(true), State: StatePresent, } result = clearBeforeTask.Execute() diff --git a/tasks/resource_limit_task_test.go b/tasks/resource_limit_task_test.go index 64658cc..621392a 100644 --- a/tasks/resource_limit_task_test.go +++ b/tasks/resource_limit_task_test.go @@ -44,7 +44,7 @@ func TestResourceLimitClearBeforeConvergesWhenMatched(t *testing.T) { plan := ResourceLimitTask{ App: "test-app", Resources: map[string]string{"cpu": "100"}, - ClearBefore: true, + ClearBefore: boolPtr(true), State: StatePresent, }.Plan() if plan.Error != nil { @@ -64,7 +64,7 @@ func TestResourceLimitClearBeforeIgnoresEmptyExtraResource(t *testing.T) { plan := ResourceLimitTask{ App: "test-app", Resources: map[string]string{"cpu": "100"}, - ClearBefore: true, + ClearBefore: boolPtr(true), State: StatePresent, }.Plan() if plan.Error != nil { @@ -84,7 +84,7 @@ func TestResourceLimitClearBeforeClearsNonDesiredResource(t *testing.T) { plan := ResourceLimitTask{ App: "test-app", Resources: map[string]string{"cpu": "100"}, - ClearBefore: true, + ClearBefore: boolPtr(true), State: StatePresent, }.Plan() if plan.Error != nil { @@ -161,10 +161,9 @@ func TestGetTasksResourceLimitTaskParsedCorrectly(t *testing.T) { if rlTask.Resources["memory"] != "256" { t.Errorf("Resources[memory] = %q, want %q", rlTask.Resources["memory"], "256") } - // Unlike ConfigTask.Restart (default:"true"), ClearBefore has default:"false" which - // is the zero value for bool. Since defaults.SetDefaults only overrides zero values, - // and true is non-zero, setting clear_before: true in YAML is preserved correctly. - if !rlTask.ClearBefore { + // ClearBefore is a *bool, so an explicit clear_before: true survives decoding + // (go-defaults leaves pointer fields untouched). + if rlTask.ClearBefore == nil || !*rlTask.ClearBefore { t.Error("ClearBefore = false, want true (YAML value should be preserved)") } } diff --git a/tasks/resource_reserve_task.go b/tasks/resource_reserve_task.go index 36f6a89..8d549ad 100644 --- a/tasks/resource_reserve_task.go +++ b/tasks/resource_reserve_task.go @@ -11,8 +11,9 @@ type ResourceReserveTask struct { // Resources is a map of resource type to quantity Resources map[string]string `yaml:"resources" description:"Map of resource type to quantity"` - // ClearBefore clears all resource reservations before applying new ones - ClearBefore bool `yaml:"clear_before" default:"false" description:"ClearBefore clears all resource reservations before applying new ones"` + // ClearBefore clears all resource reservations before applying new ones. It + // is a *bool so the value survives decoding unchanged; nil defaults to false. + ClearBefore *bool `yaml:"clear_before,omitempty" default:"false" description:"ClearBefore clears all resource reservations before applying new ones"` // State is the desired state of the resource reservations State State `required:"false" yaml:"state,omitempty" default:"present" options:"present,absent" description:"Desired state of the resource reservations"` @@ -94,7 +95,7 @@ func (t ResourceReserveTask) Validate() error { // Plan reports the drift the ResourceReserveTask would produce. func (t ResourceReserveTask) Plan() PlanResult { - return planResource(t.State, t.App, t.ProcessType, t.Resources, t.ClearBefore, "resource:reserve") + return planResource(t.State, t.App, t.ProcessType, t.Resources, boolValue(t.ClearBefore, false), "resource:reserve") } // init registers the ResourceReserveTask with the task registry diff --git a/tasks/resource_reserve_task_integration_test.go b/tasks/resource_reserve_task_integration_test.go index 944775f..d8fa0d9 100644 --- a/tasks/resource_reserve_task_integration_test.go +++ b/tasks/resource_reserve_task_integration_test.go @@ -103,7 +103,7 @@ func TestIntegrationResourceReserveProcessType(t *testing.T) { App: appName, ProcessType: "web", Resources: map[string]string{"cpu": "200"}, - ClearBefore: true, + ClearBefore: boolPtr(true), State: StatePresent, } result = clearBeforeTask.Execute() diff --git a/tasks/resource_reserve_task_test.go b/tasks/resource_reserve_task_test.go index a90c970..0d03a3f 100644 --- a/tasks/resource_reserve_task_test.go +++ b/tasks/resource_reserve_task_test.go @@ -82,7 +82,9 @@ func TestGetTasksResourceReserveTaskParsedCorrectly(t *testing.T) { if rrTask.Resources["memory"] != "256" { t.Errorf("Resources[memory] = %q, want %q", rrTask.Resources["memory"], "256") } - if !rrTask.ClearBefore { + // ClearBefore is a *bool, so an explicit clear_before: true survives decoding + // (go-defaults leaves pointer fields untouched). + if rrTask.ClearBefore == nil || !*rrTask.ClearBefore { t.Error("ClearBefore = false, want true (YAML value should be preserved)") } } diff --git a/tests/bats/json.bats b/tests/bats/json.bats index 5038265..ed45e52 100644 --- a/tests/bats/json.bats +++ b/tests/bats/json.bats @@ -9,6 +9,8 @@ setup() { dokku_clean_app docket-test-json-clean dokku_clean_app docket-test-json-drift dokku_clean_app docket-test-json-mut + dokku_clean_app docket-test-json-norestart + dokku_clean_app docket-test-json-restart } teardown() { @@ -16,6 +18,8 @@ teardown() { dokku_clean_app docket-test-json-clean dokku_clean_app docket-test-json-drift dokku_clean_app docket-test-json-mut + dokku_clean_app docket-test-json-norestart + dokku_clean_app docket-test-json-restart } @test "docket apply --json emits valid JSON-lines" { @@ -156,6 +160,57 @@ EOF echo "$task_event" | jq -e '.commands[0] | contains("apps:create")' >/dev/null || fail "command should mention apps:create: $task_event" } +@test "docket plan --json emits --no-restart for dokku_config restart: false" { + write_tasks_file setup.yml </dev/null || fail "config:set should include --no-restart: $task_event" +} + +@test "docket plan --json omits --no-restart when restart is not set" { + write_tasks_file setup.yml </dev/null || fail "omitted restart should not include --no-restart: $task_event" +} + @test "docket plan --detailed-exitcode returns 0 when in sync" { write_tasks_file <