fix: convert all paths to relative for deploy - #3403
Merged
Conversation
Pull Request Test Coverage Report for Build 14339664396Details
💛 - Coveralls |
avallete
approved these changes
Apr 8, 2025
Merged
pull Bot
pushed a commit
to chizee/cli
that referenced
this pull request
Aug 3, 2026
…upabase#6005) Two `functions deploy` divergences from the pinned Go CLI (`apps/cli-go`), resolved per the CLI-1985 ruling (Colum, 2026-07-30: take each point's documented recommended option). Fixes CLI-1985 ## ⚖ Parity ruling applied ### Point 1 — `NPM_AUTH_TOKEN` is no longer forwarded into the Docker bundler (strict parity; **breaking** for private-registry users) **Decision:** remove the forwarding. The eszip bundler container now receives only `NPM_CONFIG_REGISTRY` from the host, exactly matching Go (`apps/cli-go/internal/functions/deploy/bundle.go:68-70`). **Evidence:** - The Go CLI never forwarded `NPM_AUTH_TOKEN` at any point in its history — only `NPM_CONFIG_REGISTRY` (added in `8e17f033`). - The Go-side PR proposing the token forwarding (supabase#4933, addressing supabase#4927) was **closed unmerged** on 2026-06-22 ("The command is now ported in TypeScript so I'm closing this PR"). - The TS-only forwarding came from supabase#5645, which ported the unmerged supabase#4933. CLI-1985 ruled strict parity over that TS-only addition. **User-visible change (flagging prominently):** users whose `.npmrc` expands `${NPM_AUTH_TOKEN}` for private npm registries will find `--use-docker` / `--legacy-bundle` deploys failing registry auth again (the pre-supabase#5645 and Go CLI behavior; re-opens the CI/CD-host case of supabase#4927). Workarounds: inline the token in `.npmrc`, or deploy via the default `--use-api` path. Per the strict-parity contract (stderr bytes included), no TS-only warning was added when the variable is set — a DX reviewer requested one and it was rejected on parity grounds; the breaking impact is documented here and in the commit message instead. **Shared-code caveat (per the ruling):** `dockerNpmEnv` lives in `apps/cli/src/shared/functions/deploy.ts` and serves **both** shells — `next/` (`functions deploy`) and `legacy/`. The removal therefore applies to the next/ shell too. The strict-parity contract only binds the legacy shell, but keeping one code path is the simplest correct design per repo policy, so next/ loses the forwarding as well — stated here explicitly. `functions serve` is unaffected (it has its own env handling, matching Go's serve which loads `supabase/functions/.env`). ### Point 2 — API-deploy upload paths re-anchored at the workdir (align to the pinned oracle; behaviour change) **Directive:** confirm the intended reference point first, then align or record. **Evidence found:** - Upstream Go **never** anchored deploy paths at the git root. The full history of `pkg/function/deploy.go` (pre- and post-monorepo move) shows `toRelPath` anchored at `os.Getwd()` since `29021998` ("convert all paths to relative for deploy", supabase#3403), unchanged since. The Go CLI chdirs to the workdir (`internal/utils/misc.go:238`), so `os.Getwd()` ≡ the workdir. - The TS git-root anchoring came from supabase#5755 (merged 2026-07-02), a deliberate TS-side monorepo fix closing supabase#3467 (Go hard-fails on imports outside the workdir: `failed to read file: open ../common/index.ts`) — **not** a port of newer upstream Go behavior. There is no newer upstream Go reference to record against. **Decision (per the ruling's matrix — upstream never did this → align):** uploaded multipart file names and the server-recorded `entrypoint_path` / `import_map_path` / `static_patterns` are now anchored at the workdir with Go's exact `toRelPath` semantics (relative to `os.Getwd()`, forward slashes, `../`-relative when the file lies outside the workdir). **Scope note:** supabase#5755's import-walk *containment boundary* (which files may be uploaded at all) is intentionally **kept** at the nearest git root. The boundary is a TS-only safeguard with no Go equivalent — Go's walker uploads any reachable import unbounded (and then hard-fails opening `..` paths through `afero.NewIOFS`, which is exactly bug supabase#3467). Reverting the boundary would re-break supabase#3467 and is outside CLI-1985's anchoring scope. **User-visible change:** in monorepos where the git root is an ancestor of the workdir, redeploys now record `supabase/functions/<slug>/index.ts` (matching what the Go CLI records and the dashboard shows for Go deploys) instead of `apps/myapp/supabase/functions/<slug>/index.ts`. Imports outside the workdir but inside the git root still deploy, uploading with Go-style `../`-relative names — the same name shape Go's `toRelPath` emits, so nothing new is required of the server. Non-monorepo projects (git root == workdir, the common case) are byte-for-byte unchanged. ## What changed - `apps/cli/src/shared/functions/deploy.ts` — `dockerNpmEnvNames` trimmed to `NPM_CONFIG_REGISTRY`; `deployViaApi` now threads the workdir as the path anchor through `uploadFunctionSource` / `writeSourceDeployForm` / `createSourceMetadata` while the git-root `sourceRoot` remains the containment boundary; ENOENT warn display paths follow the workdir anchor (matching Go's workdir-relative walker paths). Docker bind construction is untouched. - `apps/cli/src/legacy/commands/functions/deploy/deploy.integration.test.ts` — new regression test: workdir≠git-root monorepo deploy asserts workdir-anchored metadata, `../`-relative upload names, and the Go-parity `Uploading asset` stderr line. - `apps/cli/src/next/commands/functions/deploy/deploy.integration.test.ts` — the two git-root upload tests updated to the workdir anchoring; the npm env test now asserts `NPM_CONFIG_REGISTRY` is forwarded and `NPM_AUTH_TOKEN` is not. - `apps/cli/src/legacy/commands/functions/deploy/SIDE_EFFECTS.md` — env table states only `NPM_CONFIG_REGISTRY` is forwarded; new note documents the workdir anchoring and the TS-only git-root boundary. All four changed/added tests fail against the previous implementation and pass with this change.
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
3 participants
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
What is the current behavior?
What is the new behavior?
No error when bundling with import map flag
Additional context
Add any other context or screenshots.