fix(cli): functions deploy --jobs validation + bundler routing (Go parity)#5797
Draft
Coly010 wants to merge 2 commits into
Draft
Conversation
Go's only --jobs guard (cmd/functions.go:79-82) is `if useApi { ... }
else if maxJobs > 1 { error }` — keyed on the resolved --use-api value,
independent of Docker/legacy-bundle. The TS port instead guarded on
`!explicitUseApi && (useDocker || legacyBundle)`, so --use-docker=false
--jobs 2 silently passed in TS while Go rejected it, and the error text
diverged ("cannot be used with local bundling" vs Go's "--jobs must be
used together with --use-api").
Fixes CLI-1861.
…i too
Review of the --jobs guard fix surfaced the same presence-vs-resolved-value
bug one line above it: useLocalBundler keyed off explicitUseApi (true for
--use-api=false, since the flag was still typed), silently forcing the API
bundler even when the user explicitly disabled it. Go's `if useApi {
useDocker = false }` only forces that when the resolved value is true
(cmd/functions.go:79-80), so --use-api=false alone should fall through to
whatever --use-docker/--legacy-bundle already resolved to.
Also closes two test-coverage gaps flagged in review: the next/ shell shares
deployFunctions() but had no --jobs guard coverage, and --jobs 0's
normalize-to-1 branch had zero hits in either shell.
Contributor
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
What kind of change does this PR introduce?
Bug fix (Go parity).
What is the current behavior?
supabase functions deploy --jobsdiverges from the Go CLI in two related ways:--jobsguard only errored when local bundling (Docker/legacy-bundle) was active, so--use-docker=false --jobs 2(no--use-api) silently passed in TS while Go rejects it. The error text also differed from Go's wording.useLocalBundlerdecided Docker-vs-API routing based on whether--use-apiwas typed on the command line (explicitUseApi), not its resolved value — so--use-api=falsealone silently forced the API bundler instead of falling through to Docker, contrary to Go'sif useApi { useDocker = false }(which only fires when the resolved value is true).Go's only guard (
apps/cli-go/cmd/functions.go:79-82) is:What is the new behavior?
--jobs > 1is now rejected whenever--use-api's resolved value isfalse, regardless of--use-docker/--legacy-bundle, with the exact Go error text:--jobs must be used together with --use-api.useLocalBundler) now keys off the same resolvedflags.useApivalue, so--use-api=falsealone falls through to Docker/legacy-bundle instead of silently switching to the API path.deployFunctions()used by both thelegacyandnextshells, with regression coverage added in both.Fixes CLI-1861.