From cfa2450e7a5a299125031e78d66aae97c16f2df2 Mon Sep 17 00:00:00 2001 From: Jose Diaz-Gonzalez Date: Sat, 18 Jul 2026 00:23:15 -0400 Subject: [PATCH] fix: make dokku_storage_ensure chown genuinely optional `dokku_storage_ensure` documented `chown` as optional, but validation rejected an omitted value with `invalid chown value specified`, making the field effectively required. An omitted `chown` now passes validation and drops `--chown` from the dispatched command so dokku applies its default ownership, while a non-empty value that is not a known preset is still rejected with a clearer message. --- docs/tasks/dokku_storage_ensure.md | 2 +- tasks/storage_ensure_task.go | 27 +++++++++++--- tasks/storage_ensure_task_integration_test.go | 23 ++++++++++++ tasks/storage_ensure_task_test.go | 36 +++++++++++++++---- tasks/validate_test.go | 34 ++++++++++++++++++ 5 files changed, 109 insertions(+), 13 deletions(-) diff --git a/docs/tasks/dokku_storage_ensure.md b/docs/tasks/dokku_storage_ensure.md index c2fd13c..ecfe6d4 100644 --- a/docs/tasks/dokku_storage_ensure.md +++ b/docs/tasks/dokku_storage_ensure.md @@ -15,7 +15,7 @@ Not supported - deprecated; storage state is exported via dokku_storage_mount. | Parameter | Type | Required | Default | Choices | Description | | --- | --- | --- | --- | --- | --- | | `app` | string | yes | | | Name of the app | -| `chown` | string | no | | | Chown value to set | +| `chown` | string | no | | heroku, herokuish, paketo, root, false | Chown value to set | | `state` | string | no | present | present, absent | Desired state of the storage | ## Examples diff --git a/tasks/storage_ensure_task.go b/tasks/storage_ensure_task.go index b2c62f5..c891d7c 100644 --- a/tasks/storage_ensure_task.go +++ b/tasks/storage_ensure_task.go @@ -2,6 +2,8 @@ package tasks import ( "errors" + "strings" + "github.com/dokku/docket/subprocess" ) @@ -11,7 +13,7 @@ type StorageEnsureTask struct { App string `required:"true" yaml:"app" description:"Name of the app"` // Chown is the chown value to set - Chown string `required:"false" yaml:"chown,omitempty" description:"Chown value to set"` + Chown string `required:"false" yaml:"chown,omitempty" options:"heroku,herokuish,paketo,root,false" description:"Chown value to set"` // State is the desired state of the storage State State `required:"false" yaml:"state,omitempty" default:"present" options:"present,absent" description:"Desired state of the storage"` @@ -75,13 +77,16 @@ func (t StorageEnsureTask) Execute() TaskOutputState { } // Validate checks the StorageEnsureTask's inputs without contacting the server. +// chown is optional: an omitted value lets dokku apply its default (herokuish) +// ownership. A non-empty value must name one of the ownership presets dokku's +// storage:ensure-directory understands so a typo is caught before dispatch. func (t StorageEnsureTask) Validate() error { - if t.State == StatePresent { + if t.State == StatePresent && t.Chown != "" { chownValues := map[string]bool{ "heroku": true, "herokuish": true, "paketo": true, "root": true, "false": true, } if !chownValues[t.Chown] { - return errors.New("invalid chown value specified") + return errors.New("'chown' must be one of heroku, herokuish, paketo, root, false") } } if t.State == StateAbsent { @@ -90,6 +95,17 @@ func (t StorageEnsureTask) Validate() error { return nil } +// ensureArgs builds the storage:ensure-directory command arguments. The +// --chown flag is omitted when no chown value is set so the field stays +// genuinely optional and dokku applies its default ownership. +func (t StorageEnsureTask) ensureArgs() []string { + args := []string{"--quiet", "storage:ensure-directory"} + if t.Chown != "" { + args = append(args, "--chown", t.Chown) + } + return append(args, t.App) +} + // Plan reports the drift the StorageEnsureTask would produce. dokku does // not expose a probe for storage:ensure-directory, so the plan reports // drift unconditionally. @@ -99,15 +115,16 @@ func (t StorageEnsureTask) Plan() PlanResult { } return DispatchPlan(t.State, map[State]func() PlanResult{ StatePresent: func() PlanResult { + args := t.ensureArgs() inputs := []subprocess.ExecCommandInput{{ Command: "dokku", - Args: []string{"--quiet", "storage:ensure-directory", "--chown", t.Chown, t.App}, + Args: args, }} return PlanResult{ InSync: false, Status: PlanStatusModify, Reason: "directory presence not probed", - Mutations: []string{"storage:ensure-directory --chown " + t.Chown + " " + t.App}, + Mutations: []string{strings.Join(args[1:], " ")}, Commands: resolveCommands(inputs), apply: func() TaskOutputState { return runExecInputs(TaskOutputState{State: StateAbsent}, StatePresent, inputs) diff --git a/tasks/storage_ensure_task_integration_test.go b/tasks/storage_ensure_task_integration_test.go index ede29ff..7c5dc1a 100644 --- a/tasks/storage_ensure_task_integration_test.go +++ b/tasks/storage_ensure_task_integration_test.go @@ -26,3 +26,26 @@ func TestIntegrationStorageEnsure(t *testing.T) { t.Errorf("expected state 'present', got '%s'", result.State) } } + +func TestIntegrationStorageEnsureOmittedChown(t *testing.T) { + skipIfNoDokkuT(t) + + appName := "docket-test-storage-nochown" + + destroyApp(appName) + createApp(appName) + defer destroyApp(appName) + + // chown omitted: dokku applies its default (herokuish) ownership. + task := StorageEnsureTask{ + App: appName, + State: StatePresent, + } + result := task.Execute() + if result.Error != nil { + t.Fatalf("failed to ensure storage without chown: %v", result.Error) + } + if result.State != StatePresent { + t.Errorf("expected state 'present', got '%s'", result.State) + } +} diff --git a/tasks/storage_ensure_task_test.go b/tasks/storage_ensure_task_test.go index 4c75c3b..a9dad82 100644 --- a/tasks/storage_ensure_task_test.go +++ b/tasks/storage_ensure_task_test.go @@ -19,7 +19,7 @@ func TestStorageEnsureValidChownValues(t *testing.T) { result := task.Execute() // These will fail because dokku isn't running, but should NOT fail // due to invalid chown value - if result.Error != nil && result.Error.Error() == "invalid chown value specified" { + if result.Error != nil && result.Error.Error() == "'chown' must be one of heroku, herokuish, paketo, root, false" { t.Errorf("chown value %q should be valid but was rejected", chown) } } @@ -28,7 +28,7 @@ func TestStorageEnsureValidChownValues(t *testing.T) { func TestStorageEnsureInvalidChownValue(t *testing.T) { task := StorageEnsureTask{App: "test-app", Chown: "packeto", State: StatePresent} result := task.Execute() - if result.Error == nil || result.Error.Error() != "invalid chown value specified" { + if result.Error == nil || result.Error.Error() != "'chown' must be one of heroku, herokuish, paketo, root, false" { t.Errorf("chown value 'packeto' (misspelled) should be rejected as invalid") } } @@ -41,10 +41,32 @@ func TestStorageEnsureAbsentStateReturnsError(t *testing.T) { } } -func TestStorageEnsureEmptyChown(t *testing.T) { - task := StorageEnsureTask{App: "test-app", Chown: "", State: StatePresent} - result := task.Execute() - if result.Error == nil || result.Error.Error() != "invalid chown value specified" { - t.Errorf("expected 'invalid chown value specified' error, got: %v", result.Error) +func TestStorageEnsureOmittedChownAllowed(t *testing.T) { + task := StorageEnsureTask{App: "test-app", State: StatePresent} + if err := task.Validate(); err != nil { + t.Errorf("an omitted chown should pass validation, got: %v", err) + } +} + +func TestStorageEnsureChownCommandShape(t *testing.T) { + task := StorageEnsureTask{App: "node-js-app", Chown: "herokuish", State: StatePresent} + args := task.ensureArgs() + want := []string{"--quiet", "storage:ensure-directory", "--chown", "herokuish", "node-js-app"} + if !equalStrings(args, want) { + t.Errorf("ensureArgs mismatch:\n got: %v\n want: %v", args, want) + } +} + +func TestStorageEnsureOmittedChownCommandShape(t *testing.T) { + task := StorageEnsureTask{App: "node-js-app", State: StatePresent} + args := task.ensureArgs() + want := []string{"--quiet", "storage:ensure-directory", "node-js-app"} + if !equalStrings(args, want) { + t.Errorf("ensureArgs mismatch:\n got: %v\n want: %v", args, want) + } + for _, a := range args { + if a == "--chown" { + t.Errorf("omitted chown must not emit --chown flag: %v", args) + } } } diff --git a/tasks/validate_test.go b/tasks/validate_test.go index 0cd7d0e..4abe5d0 100644 --- a/tasks/validate_test.go +++ b/tasks/validate_test.go @@ -564,6 +564,40 @@ func TestValidateInvalidTaskInputSatisfied(t *testing.T) { } } +func TestValidateStorageEnsureOmittedChownValid(t *testing.T) { + // chown is optional on dokku_storage_ensure; omitting it lets dokku apply + // its default ownership, so the recipe validates without an + // invalid_task_input problem. + data := []byte(`--- +- tasks: + - dokku_storage_ensure: + app: node-js-app +`) + problems := Validate(data, ValidateOptions{}) + if n := countProblems(problems, "invalid_task_input"); n != 0 { + t.Errorf("expected no invalid_task_input for an omitted chown, got %d: %+v", n, problems) + } +} + +func TestValidateStorageEnsureInvalidChown(t *testing.T) { + // A non-empty chown that is not one of the known ownership presets is + // still rejected so a typo surfaces at validate time. + data := []byte(`--- +- tasks: + - dokku_storage_ensure: + app: node-js-app + chown: packeto +`) + problems := Validate(data, ValidateOptions{}) + p := findProblem(problems, "invalid_task_input") + if p == nil { + t.Fatalf("expected invalid_task_input problem, got: %+v", problems) + } + if !strings.Contains(p.Message, "'chown' must be one of") { + t.Errorf("expected message to mention the allowed chown values, got: %q", p.Message) + } +} + func TestValidateInvalidTaskInputSkippedWhenRequiredMissing(t *testing.T) { // When a required field is missing, only missing_required_field is // reported; the conditional Validate() check (which depends on that