From 16551c3aa284d11d01047de970bfb2a5c7108cc2 Mon Sep 17 00:00:00 2001 From: Jose Diaz-Gonzalez Date: Tue, 14 Jul 2026 18:58:26 -0400 Subject: [PATCH 1/2] fix: reconcile documentation with actual behavior The review tracked in #374 surfaced six documented behaviors the code did not implement. Where the documentation was the intended spec, the code now matches it: `apply` and `plan` fetch a recipe from an `http(s)` `--tasks` URL, and `docket init` writes a play-level name so `--play` resolves after scaffolding. The apply marker table, the play-skipped summary, the vars-file bool coercion forms, and the WSL install destination were corrected in the docs to match the code. --- commands/apply.go | 6 +- commands/init_test.go | 36 ++++++ commands/plan.go | 6 +- commands/task_file.go | 70 ++++++++++- commands/task_file_url_test.go | 160 ++++++++++++++++++++++++++ commands/templates/default.json5.tmpl | 1 + commands/templates/default.yml.tmpl | 3 +- commands/templates/minimal.json5.tmpl | 1 + commands/templates/minimal.yml.tmpl | 3 +- docs/command-reference.md | 6 +- docs/getting-started.md | 5 +- docs/inputs.md | 5 +- docs/recipes.md | 3 +- install.sh | 3 +- tests/bats/init.bats | 17 +++ tests/bats/task_file_url.bats | 84 ++++++++++++++ 16 files changed, 388 insertions(+), 21 deletions(-) create mode 100644 commands/task_file_url_test.go create mode 100644 tests/bats/task_file_url.bats diff --git a/commands/apply.go b/commands/apply.go index 6c5061d..34a81d4 100644 --- a/commands/apply.go +++ b/commands/apply.go @@ -72,7 +72,7 @@ func (c *ApplyCommand) ParsedArguments(args []string) (map[string]command.Argume func (c *ApplyCommand) FlagSet() *flag.FlagSet { f := c.Meta.FlagSet(c.Name(), command.FlagSetClient) - f.StringVar(&c.tasksFile, "tasks", "", "task file (YAML or JSON5) containing a task list. When omitted, docket probes tasks.yml -> tasks.yaml -> tasks.json in the current directory.") + f.StringVar(&c.tasksFile, "tasks", "", "task file (YAML or JSON5) containing a task list. When omitted, docket probes tasks.yml -> tasks.yaml -> tasks.json in the current directory. An http(s):// URL is fetched over HTTP.") f.BoolVar(&c.verbose, "verbose", false, "echo the resolved dokku command for each task as a continuation line. Values from inputs declared `sensitive: true` and from task struct fields tagged `sensitive:\"true\"` are masked as `***`. Ignored when --json is set; the JSON output already includes the resolved commands.") f.BoolVar(&c.json, "json", false, "emit one JSON-lines event per play/task/summary instead of human-readable output. Schema is keyed by `version: 1`; sensitive values mask to `***`.") f.StringVar(&c.host, "host", "", "remote dokku host as [user@]host[:port]; equivalent to DOKKU_HOST. Routes every dokku invocation through ssh.") @@ -87,7 +87,7 @@ func (c *ApplyCommand) FlagSet() *flag.FlagSet { f.StringVar(&c.startAtTask, "start-at-task", "", "skip every task before the matched name; the matched task and successors run normally. Filter order: --start-at-task -> --tags/--skip-tags -> per-task when: at execution. The name search walks every play in source order, narrowed by --play.") taskFile, format := resolveTaskFileFromArgs(os.Args) - data, err := os.ReadFile(taskFile) + data, err := readTaskFileData(taskFile) if err != nil { return f } @@ -152,7 +152,7 @@ func (c *ApplyCommand) Run(args []string) int { c.tasksFile = resolvedPath c.tasksFormat = resolvedFormat - data, err := os.ReadFile(c.tasksFile) + data, err := readTaskFileData(c.tasksFile) if err != nil { c.Ui.Error(fmt.Sprintf("read error: %v", err)) return 1 diff --git a/commands/init_test.go b/commands/init_test.go index fc405f9..99f64ba 100644 --- a/commands/init_test.go +++ b/commands/init_test.go @@ -156,6 +156,42 @@ func TestInitNameFlagSetsAppDefault(t *testing.T) { } } +// TestInitNameSetsPlayName: --name names the scaffolded play (not just the +// app input default) so `docket apply --play ` resolves after init. +// Every template variant is checked because the play name is what the +// documented `--name` behaviour promises. +func TestInitNameSetsPlayName(t *testing.T) { + cases := []struct { + label string + opts initOptions + format string + context map[string]interface{} + }{ + {"yaml-default", initOptions{Name: "web"}, tasks.FormatYAML, map[string]interface{}{"app": "web", "repo": "https://example.com/r.git"}}, + {"yaml-minimal", initOptions{Name: "web", Minimal: true}, tasks.FormatYAML, nil}, + {"json5-default", initOptions{Name: "web", Format: tasks.FormatNameJSON5}, tasks.FormatNameJSON5, map[string]interface{}{"app": "web", "repo": "https://example.com/r.git"}}, + {"json5-minimal", initOptions{Name: "web", Minimal: true, Format: tasks.FormatNameJSON5}, tasks.FormatNameJSON5, nil}, + } + for _, tc := range cases { + t.Run(tc.label, func(t *testing.T) { + out, err := renderInit(tc.opts) + if err != nil { + t.Fatalf("renderInit: %v", err) + } + plays, err := tasks.GetPlaysWithFormat(out, tc.format, tc.context, nil) + if err != nil { + t.Fatalf("GetPlaysWithFormat: %v\n%s", err, out) + } + if len(plays) != 1 { + t.Fatalf("plays = %d, want 1\n%s", len(plays), out) + } + if plays[0].Name != "web" { + t.Errorf("play name = %q, want \"web\"\n%s", plays[0].Name, out) + } + }) + } +} + func TestInitRepoFlagSetsRepoDefault(t *testing.T) { out, err := renderInit(initOptions{Name: "demo", Repo: "git@github.com:foo/bar.git"}) if err != nil { diff --git a/commands/plan.go b/commands/plan.go index a937d13..4bd9aa1 100644 --- a/commands/plan.go +++ b/commands/plan.go @@ -71,7 +71,7 @@ func (c *PlanCommand) ParsedArguments(args []string) (map[string]command.Argumen func (c *PlanCommand) FlagSet() *flag.FlagSet { f := c.Meta.FlagSet(c.Name(), command.FlagSetClient) - f.StringVar(&c.tasksFile, "tasks", "", "task file (YAML or JSON5) containing a task list. When omitted, docket probes tasks.yml -> tasks.yaml -> tasks.json in the current directory.") + f.StringVar(&c.tasksFile, "tasks", "", "task file (YAML or JSON5) containing a task list. When omitted, docket probes tasks.yml -> tasks.yaml -> tasks.json in the current directory. An http(s):// URL is fetched over HTTP.") f.BoolVar(&c.json, "json", false, "emit one JSON-lines event per play/task/summary instead of human-readable output. Schema is keyed by `version: 1`; sensitive values mask to `***`.") f.BoolVar(&c.detailedExitCode, "detailed-exitcode", false, "exit 0 when no drift is detected, 2 when drift is detected, 1 on error. Without this flag plan exits 0 regardless of drift.") f.StringVar(&c.host, "host", "", "remote dokku host as [user@]host[:port]; equivalent to DOKKU_HOST. Routes every dokku invocation through ssh.") @@ -84,7 +84,7 @@ func (c *PlanCommand) FlagSet() *flag.FlagSet { f.BoolVar(&c.listTasks, "list-tasks", false, "print the resolved task plan and exit without contacting the server. Honors --play / --tags / --skip-tags and shows expanded loop iterations and [skipped] markers for when:-skipped tasks.") taskFile, format := resolveTaskFileFromArgs(os.Args) - data, err := os.ReadFile(taskFile) + data, err := readTaskFileData(taskFile) if err != nil { return f } @@ -161,7 +161,7 @@ func (c *PlanCommand) Run(args []string) int { c.tasksFile = resolvedPath c.tasksFormat = resolvedFormat - data, err := os.ReadFile(c.tasksFile) + data, err := readTaskFileData(c.tasksFile) if err != nil { c.Ui.Error(fmt.Sprintf("read error: %v", err)) return 1 diff --git a/commands/task_file.go b/commands/task_file.go index 5cd8bd8..f6929f8 100644 --- a/commands/task_file.go +++ b/commands/task_file.go @@ -3,9 +3,13 @@ package commands import ( "errors" "fmt" + "io" + "net/http" + "net/url" "os" "path/filepath" "strings" + "time" ) // Task file format identifiers used throughout the commands package and @@ -27,10 +31,17 @@ var defaultTaskFileCandidates = []string{"tasks.yml", "tasks.yaml", "tasks.json" // detectTaskFileFormat returns "json5" when path's extension is .json or // .json5 (case-insensitive), and "yaml" otherwise. Unknown extensions // default to YAML so explicit paths like `--tasks recipe.txt` keep the -// pre-#218 behaviour. HTTP URLs and other non-filesystem paths flow -// through filepath.Ext just fine because they still carry an extension. +// pre-#218 behaviour. For an http(s) URL the format is taken from the URL +// path component so a trailing query string or fragment +// (`tasks.json?ref=main`) does not get glued onto the extension. func detectTaskFileFormat(path string) string { - switch strings.ToLower(filepath.Ext(path)) { + ext := filepath.Ext(path) + if isTaskFileURL(path) { + if u, err := url.Parse(path); err == nil { + ext = filepath.Ext(u.Path) + } + } + switch strings.ToLower(ext) { case ".json", ".json5": return taskFileFormatJSON5 default: @@ -38,6 +49,59 @@ func detectTaskFileFormat(path string) string { } } +// taskFileFetchTimeout bounds a remote recipe fetch so a hung server does +// not stall the whole command. +const taskFileFetchTimeout = 30 * time.Second + +// maxTaskFileBytes caps the size of a fetched recipe so a runaway or +// hostile response cannot exhaust memory. Recipes are small; 16 MiB is far +// above any realistic task file. +const maxTaskFileBytes = 16 << 20 + +// isTaskFileURL reports whether path is an http(s) URL docket should fetch +// over HTTP rather than read from the local filesystem. +func isTaskFileURL(path string) bool { + return strings.HasPrefix(path, "http://") || strings.HasPrefix(path, "https://") +} + +// readTaskFileData returns the bytes of the recipe at path. An http(s) URL +// is fetched over HTTP (apply and plan advertise a remote --tasks URL in +// their help); any other value is read from disk so the familiar +// os.ReadFile "no such file" error still surfaces for a mistyped path. +func readTaskFileData(path string) ([]byte, error) { + if isTaskFileURL(path) { + return fetchTaskFileURL(path) + } + return os.ReadFile(path) +} + +// fetchTaskFileURL GETs a recipe from an http(s) URL. A transport error, a +// non-2xx response, or a body larger than maxTaskFileBytes is reported as +// an error naming the URL so the read-error message stays actionable. +func fetchTaskFileURL(rawURL string) ([]byte, error) { + client := &http.Client{Timeout: taskFileFetchTimeout} + resp, err := client.Get(rawURL) + if err != nil { + return nil, fmt.Errorf("fetch %s: %w", rawURL, err) + } + defer resp.Body.Close() + + if resp.StatusCode < 200 || resp.StatusCode >= 300 { + return nil, fmt.Errorf("fetch %s: unexpected status %s", rawURL, resp.Status) + } + + // Read one byte past the cap so an over-limit body is detected rather + // than silently truncated. + data, err := io.ReadAll(io.LimitReader(resp.Body, maxTaskFileBytes+1)) + if err != nil { + return nil, fmt.Errorf("fetch %s: %w", rawURL, err) + } + if len(data) > maxTaskFileBytes { + return nil, fmt.Errorf("fetch %s: recipe exceeds %d bytes", rawURL, maxTaskFileBytes) + } + return data, nil +} + // hasTaskFileExtension reports whether path carries one of the recipe // file extensions. Used to spot a positional recipe path in an argv the // flag parser has not yet processed. diff --git a/commands/task_file_url_test.go b/commands/task_file_url_test.go new file mode 100644 index 0000000..9e36053 --- /dev/null +++ b/commands/task_file_url_test.go @@ -0,0 +1,160 @@ +package commands + +import ( + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "strings" + "testing" +) + +// serveRecipe starts an httptest server that returns body for any GET and +// returns the base URL plus a `/tasks.yml` recipe URL under it. The caller +// gets a URL whose path carries a .yml extension so detectTaskFileFormat +// resolves YAML. +func serveRecipe(t *testing.T, body string) *httptest.Server { + t.Helper() + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "text/yaml") + _, _ = w.Write([]byte(body)) + })) + t.Cleanup(srv.Close) + return srv +} + +// TestReadTaskFileDataFetchesURL: readTaskFileData GETs an http(s) URL and +// returns the response body verbatim. +func TestReadTaskFileDataFetchesURL(t *testing.T) { + const recipe = "---\n- tasks:\n - name: x\n dokku_app: { app: demo }\n" + srv := serveRecipe(t, recipe) + + data, err := readTaskFileData(srv.URL + "/tasks.yml") + if err != nil { + t.Fatalf("readTaskFileData(url) error: %v", err) + } + if string(data) != recipe { + t.Errorf("fetched body = %q, want %q", string(data), recipe) + } +} + +// TestReadTaskFileDataURLNon2xx: a non-2xx response is a clear error that +// names the status and the URL rather than a silent empty read. +func TestReadTaskFileDataURLNon2xx(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + http.Error(w, "nope", http.StatusNotFound) + })) + t.Cleanup(srv.Close) + + url := srv.URL + "/missing.yml" + _, err := readTaskFileData(url) + if err == nil { + t.Fatal("expected an error for a 404 response") + } + if !strings.Contains(err.Error(), "unexpected status") || !strings.Contains(err.Error(), "404") { + t.Errorf("error = %q, want it to mention the 404 status", err.Error()) + } + if !strings.Contains(err.Error(), url) { + t.Errorf("error = %q, want it to name the URL", err.Error()) + } +} + +// TestReadTaskFileDataLocalFile: a non-URL path still reads from disk. +func TestReadTaskFileDataLocalFile(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "tasks.yml") + const body = "---\n- tasks: []\n" + if err := os.WriteFile(path, []byte(body), 0o644); err != nil { + t.Fatalf("write: %v", err) + } + data, err := readTaskFileData(path) + if err != nil { + t.Fatalf("readTaskFileData(path) error: %v", err) + } + if string(data) != body { + t.Errorf("read body = %q, want %q", string(data), body) + } +} + +// TestReadTaskFileDataLocalMissing: a missing local path surfaces the +// familiar os.ReadFile error (not a URL fetch error). +func TestReadTaskFileDataLocalMissing(t *testing.T) { + _, err := readTaskFileData(filepath.Join(t.TempDir(), "nope.yml")) + if err == nil { + t.Fatal("expected an error for a missing local file") + } + if strings.Contains(err.Error(), "fetch ") { + t.Errorf("missing local file should not report a fetch error: %v", err) + } +} + +// TestDetectTaskFileFormatURL: URLs resolve their format from the path +// component so a trailing query string does not corrupt the extension. +func TestDetectTaskFileFormatURL(t *testing.T) { + cases := []struct { + path string + want string + }{ + {"http://h/docket/example.yml", taskFileFormatYAML}, + {"https://h/recipes/tasks.yaml", taskFileFormatYAML}, + {"http://h/tasks.json", taskFileFormatJSON5}, + {"http://h/tasks.json5", taskFileFormatJSON5}, + {"http://h/tasks.json?ref=main", taskFileFormatJSON5}, + {"https://h/a/b/tasks.yml?token=abc#frag", taskFileFormatYAML}, + {"http://h/tasks.json?download=recipe.yml", taskFileFormatJSON5}, + } + for _, tc := range cases { + if got := detectTaskFileFormat(tc.path); got != tc.want { + t.Errorf("detectTaskFileFormat(%q) = %q, want %q", tc.path, got, tc.want) + } + } +} + +// TestPlanFetchesRecipeFromURL: plan reads a recipe served over HTTP and +// runs it to completion (a read failure would exit 1 with "read error"). +func TestPlanFetchesRecipeFromURL(t *testing.T) { + defer stubReset() + stubSet("url-plan", StubFixture{}) + + srv := serveRecipe(t, `--- +- tasks: + - name: stub task + dokku_stub: + key: url-plan +`) + + stdout, stderr, exit := runPlan(t, srv.URL+"/tasks.yml") + if exit != 0 { + t.Fatalf("plan over URL exit = %d, want 0; stderr=%s", exit, stderr) + } + if !strings.Contains(stdout, "==> Play:") || !strings.Contains(stdout, "Plan:") { + t.Errorf("plan output did not complete a run over URL:\n%s", stdout) + } +} + +// TestApplyURLRecipePreregistersInputFlags: a URL recipe's own inputs are +// pre-registered as flags (which requires FlagSet fetching the URL), so an +// --input override both parses and propagates into the rendered recipe. +// The stub key resolves to the overridden app value; only the "override" +// key carries a changed fixture, so a propagated override yields 1 changed. +func TestApplyURLRecipePreregistersInputFlags(t *testing.T) { + defer stubReset() + stubSet("override", StubFixture{Changed: true}) + + srv := serveRecipe(t, `--- +- inputs: + - { name: app, default: baseline } + tasks: + - name: stub task + dokku_stub: + key: "{{ .app }}" +`) + + stdout, stderr, exit := runApply(t, srv.URL+"/tasks.yml", "--app=override") + if exit != 0 { + t.Fatalf("apply over URL with --app override exit = %d, want 0; stderr=%s", exit, stderr) + } + if !strings.Contains(stdout, "1 changed") { + t.Errorf("expected the --app override to propagate (1 changed); output:\n%s", stdout) + } +} diff --git a/commands/templates/default.json5.tmpl b/commands/templates/default.json5.tmpl index df6839c..d083ce5 100644 --- a/commands/templates/default.json5.tmpl +++ b/commands/templates/default.json5.tmpl @@ -1,5 +1,6 @@ [ { + name: <>, inputs: [ { name: "app", diff --git a/commands/templates/default.yml.tmpl b/commands/templates/default.yml.tmpl index 38d5eec..267c206 100644 --- a/commands/templates/default.yml.tmpl +++ b/commands/templates/default.yml.tmpl @@ -1,4 +1,5 @@ -- inputs: +- name: <> + inputs: - name: app default: <> description: "Application name" diff --git a/commands/templates/minimal.json5.tmpl b/commands/templates/minimal.json5.tmpl index 988552b..69cb771 100644 --- a/commands/templates/minimal.json5.tmpl +++ b/commands/templates/minimal.json5.tmpl @@ -1,5 +1,6 @@ [ { + name: <>, tasks: [ { name: "create app", diff --git a/commands/templates/minimal.yml.tmpl b/commands/templates/minimal.yml.tmpl index f09e0b8..2a13722 100644 --- a/commands/templates/minimal.yml.tmpl +++ b/commands/templates/minimal.yml.tmpl @@ -1,4 +1,5 @@ -- tasks: +- name: <> + tasks: - name: create app dokku_app: app: <> diff --git a/docs/command-reference.md b/docs/command-reference.md index f875c5b..542a4e6 100644 --- a/docs/command-reference.md +++ b/docs/command-reference.md @@ -151,7 +151,7 @@ always report drift with a `(... not probed)` reason. | Flag | Effect | |------|--------| -| `--tasks ` | Use a specific recipe. | +| `--tasks ` | Use a specific recipe. Accepts a local path or an `http(s)://` URL (fetched over HTTP). | | `--json` | Emit JSON-lines events instead of the human formatter. See [JSON output](json-output.md). | | `--detailed-exitcode` | Exit `0` for no drift, `2` for drift, `1` on error. Errors win over drift. Mirrors `terraform plan -detailed-exitcode`. | | `--vars-file ` | Load input values from a file (repeatable). See [inputs](inputs.md#layered-values-with---vars-file). | @@ -177,7 +177,7 @@ gets a status marker: |--------|---------| | `[ok]` | Ran, no change. | | `[changed]` | Ran, changed state. | -| `[skipped]` | Filtered out by tags, `when:`, or `--start-at-task`. | +| `[skipped]` | Filtered out by `when:` or `--start-at-task`. | | `[error]` | Errored. | A play header precedes the task lines, and a summary closes the run: @@ -196,7 +196,7 @@ summary still prints with partial counts. | Flag | Effect | |------|--------| -| `--tasks ` | Use a specific recipe. | +| `--tasks ` | Use a specific recipe. Accepts a local path or an `http(s)://` URL (fetched over HTTP). | | `--verbose` | After each task, echo every resolved Dokku command it ran, one per `→` line. Masked against sensitive values. Ignored with `--json` (which already includes commands). | | `--json` | Emit JSON-lines events instead of the human formatter. See [JSON output](json-output.md). | | `--vars-file ` | Load input values from a file (repeatable). See [inputs](inputs.md#layered-values-with---vars-file). | diff --git a/docs/getting-started.md b/docs/getting-started.md index 62c0838..a540208 100644 --- a/docs/getting-started.md +++ b/docs/getting-started.md @@ -65,8 +65,9 @@ BIN_DIR="$HOME/.local/bin" curl -fsSL https://raw.githubusercontent.com/dokku/do ``` On Linux and macOS the script installs to `/usr/local/bin` (using `sudo` only if that directory -is not writable). On Windows, run the script from a POSIX shell - Git Bash, MSYS2, Cygwin, or -WSL - and it installs `docket.exe` to `$HOME/bin`. +is not writable). On Windows, run the script from a POSIX shell - Git Bash, MSYS2, or Cygwin - and +it installs `docket.exe` to `$HOME/bin`. Under WSL the script sees a Linux environment, so it +installs the Linux binary to `/usr/local/bin` like any other Linux host. ### Homebrew (macOS, Linux) diff --git a/docs/inputs.md b/docs/inputs.md index 900566b..eaf89d9 100644 --- a/docs/inputs.md +++ b/docs/inputs.md @@ -188,8 +188,9 @@ Values are coerced to each input's declared `type`: - `string`: any scalar (a YAML boolean `true` becomes the string `"true"`). - `int`: whole numbers, including numeric strings and whole-valued JSON numbers. - `float`: floats, ints, and parseable numeric strings. -- `bool`: native booleans and the same string forms the CLI accepts (`true`/`yes`/`on`/`y` and - their false counterparts). +- `bool`: native booleans, or a string spelled `true`/`yes`/`on`/`y` (or `false`/`no`/`off`/`n`). + A `--name=value` flag on the command line is parsed separately by pflag, which accepts + `true`/`false`/`1`/`0` but not `yes`/`on`. A key in a vars file that does not match any declared input is a hard error, with a suggestion for the closest real name: diff --git a/docs/recipes.md b/docs/recipes.md index 44212d9..2c3806b 100644 --- a/docs/recipes.md +++ b/docs/recipes.md @@ -138,8 +138,7 @@ docket apply --tasks tasks.yml # default: stop this play, continue t docket apply --tasks tasks.yml --fail-fast # stop the whole run on the first error ``` -When a play is skipped (by its `when:` or because every task in it was skipped), the summary line -gains a `· N play skipped` segment: +When a play is skipped by its `when:`, the summary line gains a `· N play skipped` segment: ```text ==> Play: api diff --git a/install.sh b/install.sh index 6a1932e..9cf10ff 100755 --- a/install.sh +++ b/install.sh @@ -10,7 +10,8 @@ # BIN_DIR Destination directory (defaults to /usr/local/bin on Linux/macOS, # $HOME/bin on Windows shells). # -# Windows is supported through a POSIX shell: Git Bash, MSYS2, Cygwin, or WSL. +# Windows is supported through a POSIX shell: Git Bash, MSYS2, or Cygwin. WSL reports itself as +# Linux, so it installs the native Linux binary to /usr/local/bin rather than docket.exe. set -eu diff --git a/tests/bats/init.bats b/tests/bats/init.bats index 6d16740..577ace9 100644 --- a/tests/bats/init.bats +++ b/tests/bats/init.bats @@ -124,3 +124,20 @@ CFG refute_output --partial "dokku_git_sync" refute_output --partial "inputs:" } + +@test "docket init --name sets the play name so --play resolves" { + cd "$BATS_TEST_TMPDIR" + run "$(docket_bin)" init --name web --minimal + assert_success + + run cat tasks.yml + assert_output --partial "name: web" + + # The scaffolded play is named after --name (not the legacy auto-name + # "tasks"), so --play web resolves after init. --minimal avoids the + # default template's required repo input, which is enforced before the + # --list-tasks branch. + run "$(docket_bin)" plan --tasks tasks.yml --play web --list-tasks + assert_success + assert_output --partial "create app" +} diff --git a/tests/bats/task_file_url.bats b/tests/bats/task_file_url.bats new file mode 100644 index 0000000..f9bc11a --- /dev/null +++ b/tests/bats/task_file_url.bats @@ -0,0 +1,84 @@ +#!/usr/bin/env bats + +load test_helper + +setup() { + docket_build +} + +teardown() { + if [ -n "${SERVER_PID:-}" ]; then + kill "$SERVER_PID" 2>/dev/null || true + wait "$SERVER_PID" 2>/dev/null || true + fi +} + +# docket apply / plan accept an http(s):// --tasks URL, fetched over HTTP. +# This drives the shipped binary against a local HTTP server; --list-tasks +# reads and parses the recipe without contacting a Dokku server, so the +# served recipe just needs a real task type and no required inputs. +@test "docket plan fetches a recipe from an http URL" { + command -v python3 >/dev/null 2>&1 || skip "python3 not available" + + mkdir -p "$BATS_TEST_TMPDIR/www" + cat >"$BATS_TEST_TMPDIR/www/tasks.yml" <<'EOF' +--- +- tasks: + - name: create app + dokku_app: + app: url-recipe + state: present +EOF + + # Bind an ephemeral port and write it out so the test never collides + # with a fixed port already in use on the runner. + python3 - "$BATS_TEST_TMPDIR/www" "$BATS_TEST_TMPDIR/port" <<'PY' & +import http.server, socketserver, sys, os +os.chdir(sys.argv[1]) +with socketserver.TCPServer(("127.0.0.1", 0), http.server.SimpleHTTPRequestHandler) as httpd: + with open(sys.argv[2], "w") as f: + f.write(str(httpd.server_address[1])) + httpd.serve_forever() +PY + SERVER_PID=$! + + for _ in $(seq 1 50); do + [ -s "$BATS_TEST_TMPDIR/port" ] && break + sleep 0.1 + done + [ -s "$BATS_TEST_TMPDIR/port" ] || { echo "http server did not start"; return 1; } + port="$(cat "$BATS_TEST_TMPDIR/port")" + + run "$(docket_bin)" plan --tasks "http://127.0.0.1:${port}/tasks.yml" --list-tasks + assert_success + assert_output --partial "create app" +} + +@test "docket plan reports a clear error for a 404 recipe URL" { + command -v python3 >/dev/null 2>&1 || skip "python3 not available" + + python3 - "$BATS_TEST_TMPDIR/port" <<'PY' & +import http.server, socketserver, sys +class H(http.server.BaseHTTPRequestHandler): + def do_GET(self): + self.send_error(404, "not found") + def log_message(self, *a): + pass +with socketserver.TCPServer(("127.0.0.1", 0), H) as httpd: + with open(sys.argv[1], "w") as f: + f.write(str(httpd.server_address[1])) + httpd.serve_forever() +PY + SERVER_PID=$! + + for _ in $(seq 1 50); do + [ -s "$BATS_TEST_TMPDIR/port" ] && break + sleep 0.1 + done + [ -s "$BATS_TEST_TMPDIR/port" ] || { echo "http server did not start"; return 1; } + port="$(cat "$BATS_TEST_TMPDIR/port")" + + run "$(docket_bin)" plan --tasks "http://127.0.0.1:${port}/missing.yml" --list-tasks + assert_failure + assert_output --partial "unexpected status" +} From e72e5d3b3b6b526b0039c3270df62fe17f152b46 Mon Sep 17 00:00:00 2001 From: Jose Diaz-Gonzalez Date: Tue, 14 Jul 2026 19:16:03 -0400 Subject: [PATCH 2/2] perf: fetch a --tasks URL once per invocation A remote `--tasks` URL was requested twice per command, once while parsing flags and once while running; the recipe bytes read during flag parsing are now reused when the same source is resolved, so `apply` and `plan` make a single request. Also reformats the new bats test to satisfy shfmt. --- commands/apply.go | 23 ++++++++++++++++++---- commands/plan.go | 23 ++++++++++++++++++---- commands/task_file_url_test.go | 35 ++++++++++++++++++++++++++++++++++ tests/bats/task_file_url.bats | 10 ++++++++-- 4 files changed, 81 insertions(+), 10 deletions(-) diff --git a/commands/apply.go b/commands/apply.go index 34a81d4..4ce1fe9 100644 --- a/commands/apply.go +++ b/commands/apply.go @@ -33,6 +33,14 @@ type ApplyCommand struct { listTasks bool startAtTask string arguments map[string]*Argument + + // tasksData caches the recipe bytes read while building the FlagSet (to + // pre-register input flags). Run reuses them instead of reading the + // source a second time, so a --tasks URL is fetched once per + // invocation. tasksDataSource records where those bytes came from; the + // cache is honored only when Run resolves the same source. + tasksData []byte + tasksDataSource string } func (c *ApplyCommand) Name() string { @@ -91,6 +99,8 @@ func (c *ApplyCommand) FlagSet() *flag.FlagSet { if err != nil { return f } + c.tasksData = data + c.tasksDataSource = taskFile arguments, err := registerInputFlags(f, data, format) if err != nil { @@ -152,10 +162,15 @@ func (c *ApplyCommand) Run(args []string) int { c.tasksFile = resolvedPath c.tasksFormat = resolvedFormat - data, err := readTaskFileData(c.tasksFile) - if err != nil { - c.Ui.Error(fmt.Sprintf("read error: %v", err)) - return 1 + // Reuse the bytes FlagSet already read for this source (see tasksData); + // for a --tasks URL this avoids a second HTTP fetch of the same recipe. + data := c.tasksData + if data == nil || c.tasksDataSource != c.tasksFile { + data, err = readTaskFileData(c.tasksFile) + if err != nil { + c.Ui.Error(fmt.Sprintf("read error: %v", err)) + return 1 + } } context := make(map[string]interface{}) diff --git a/commands/plan.go b/commands/plan.go index 4bd9aa1..d53bcf8 100644 --- a/commands/plan.go +++ b/commands/plan.go @@ -32,6 +32,14 @@ type PlanCommand struct { play string listTasks bool arguments map[string]*Argument + + // tasksData caches the recipe bytes read while building the FlagSet (to + // pre-register input flags). Run reuses them instead of reading the + // source a second time, so a --tasks URL is fetched once per + // invocation. tasksDataSource records where those bytes came from; the + // cache is honored only when Run resolves the same source. + tasksData []byte + tasksDataSource string } func (c *PlanCommand) Name() string { @@ -88,6 +96,8 @@ func (c *PlanCommand) FlagSet() *flag.FlagSet { if err != nil { return f } + c.tasksData = data + c.tasksDataSource = taskFile arguments, err := registerInputFlags(f, data, format) if err != nil { @@ -161,10 +171,15 @@ func (c *PlanCommand) Run(args []string) int { c.tasksFile = resolvedPath c.tasksFormat = resolvedFormat - data, err := readTaskFileData(c.tasksFile) - if err != nil { - c.Ui.Error(fmt.Sprintf("read error: %v", err)) - return 1 + // Reuse the bytes FlagSet already read for this source (see tasksData); + // for a --tasks URL this avoids a second HTTP fetch of the same recipe. + data := c.tasksData + if data == nil || c.tasksDataSource != c.tasksFile { + data, err = readTaskFileData(c.tasksFile) + if err != nil { + c.Ui.Error(fmt.Sprintf("read error: %v", err)) + return 1 + } } context := make(map[string]interface{}) diff --git a/commands/task_file_url_test.go b/commands/task_file_url_test.go index 9e36053..cea2879 100644 --- a/commands/task_file_url_test.go +++ b/commands/task_file_url_test.go @@ -6,6 +6,7 @@ import ( "os" "path/filepath" "strings" + "sync" "testing" ) @@ -158,3 +159,37 @@ func TestApplyURLRecipePreregistersInputFlags(t *testing.T) { t.Errorf("expected the --app override to propagate (1 changed); output:\n%s", stdout) } } + +// TestURLRecipeFetchedOncePerInvocation: FlagSet pre-registration and Run +// share a single fetch of a --tasks URL, so the recipe is requested once +// per command rather than twice. +func TestURLRecipeFetchedOncePerInvocation(t *testing.T) { + defer stubReset() + stubSet("cache-key", StubFixture{}) + + var mu sync.Mutex + hits := 0 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + mu.Lock() + hits++ + mu.Unlock() + _, _ = w.Write([]byte(`--- +- tasks: + - name: stub task + dokku_stub: + key: cache-key +`)) + })) + t.Cleanup(srv.Close) + + _, stderr, exit := runPlan(t, srv.URL+"/tasks.yml") + if exit != 0 { + t.Fatalf("plan over URL exit = %d, want 0; stderr=%s", exit, stderr) + } + mu.Lock() + got := hits + mu.Unlock() + if got != 1 { + t.Errorf("recipe fetched %d times, want 1 (FlagSet and Run should share one fetch)", got) + } +} diff --git a/tests/bats/task_file_url.bats b/tests/bats/task_file_url.bats index f9bc11a..2a7cc7b 100644 --- a/tests/bats/task_file_url.bats +++ b/tests/bats/task_file_url.bats @@ -46,7 +46,10 @@ PY [ -s "$BATS_TEST_TMPDIR/port" ] && break sleep 0.1 done - [ -s "$BATS_TEST_TMPDIR/port" ] || { echo "http server did not start"; return 1; } + [ -s "$BATS_TEST_TMPDIR/port" ] || { + echo "http server did not start" + return 1 + } port="$(cat "$BATS_TEST_TMPDIR/port")" run "$(docket_bin)" plan --tasks "http://127.0.0.1:${port}/tasks.yml" --list-tasks @@ -75,7 +78,10 @@ PY [ -s "$BATS_TEST_TMPDIR/port" ] && break sleep 0.1 done - [ -s "$BATS_TEST_TMPDIR/port" ] || { echo "http server did not start"; return 1; } + [ -s "$BATS_TEST_TMPDIR/port" ] || { + echo "http server did not start" + return 1 + } port="$(cat "$BATS_TEST_TMPDIR/port")" run "$(docket_bin)" plan --tasks "http://127.0.0.1:${port}/missing.yml" --list-tasks