Skip to content

Commit eceb7d5

Browse files
feat(workers): bring the command family's output onto one shape (#6389)
## Summary The workers commands each grew their own way of saying "here is what happened" and "here is what to run next". This settles them on the shapes the rest of the legacy shell already uses. **No command changes what it does** — this is output, plus the coverage that pins it. - **Success trailers.** "What to run next" lines in `new`, `push`, `delete` and `status` move to `emitSuccessTrailer`, the way `stop`, `bootstrap`, `migration repair` and `gen signing-key` already emit theirs: printed once at the end of the run rather than inline, so a multi-worker push does not bury each worker's hint under the next worker's output. The commands within them are aqua'd. - **`list` advisories.** Both take the yellow `WARNING:` prefix and the two-line consequence shape `start`'s Docker notice uses. Each was one long sentence that re-flowed at a different width, directly under a table that lines its columns up. - **`list` drops the URL column.** Every worker's URL is the same host and prefix with the name on the end, and carrying it pushed the table past 130 columns for one derivable field — `renderGlamourTable` sizes each column to its widest cell and never wraps. `status` still renders it vertically, and every machine format still carries `url` per worker. - **`push` progress.** Per-worker announcements are counted (`Deploying Worker 1/2:`) and a multi-worker run closes with a summary. Each worker takes minutes; the name alone said nothing about how much of the run was left. - **`push` names what it never attempted.** The loop stops at the first failure and the error only names the worker that broke, leaving the rest to be reconstructed from argument order. On stderr in every format, machine ones included: that run is a CI run. - **`--project-ref` survives into `push`'s retry suggestions**, via the `legacyWorkersProjectRefSuffix` helper `status` and `delete` already use. A suggestion is copy-pasted verbatim, so one that dropped it re-resolved against whatever this checkout was linked to. Also adds unit coverage for `legacyRenderWorkerDetails`, pins the shared `-o env` refusal, and adds a guard (own commit) asserting no legacy boolean flag ships required — `Flag.boolean` alone builds a *required* param, and nothing in the existing suites notices. ## Stack On top of the `workers new` name prompt (#6349). Above it: `workers logs` (#6410), then `push --wait` (#6371) last, so the output work can ship independently of both. ## Linked issue [FUNC-851](https://linear.app/supabase/issue/FUNC-851/general-output-polish). Supabase maintainer, exempt from the `open-for-contribution` flow. ## Checklist - [x] The PR title follows [Conventional Commits](https://www.conventionalcommits.org/) --------- Co-authored-by: kanad <git@kanad.dev>
1 parent c636947 commit eceb7d5

20 files changed

Lines changed: 910 additions & 58 deletions
Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,71 @@
1+
import { describe, expect, it } from "vitest";
2+
import { Primitive, type Command } from "effect/unstable/cli";
3+
import {
4+
legacyCommandInternals,
5+
legacyFlattenSubcommands,
6+
legacyUserGlobalFlagParams,
7+
} from "../docs/legacy-docs-introspection.ts";
8+
import { legacyUnwrapParam } from "../shared/legacy-param-introspection.ts";
9+
import { legacyRoot } from "./root.ts";
10+
11+
/**
12+
* `Flag.boolean(name)` builds a bare `Single` param, and a bare `Single` is
13+
* *required* — omitting it fails the whole command with a missing-flag error
14+
* before the handler ever runs. Every boolean flag therefore has to be closed
15+
* off with `Flag.withDefault(false)` or `Flag.optional`.
16+
*
17+
* Nothing else catches this: handler integration tests build their flags record
18+
* directly, so they never touch the parser, and the required-ness is invisible
19+
* to the type checker because a required boolean flag still infers as
20+
* `boolean`. The flag only misbehaves when a real invocation omits it, which is
21+
* precisely the invocation no handler test makes — so the guard walks the
22+
* command tree instead of waiting for a command to be exercised end to end.
23+
*/
24+
25+
/**
26+
* The published getter for a primitive's kind — `Primitive.getTypeName`, whose
27+
* own doc example pins `Primitive.boolean` to `"boolean"`. Reading
28+
* `primitiveType._tag` instead would couple this guard to effect's runtime
29+
* representation, which this repo forbids in tests as well as in source.
30+
*
31+
* Derived from `Primitive.boolean` rather than written as the literal
32+
* `"boolean"`: were that name to change upstream, a hardcoded literal would
33+
* match nothing and leave the guard silently passing every command, which is
34+
* the one failure mode a regression test must not have.
35+
*/
36+
const BOOLEAN_TYPE_NAME = Primitive.getTypeName(Primitive.boolean);
37+
38+
function booleanFlagsRequiringAValue(command: Command.Command.Any): ReadonlyArray<string> {
39+
const internals = legacyCommandInternals(command);
40+
// All three parameter sets a command can be parsed with, not just its own:
41+
// `Command.withSharedFlags` puts inherited flags on `contextConfig`, and the
42+
// root's persistent flags arrive as `globalFlags`. A bare boolean introduced
43+
// through either would break every command that inherits it while a guard
44+
// reading only `config.flags` stayed green.
45+
const params = [
46+
...internals.config.flags,
47+
...internals.contextConfig.flags,
48+
...legacyUserGlobalFlagParams(command),
49+
];
50+
51+
// Throws rather than skipping if effect's internal shape moves, so this
52+
// cannot quietly degrade into a test that inspects nothing.
53+
const own = params.flatMap((flag) => {
54+
const unwrapped = legacyUnwrapParam(flag);
55+
if (unwrapped === undefined) {
56+
throw new Error(`Unrecognizable flag param on "${command.name}".`);
57+
}
58+
const { single, isOptional } = unwrapped;
59+
return Primitive.getTypeName(single.primitiveType) === BOOLEAN_TYPE_NAME && !isOptional
60+
? [`${command.name} --${single.name}`]
61+
: [];
62+
});
63+
64+
return [...own, ...legacyFlattenSubcommands(command).flatMap(booleanFlagsRequiringAValue)];
65+
}
66+
67+
describe("legacy boolean flag wiring", () => {
68+
it("gives every boolean flag a default, so omitting it is not a parse error", () => {
69+
expect(booleanFlagsRequiringAValue(legacyRoot)).toEqual([]);
70+
});
71+
});

‎apps/cli/src/legacy/commands/experimental/workers/delete/SIDE_EFFECTS.md‎

Lines changed: 14 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -77,11 +77,17 @@ wrapper emits for every command.
7777

7878
## Output Formats
7979

80-
| Mode | stdout | stderr |
81-
| ----------------------------- | --------------------------------------------------------------------------------------------- | --------------------------------------------- |
82-
| text (default) | the confirmation prompt, then what was deleted and kept | that nothing local was kept, when nothing was |
83-
| `--output-format json` | one structured result carrying `worker_name`, `project_ref`, `kept_*` | as above |
84-
| `--output-format stream-json` | the same result as a single terminal event | as above |
85-
| `-o json` / `yaml` / `toml` | the same payload in that encoding, and nothing else | as above |
86-
| `-o pretty` / `table` / `csv` | the text rendering — these fall through rather than encoding | as above |
87-
| `-o env` | refused **before** the DELETE; discovering it at emit time deleted the worker and then failed | the error |
80+
| Mode | stdout | stderr |
81+
| ----------------------------- | --------------------------------------------------------------------------------------------- | ------------------------------------------------------------------- |
82+
| text (default) | the confirmation prompt, then what was deleted and kept | that nothing local was kept when nothing was, and the redeploy hint |
83+
| `--output-format json` | one structured result carrying `worker_name`, `project_ref`, `kept_*` | neither — both are text-only |
84+
| `--output-format stream-json` | the same result as a single terminal event | neither — both are text-only |
85+
| `-o json` / `yaml` / `toml` | the same payload in that encoding, and nothing else | neither — both are text-only |
86+
| `-o pretty` / `table` / `csv` | the text rendering — these fall through rather than encoding | both, as in text |
87+
| `-o env` | refused **before** the DELETE; discovering it at emit time deleted the worker and then failed | the error |
88+
89+
A structured emission is the end of the run: the handler returns at
90+
`legacyEmitWorkersMachineOutput` or at `output.success`, so nothing in the text
91+
branch below it — the kept-nothing notice and the redeploy trailer — is reached.
92+
`-o pretty`, `table` and `csv` are the exception, since they encode nothing and
93+
fall through to that same text branch.

‎apps/cli/src/legacy/commands/experimental/workers/delete/delete.handler.ts‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import { Effect, Option } from "effect";
22
import { Output } from "../../../../../shared/output/output.service.ts";
3+
import { emitSuccessTrailer } from "../../../../../shared/cli/success-trailer.ts";
34
import { legacyAqua } from "../../../../shared/legacy-colors.ts";
45
import { legacyRenderWorkerDetails } from "../workers.format.ts";
56
import {
@@ -232,8 +233,9 @@ export const legacyWorkersDelete = Effect.fn("legacy.experimental.workers.delete
232233
// alone is not enough to redeploy from, so `push` would fail on the very
233234
// command this line recommends.
234235
if (keptSource !== undefined) {
235-
yield* output.raw(
236-
`Redeploy it with supabase experimental workers push ${name}${refSuffix}.\n`,
236+
// Trailer, like every other "what to run next" line in this shell.
237+
yield* emitSuccessTrailer(
238+
`Redeploy it with ${legacyAqua(`supabase experimental workers push ${name}${refSuffix}`)}.\n`,
237239
);
238240
}
239241
} else {

‎apps/cli/src/legacy/commands/experimental/workers/delete/delete.integration.test.ts‎

Lines changed: 65 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -72,13 +72,14 @@ describe("legacy workers delete", () => {
7272
// Nothing local is touched — that is what makes `push` a one-command undo.
7373
expect(existsSync(join(repo.dir, "supabase", "workers", "api", "index.js"))).toBe(true);
7474
expect(readFileSync(join(repo.dir, "supabase", "config.toml"), "utf8")).toBe(CONFIG);
75-
expect(out.stdoutText).toContain("supabase experimental workers push api");
75+
// The redeploy hint is a success trailer, which lands on stderr.
76+
expect(out.stderrText).toContain("supabase experimental workers push api");
7677
}).pipe(Effect.provide(layer), Effect.ensuring(Effect.sync(repo.cleanup)));
7778
});
7879

79-
// The refusal used to live at emit time, which on this command is *after* the
80-
// DELETE: `--yes -o env` removed the worker and then exited non-zero with no
81-
// payload, which a script reads as "the delete failed" and may retry.
80+
// The refusal has to precede the DELETE. At emit time `--yes -o env` would
81+
// remove the worker and then exit non-zero with no payload, which a script
82+
// reads as "the delete failed" and may retry.
8283
// Deletion never touches local files, so a malformed local config has no
8384
// business standing between the user and a worker they named explicitly.
8485
it.live("deletes a remote worker despite an unparseable local config", () => {
@@ -327,8 +328,8 @@ describe("legacy workers delete", () => {
327328
}).pipe(Effect.provide(layer), Effect.ensuring(Effect.sync(repo.cleanup)));
328329
});
329330

330-
// `interactive` follows stdout, so a plain `>` redirect reaches this branch
331-
// even from a live terminal — the case that used to delete without asking.
331+
// `interactive` follows stdout, so a plain `>` redirect reaches this branch even
332+
// from a live terminal — the case where deleting without asking would be worst.
332333
it.live("refuses when stdout is redirected and no --yes was given", () => {
333334
const repo = project();
334335
const { layer, http } = setupLegacyWorkers({
@@ -541,6 +542,63 @@ describe("legacy workers delete", () => {
541542
}).pipe(Effect.provide(layer), Effect.ensuring(Effect.sync(repo.cleanup)));
542543
});
543544

545+
it.live("pluralizes the live instance count in the confirmation", () => {
546+
const repo = project();
547+
const { layer, out } = setupLegacyWorkers({
548+
workdir: repo.dir,
549+
promptTextResponses: ["api"],
550+
routes: {
551+
...routes,
552+
[getRoute]: {
553+
status: 200,
554+
body: {
555+
data: workerResource({
556+
name: "api",
557+
instances: 3,
558+
instanceCounts: { declared: 3, live: 2, ready: 2, stale: 0 },
559+
}),
560+
},
561+
},
562+
},
563+
});
564+
565+
return Effect.gen(function* () {
566+
yield* legacyWorkersDelete({ name: "api", projectRef: Option.none() });
567+
568+
expect(out.stdoutText).toContain("2 running instances will be terminated");
569+
}).pipe(Effect.provide(layer), Effect.ensuring(Effect.sync(repo.cleanup)));
570+
});
571+
572+
// Scaled to zero: there is a tally, and it says nothing is running. Warning
573+
// about terminated instances there would invent a consequence.
574+
it.live("promises no terminations when nothing is running", () => {
575+
const repo = project();
576+
const { layer, out } = setupLegacyWorkers({
577+
workdir: repo.dir,
578+
promptTextResponses: ["api"],
579+
routes: {
580+
...routes,
581+
[getRoute]: {
582+
status: 200,
583+
body: {
584+
data: workerResource({
585+
name: "api",
586+
instances: 2,
587+
instanceCounts: { declared: 2, live: 0, ready: 0, stale: 0 },
588+
}),
589+
},
590+
},
591+
},
592+
});
593+
594+
return Effect.gen(function* () {
595+
yield* legacyWorkersDelete({ name: "api", projectRef: Option.none() });
596+
597+
expect(out.stdoutText).toContain("permanently deletes");
598+
expect(out.stdoutText).not.toContain("will be terminated");
599+
}).pipe(Effect.provide(layer), Effect.ensuring(Effect.sync(repo.cleanup)));
600+
});
601+
544602
// An orphan — deployed from another checkout — has no local entry and no local
545603
// directory, so there is nothing that was "kept" and `push` has no source to
546604
// redeploy from.
@@ -604,7 +662,7 @@ describe("legacy workers delete", () => {
604662
}).pipe(Effect.provide(layer), Effect.ensuring(Effect.sync(repo.cleanup)));
605663
});
606664

607-
// Deletion never reads the local source, so a `source` that no longer resolves
665+
// Deletion never reads the local source, so a `source` that does not resolve
608666
// inside the project must not block removing the remote worker.
609667
it.live("deletes the remote worker even when the configured source is unusable", () => {
610668
const repo = project('project_id = "demo"\n\n[workers.api]\nsource = "../../elsewhere"\n');

‎apps/cli/src/legacy/commands/experimental/workers/list/SIDE_EFFECTS.md‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -67,3 +67,7 @@ wrapper emits for every command.
6767
| `-o json` / `yaml` / `toml` | the same payload in that encoding, and nothing else | as above |
6868
| `-o pretty` / `table` / `csv` | the text rendering — these fall through rather than encoding | as above |
6969
| `-o env` | refused before any request; the payload carries a `workers` array a flat `KEY=value` list cannot express | the error |
70+
71+
The text table omits each worker's URL — it is the same host and prefix on
72+
every row, and carrying it made the table 137 columns wide. Every machine
73+
format still carries `url` per worker, and `workers status` renders it.

‎apps/cli/src/legacy/commands/experimental/workers/list/list.handler.ts‎

Lines changed: 31 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
import { Effect } from "effect";
22
import { Output } from "../../../../../shared/output/output.service.ts";
3+
import { legacyAqua, legacyYellow } from "../../../../shared/legacy-colors.ts";
4+
import { displayPath } from "../../../../../shared/workers/worker-paths.ts";
35
import { renderGlamourTable } from "../../../../output/legacy-glamour-table.ts";
46
import { legacyEmitWorkersMachineOutput, legacyRejectWorkersEnvOutput } from "../workers.output.ts";
57
import { LegacyPlatformApi } from "../../../../auth/legacy-platform-api.service.ts";
@@ -28,7 +30,15 @@ import type { LegacyWorkersListFlags } from "./list.command.ts";
2830
* count from the spec. `status` is where the live tally lives.
2931
*/
3032

31-
const HEADERS = ["NAME", "RUNTIME", "SIZE", "STATE", "INSTANCES", "URL"] as const;
33+
/**
34+
* No URL column. Every worker's URL is the same 40-odd characters of host and
35+
* prefix with the name on the end, which pushed the table past 130 columns to
36+
* carry one derivable field — `renderGlamourTable` sizes each column to its
37+
* widest cell and never wraps. `workers status` renders it, vertically, for the
38+
* same reason (see `workers.format.ts`), and every machine format still carries
39+
* `url` per worker.
40+
*/
41+
const HEADERS = ["NAME", "RUNTIME", "SIZE", "STATE", "INSTANCES"] as const;
3242

3343
interface WorkerRow {
3444
readonly name: string;
@@ -68,14 +78,21 @@ function runtimeLabel(row: WorkerRow): string {
6878
return runtimeLabelFor(row) ?? "-";
6979
}
7080

81+
/**
82+
* `api is` / `api, box are` — the subject of both advisories below, which only
83+
* ever differ in the verb.
84+
*/
85+
function nameList(names: ReadonlyArray<string>): string {
86+
return `${names.join(", ")} ${names.length === 1 ? "is" : "are"}`;
87+
}
88+
7189
function toCells(row: WorkerRow): ReadonlyArray<string> {
7290
return [
7391
row.name,
7492
runtimeLabel(row),
7593
row.deployed === undefined ? "-" : formatApiSize(row.deployed.spec.size),
7694
stateLabel(row),
7795
row.deployed === undefined ? "-" : String(row.deployed.spec.instances),
78-
row.url ?? "-",
7996
];
8097
}
8198

@@ -165,7 +182,7 @@ export const legacyWorkersList = Effect.fn("legacy.experimental.workers.list")(f
165182

166183
if (rows.length === 0) {
167184
yield* output.raw(
168-
"No workers found. Scaffold one with supabase experimental workers new <name>.\n",
185+
`No workers found. Scaffold one with ${legacyAqua("supabase experimental workers new <name>", process.stdout)}.\n`,
169186
);
170187
return;
171188
}
@@ -178,14 +195,20 @@ export const legacyWorkersList = Effect.fn("legacy.experimental.workers.list")(f
178195
// the source directory *before* inferring a runtime and fails with
179196
// `WorkerSourceMissingError`, so telling that user about runtime guessing
180197
// points them at the wrong prerequisite.
198+
//
199+
// Both are written the way this shell writes every other heads-up that is
200+
// not a failure: a yellow `WARNING:` prefix, then the consequence on its own
201+
// line (`start`'s Docker-on-Windows notice is the same two-line shape). A
202+
// single long sentence re-flows differently at every terminal width, right
203+
// under a table that lines its columns up.
181204
const unconfigured = rows
182205
.filter((row) => row.deployed !== undefined && !row.configured && row.local)
183206
.map((row) => row.name);
184207
if (unconfigured.length > 0) {
208+
const configDisplay = displayPath(project.projectRoot, project.configPath);
185209
yield* output.raw(
186-
`${unconfigured.join(", ")} ${
187-
unconfigured.length === 1 ? "is" : "are"
188-
} deployed but absent from supabase/config.toml: pushing from here would have to guess the runtime.\n`,
210+
`${legacyYellow("WARNING:")} ${nameList(unconfigured)} deployed but not in ${configDisplay}.\n` +
211+
`Pushing from here would have to guess the runtime.\n`,
189212
"stderr",
190213
);
191214
}
@@ -195,9 +218,8 @@ export const legacyWorkersList = Effect.fn("legacy.experimental.workers.list")(f
195218
.map((row) => row.name);
196219
if (remoteOnly.length > 0) {
197220
yield* output.raw(
198-
`${remoteOnly.join(", ")} ${
199-
remoteOnly.length === 1 ? "is" : "are"
200-
} deployed but ${remoteOnly.length === 1 ? "has" : "have"} no source in this project: scaffold or restore it before pushing from here.\n`,
221+
`${legacyYellow("WARNING:")} ${nameList(remoteOnly)} deployed with no source in this project.\n` +
222+
`Scaffold or restore before pushing from here.\n`,
201223
"stderr",
202224
);
203225
}

0 commit comments

Comments
 (0)