Uh oh!
There was an error while loading. Please reload this page.
[VPEX][11] Default serverless target to v5 - #5962
Merged
Merged
Conversation
This was referenced Jul 17, 2026
Collaborator
Integration test reportCommit: 9d8fc59
8 interesting tests: 4 RECOVERED, 4 SKIP
Top 10 slowest tests (at least 2 minutes):
|
rugpanovforce-pushed
the
dbconnect/12-serverless-v5
branch
from
July 17, 2026 13:27
179ea18 to
846170aCompareanton-107
approved these changes
Jul 17, 2026
anton-107
left a comment
Contributor
There was a problem hiding this comment.
Reviewed as part of the full stack (#5960–#5965). Consolidating the default into defaultServerlessVersion and using it at both fallback sites is the right move. Approving.
Two things to fold in:
[medium] The v5 default has no end-to-end coverage — and nothing verifies
serverless-v5is actually published. No acceptance test exercises the default-serverless path (unpinned serverless job / bundle-serverless), and no test server servesserverless/serverless-v5/pyproject.toml(only v3/v4 exist underacceptance/localenv/*/test.toml). The two unit tests that "cover" the default (target_test.go:100,:153) assert against"serverless/serverless-"+defaultServerlessVersion— i.e. they compare the code to the same constant it uses, so they'd pass for any value. Net: ifserverless-v5isn't yet a published key in the (not-yet-public) constraints repo, a real user hitting the default getsE_ENV_UNSUPPORTEDat fetch and no test in the repo would catch it. Since "default to v5" is the whole point of this PR, an acceptance case that servesserverless-v5and drives the default path would be valuable.[low] Stale
v4comments left behind. This PR moved the default to v5 but two code comments still say v4:cmd/environments/compute.go:78("falls back to v4 in ResolveTarget") and:116("the v4 fallback …"), plus the acceptance commentacceptance/localenv/job-serverless-check/test.toml:7("rather than defaulting to v4"). A reader reasoning about the unpinned-job path is told v4 when it's now v5.
Reviewed with AI assistance (build + unit tests + adversarial verification against the checked-out top of stack).
rugpanovforce-pushed
the
dbconnect/11-cluster-name
branch
2 times, most recently
from
July 20, 2026 09:38
e5e16ab to
8e83519ComparePer the [P0] CLI Changes spec, the serverless stand-in used when the source does not pin a version is now serverless-v5 (was v4). This applies only to the fallback cases — a serverless job with no recorded version, and a bundle that records 'serverless' without a version. Explicitly passing --serverless-version is unaffected. Introduce defaultServerlessVersion in envkey.go and use it at both fallback sites so the default lives in one place. VS Code resolves the real version itself and passes --serverless-version explicitly, so this fallback applies only when the version is genuinely unknown. Review (anton-107, #5962): - Add serverless-default-check acceptance test: an unpinned serverless job resolves to serverless-v5 and fetches it end-to-end, so the v5 default is actually exercised (and would break if v5 were unpublished). - Assert concrete "serverless/serverless-v5" in the two default unit tests instead of "serverless-"+defaultServerlessVersion, which compared the constant to itself and passed for any value. - Update stale v4 comments left by the default move (compute.go and job-serverless-check/test.toml) to v5. Co-authored-by: Isaac
rugpanovforce-pushed
the
dbconnect/12-serverless-v5
branch
from
July 20, 2026 11:14
846170a to
9d8fc59Comparerugpanov
commented
Jul 20, 2026
ContributorAuthor
Addressed both findings:
Also cascade-rebased onto main after PR 10 (#5961) merged. |
rugpanov
enabled auto-merge
July 20, 2026 11:24
Uh oh!
There was an error while loading. Please reload this page.
eng-dev-ecosystem-bot
commented
Jul 20, 2026
Collaborator
Integration test reportCommit: 54b3b04
24 interesting tests: 10 flaky, 6 FAIL, 5 RECOVERED, 2 SKIP, 1 KNOWN
Top 50 slowest tests (at least 2 minutes):
|
yansonggao-db pushed a commit
to yansonggao-db/cli
that referenced
this pull request
Jul 21, 2026
…icks#5965) Stacked on databricks#5964 → databricks#5963 → databricks#5962 → databricks#5961 → databricks#5960 → databricks#5959. ## What `--serverless-version` is now documented to take a **bare number** (e.g. `5`) rather than `v5`. - Input `5` is the documented form; `v5`/`V5` are still accepted (tolerant). - Both map to the same env key `serverless/serverless-vN`, so the **environments-repo layout is unchanged** (still `serverless/serverless-v5/…`). ## Changes - Flag help: `e.g. v4` → `e.g. 5`; the `E_ENV_UNSUPPORTED` hint suggests `--serverless-version 5`. - `defaultServerlessVersion` stored in bare form (`"5"`); still resolves to `serverless-v5` via `NormalizeServerless`. - Acceptance scripts pass the bare form; env-key goldens unchanged (`serverless-vN`). - Unit tests cover bare + v-prefixed input and the default constant. ## Testing `go build ./...`, lint (0 issues), deadcode clean, unit + acceptance green. This pull request and its description were written by Isaac.
tanishgupta-db pushed a commit
to tanishgupta-db/cli
that referenced
this pull request
Jul 21, 2026
Stacked on databricks#5962 (serverless-v5) → databricks#5961 → databricks#5960 → databricks#5959. ## What Per the `[P0] CLI Changes` spec, a bad-flags usage error is now surfaced as **`E_USAGE`** at the **preflight** phase, through the phase/JSON contract — instead of a bare Cobra mutual-exclusion error printed before `RunE` (which produces no command JSON object). ## Why The VS Code extension branches on the JSON `error.code`. Previously, passing two target flags triggered Cobra's `MarkFlagsMutuallyExclusive` *before* `RunE`, so `--output json` emitted nothing structured. Now the conflict is caught inside the pipeline and reported like any other phase failure. ## Changes - Add the `E_USAGE` error code. - Validate target flags at the top of the pipeline's preflight → `E_USAGE`, `diskMutated=false`. - Drop `cmd.MarkFlagsMutuallyExclusive` and the redundant early-return in `runPipeline` so the conflict flows into the pipeline. - `flag-conflict` golden updated (now shows the phase table + preflight error); new `flag-conflict-json` asserts `error{code:"E_USAGE", failurePhase:"preflight"}`; pipeline unit test for the path. ## Testing `go build ./...`, lint (0 issues), deadcode clean, unit + acceptance green. This pull request and its description were written by Isaac.
tanishgupta-db pushed a commit
to tanishgupta-db/cli
that referenced
this pull request
Jul 21, 2026
…ricks#5964) Stacked on databricks#5963 (E_USAGE) → databricks#5962 → databricks#5961 → databricks#5960 → databricks#5959. ## What Reconciles the error codes with the `[P0] CLI Changes` spec table (spec item databricks#10). **No behavior change** — the code already emits the correct set. The spec's table was stale: - omitted `E_NOT_WRITABLE` (preflight) and `E_PYTHON_INSTALL` (provision), and - wrongly listed `E_UV_MISSING` as "reserved / not emitted" when the CLI does emit it at preflight. ## Changes - Annotate each `ErrorCode` constant with the phase that emits it, and document the two spec codes the CLI deliberately never emits: `E_PYTHON_POLICY` (no signal source yet) and `E_AUTH` (handled earlier by `MustWorkspaceClient`). This makes the constant block the authoritative reference the spec mirrors. - Fix a `TargetInfo` doc comment that still said "four precedence sources" — with `--cluster-name` there are now five flag sources (the JSON `source` value is still one of cluster/serverless/job/bundle). ## Note The spec doc's error-code table itself (in the VPEX ERD, a Google Doc) has been updated separately to match — that's outside this repo. ## Testing `go build ./...`, lint (0 issues), deadcode clean, unit + acceptance green (comment-only code change). This pull request and its description were written by Isaac.
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 freeto 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.
Stacked on #5961 (
--cluster-name) → #5960 → #5959.What
Per the
[P0] CLI Changesspec, the serverless stand-in used when the source does not pin a version is nowserverless-v5(wasv4).Applies only to the fallback cases:
--job-idwhose environment records no version, andserverlesswithout a version.Explicitly passing
--serverless-version <vN>is unaffected.Changes
defaultServerlessVersion = "v5"inenvkey.go, used at both fallback sites intarget.goso the default lives in one place.No acceptance goldens change: all serverless acceptance tests pass
--serverless-version v4explicitly, so none exercise the default path.Note
VS Code resolves the real serverless version itself and passes
--serverless-versionexplicitly (spec §63), so this fallback is only hit when the version is genuinely unknown — but v5 is now the correct stand-in per spec.This pull request and its description were written by Isaac.