-
Notifications
You must be signed in to change notification settings - Fork 492
fix(cli): default functions download --use-docker to true (Go parity) #5793
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
Coly010
wants to merge
4
commits into
develop
Choose a base branch
from
columferry/cli-1862-functions-download-default-use-docker-to-true-go-parity
base: develop
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
03e7474
fix(cli): default functions download --use-docker to true (Go parity)
Coly010 2e24961
fix(cli): address Codex review feedback on functions download proxy path
Coly010 5df8d95
fix(cli): address second round of Codex review on functions download
Coly010 78e37ac
fix(cli): short-circuit empty projects before proxying in machine mode
Coly010 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
66 changes: 66 additions & 0 deletions
66
apps/cli/src/legacy/commands/functions/download/download.e2e.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,66 @@ | ||
| import { describe, expect, test } from "vitest"; | ||
| import { makeTempHome, runSupabase } from "../../../../../tests/helpers/cli.ts"; | ||
|
|
||
| // Argument-validation negatives for `functions download`. This validation | ||
| // lives in the Go CLI today (the legacy TS command proxies to it); a | ||
| // black-box subprocess test keeps these assertions valid through the | ||
| // eventual native TS port — it guards behavior, not implementation. Mirrors | ||
| // `deploy.e2e.test.ts`'s coverage for the sibling `--use-docker` default bug | ||
| // (CLI-1862). | ||
| // | ||
| // The mutex-conflict cases below fail before any network call (flag parsing | ||
| // / mutex validation), so no auth or linked project is required. | ||
|
|
||
| const E2E_TIMEOUT_MS = 30_000; | ||
| const SLUG = "download-e2e-basic"; | ||
| const FAKE_TOKEN = `sbp_${"0".repeat(40)}`; | ||
| const FAKE_REF = "a".repeat(20); | ||
|
|
||
| describe("supabase functions download (legacy) — argument validation", () => { | ||
| const conflicts = [ | ||
| { name: "--use-api + --use-docker", flags: ["--use-api", "--use-docker"] }, | ||
| { name: "--use-api + --legacy-bundle", flags: ["--use-api", "--legacy-bundle"] }, | ||
| { name: "--use-docker + --legacy-bundle", flags: ["--use-docker", "--legacy-bundle"] }, | ||
| ] as const; | ||
|
|
||
| for (const { name, flags } of conflicts) { | ||
| test(`rejects ${name} as mutually exclusive`, { timeout: E2E_TIMEOUT_MS }, async () => { | ||
| using home = makeTempHome(); | ||
| const { exitCode, stderr } = await runSupabase( | ||
| ["functions", "download", SLUG, "--project-ref", FAKE_REF, ...flags], | ||
| { | ||
| entrypoint: "legacy", | ||
| home: home.dir, | ||
| env: { HOME: home.dir, SUPABASE_ACCESS_TOKEN: FAKE_TOKEN }, | ||
| }, | ||
| ); | ||
| expect(exitCode).not.toBe(0); | ||
| expect(stderr).toMatch(/none of the others can be|mutually exclusive/i); | ||
| }); | ||
| } | ||
|
|
||
| // CLI-1862: `--use-docker` now defaults to `true` (Go parity). Before the | ||
| // fix, that default was counted as "explicitly selected" by the mutex | ||
| // check, so passing `--use-api` alone was incorrectly rejected as | ||
| // conflicting with the (unpassed) `--use-docker` default. Covered in | ||
| // `download.integration.test.ts` ("does not treat the --use-docker default | ||
| // as conflicting with an explicit --use-api") via a mocked platform API | ||
| // instead of here: now that the mutex check is fixed, `--use-api` alone | ||
| // passes validation and proceeds to the native downloader, which calls the | ||
| // real Management API with the fake token/ref — an argument-validation | ||
| // test shouldn't depend on that network round-trip. | ||
|
|
||
| // CLI-1862: the TS→Go proxy call must not forward the now-defaulted | ||
| // `--use-docker` alongside an explicit `--legacy-bundle` — the Go binary | ||
| // re-parses this argv itself and enforces the same mutual exclusivity, so | ||
| // forwarding both breaks `--legacy-bundle` outright. Covered in | ||
| // `download.integration.test.ts` ("forwards only --legacy-bundle to the Go | ||
| // proxy...") via a mocked `LegacyGoProxy` instead of here: unlike | ||
| // `--use-api`, `--legacy-bundle` routes to the Go binary's `RunLegacy` | ||
| // downloader, which calls `InstallOrUpgradeDeno` before any network call | ||
| // (`apps/cli-go/internal/functions/download/download.go`). Each e2e run | ||
| // gets a fresh `SUPABASE_HOME`, so this would trigger a real, uncached | ||
| // Deno download from GitHub on every run — a real cross-boundary | ||
| // dependency this suite shouldn't take on to prove a pure TS-side routing | ||
| // decision. | ||
| }); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.