Skip to content

Commit 3e2d456

Browse files
committed
Address review: read deprecated client field, OS-correct install prompt, tests
Follow-ups from the #5936 review (all non-blocking): - environmentVersion now falls back to the deprecated Spec.Client field when Spec.EnvironmentVersion is empty. client is the predecessor of environment_version and some jobs still pin via it; reading both means the v4 fallback and the differing-version guard observe whichever field carries the pin, instead of treating a client-pinned job as unversioned (which would silently resolve to v4 and let two differing client values slip past the guard). base_environment stays ignored — it is a path/ID, not a version. The divergence error message is now field-agnostic ("differing versions"). - confirmUvInstall's prompt now names the OS-specific installer URL via a shared uvInstallerURL helper (install.ps1 on Windows, install.sh elsewhere), which installUv also uses, so the prompt is transparent about what actually runs. - Tests: environmentVersion field precedence (compute_test.go) and serverless --job version resolution + v4 fallback (target_test.go). The end-to-end differing-versions rejection through GetJobSparkVersion is best exercised as an acceptance scenario alongside the job fixtures in #5833; noted on the PR. Co-authored-by: Isaac
1 parent ff237a9 commit 3e2d456

4 files changed

Lines changed: 80 additions & 8 deletions

File tree

cmd/localenv/compute.go

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,7 @@ func (c sdkCompute) GetJobSparkVersion(ctx context.Context, jobID string) (spark
6969
// first. A pinned-vs-unpinned mix is also ambiguous, so compare raw values.
7070
for _, e := range job.Settings.Environments[1:] {
7171
if environmentVersion(e) != version {
72-
return "", false, "", fmt.Errorf("job %d has serverless environments with differing environment_version; pass --serverless explicitly to disambiguate", id)
72+
return "", false, "", fmt.Errorf("job %d has serverless environments with differing versions; pass --serverless explicitly to disambiguate", id)
7373
}
7474
}
7575
return "", true, version, nil
@@ -94,11 +94,21 @@ func (c sdkCompute) GetJobSparkVersion(ctx context.Context, jobID string) (spark
9494
return "", false, "", fmt.Errorf("could not determine compute for job %d from its environments or job clusters (task-level compute is not supported); pass --cluster or --serverless explicitly", id)
9595
}
9696

97-
// environmentVersion returns the serverless environment_version recorded on a
97+
// environmentVersion returns the serverless environment version recorded on a
9898
// job environment, or "" when the spec or version is absent.
99+
//
100+
// The version can arrive in either of two fields. environment_version is the
101+
// current one; client is its deprecated predecessor ("Use environment_version
102+
// instead") and is still what some jobs pin. Reading both means the v4 fallback
103+
// and the divergence guard observe whichever field actually carries the pin,
104+
// rather than treating a client-pinned job as unversioned. base_environment is
105+
// deliberately ignored: it is a path/ID, not a version.
99106
func environmentVersion(e jobs.JobEnvironment) string {
100107
if e.Spec == nil {
101108
return ""
102109
}
103-
return e.Spec.EnvironmentVersion
110+
if e.Spec.EnvironmentVersion != "" {
111+
return e.Spec.EnvironmentVersion
112+
}
113+
return e.Spec.Client
104114
}

cmd/localenv/compute_test.go

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
package localenv
2+
3+
import (
4+
"testing"
5+
6+
"github.com/databricks/databricks-sdk-go/service/compute"
7+
"github.com/databricks/databricks-sdk-go/service/jobs"
8+
"github.com/stretchr/testify/assert"
9+
)
10+
11+
func TestEnvironmentVersion(t *testing.T) {
12+
cases := []struct {
13+
name string
14+
env jobs.JobEnvironment
15+
want string
16+
}{
17+
{"nil spec", jobs.JobEnvironment{}, ""},
18+
{"environment_version", jobs.JobEnvironment{Spec: &compute.Environment{EnvironmentVersion: "3"}}, "3"},
19+
// client is the deprecated predecessor of environment_version; some jobs
20+
// still pin via it, so it must be read when environment_version is empty.
21+
{"client fallback", jobs.JobEnvironment{Spec: &compute.Environment{Client: "2"}}, "2"},
22+
// environment_version wins when both are present.
23+
{"environment_version wins", jobs.JobEnvironment{Spec: &compute.Environment{EnvironmentVersion: "4", Client: "2"}}, "4"},
24+
// base_environment is a path/ID, not a version, and is ignored.
25+
{"base_environment ignored", jobs.JobEnvironment{Spec: &compute.Environment{BaseEnvironment: "/Workspace/env.yaml"}}, ""},
26+
}
27+
for _, tc := range cases {
28+
t.Run(tc.name, func(t *testing.T) {
29+
assert.Equal(t, tc.want, environmentVersion(tc.env))
30+
})
31+
}
32+
}

libs/localenv/target_test.go

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -90,6 +90,27 @@ func TestResolveJobClassicUsesSparkVersionReturn(t *testing.T) {
9090
assert.Equal(t, "dbr/15.4.x-scala2.12", ti.EnvKey)
9191
}
9292

93+
func TestResolveJobServerlessUsesRecordedVersion(t *testing.T) {
94+
// A serverless job (isServerless=true) pins its serverless version via the
95+
// third "recorded version" return; ResolveTarget must map it to the matching
96+
// serverless-vN rather than the classic dbr path.
97+
c := jobStubCompute{isServerless: true, version: "3"}
98+
ti, err := ResolveTarget(t.Context(), TargetFlags{Job: "42"}, c, BundleTarget{})
99+
require.NoError(t, err)
100+
assert.Equal(t, "job", ti.Source)
101+
assert.Empty(t, ti.SparkVersion)
102+
assert.Equal(t, "serverless/serverless-v3", ti.EnvKey)
103+
}
104+
105+
func TestResolveJobServerlessEmptyVersionFallsBackToV4(t *testing.T) {
106+
// When the job records no serverless version, ResolveTarget defaults to v4
107+
// (documented stand-in), matching the bundle serverless path.
108+
c := jobStubCompute{isServerless: true, version: ""}
109+
ti, err := ResolveTarget(t.Context(), TargetFlags{Job: "42"}, c, BundleTarget{})
110+
require.NoError(t, err)
111+
assert.Equal(t, "serverless/serverless-v4", ti.EnvKey)
112+
}
113+
93114
func TestValidateTargetFlagsMutuallyExclusive(t *testing.T) {
94115
assert.Error(t, ValidateTargetFlags(TargetFlags{Cluster: "a", Serverless: "v4"}))
95116
assert.NoError(t, ValidateTargetFlags(TargetFlags{Cluster: "a"}))

libs/localenv/uv.go

Lines changed: 14 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -323,22 +323,31 @@ func confirmUvInstall(ctx context.Context) bool {
323323
if !cmdio.HasIO(ctx) || !cmdio.IsPromptSupported(ctx) {
324324
return false
325325
}
326-
ok, err := cmdio.AskYesOrNo(ctx, "uv is not installed. Download and run the official uv installer (https://astral.sh/uv/install.sh)?")
326+
// Name the OS-specific installer URL that installUv will actually fetch, so
327+
// the prompt is transparent about what runs (install.ps1 on Windows).
328+
ok, err := cmdio.AskYesOrNo(ctx, "uv is not installed. Download and run the official uv installer ("+uvInstallerURL()+")?")
327329
return err == nil && ok
328330
}
329331

332+
// uvInstallerURL returns the URL of the official uv installer script that
333+
// installUv fetches for the current OS.
334+
func uvInstallerURL() string {
335+
if runtime.GOOS == "windows" {
336+
return "https://astral.sh/uv/install.ps1"
337+
}
338+
return "https://astral.sh/uv/install.sh"
339+
}
340+
330341
// installUv runs the official uv installer for the current OS. Unix uses the
331342
// shell installer; Windows uses the PowerShell installer, because the Unix
332343
// `sh`/`curl` pipeline is not available in a default Windows shell.
333344
// https://docs.astral.sh/uv/getting-started/installation/
334345
func installUv(ctx context.Context) error {
335346
var cmd []string
336347
if runtime.GOOS == "windows" {
337-
// https://astral.sh/uv/install.ps1
338-
cmd = []string{"powershell", "-ExecutionPolicy", "ByPass", "-Command", "irm https://astral.sh/uv/install.ps1 | iex"}
348+
cmd = []string{"powershell", "-ExecutionPolicy", "ByPass", "-Command", "irm " + uvInstallerURL() + " | iex"}
339349
} else {
340-
// https://astral.sh/uv/install.sh
341-
cmd = []string{"sh", "-c", "curl -LsSf https://astral.sh/uv/install.sh | sh"}
350+
cmd = []string{"sh", "-c", "curl -LsSf " + uvInstallerURL() + " | sh"}
342351
}
343352
// This downloads and runs a remote installer that mutates the user's machine
344353
// (~/.local/bin), so record exactly what ran before it fires — visible under

0 commit comments

Comments
 (0)