Skip to content

Commit aaad18d

Browse files
committed
Address review: guard uv install prompt, reject mixed serverless versions
Two correctness findings from the review of the follow-up changes: - confirmUvInstall could panic: EnsureAvailable is a library entry point reachable with a context that has no cmdio (e.g. a Pipeline built with context.Background()), where cmdio.IsPromptSupported panics. Guard with cmdio.HasIO and treat a missing cmdio as non-interactive (decline). - GetJobSparkVersion used only Environments[0] for serverless jobs. A job whose environments pin differing environment_version values (or a pinned-vs-unpinned mix) has no single correct target, so refuse and ask for --serverless, mirroring the existing job-cluster divergence check. Adds a test asserting confirmUvInstall declines (no panic) with no cmdio. Co-authored-by: Isaac
1 parent 52c4795 commit aaad18d

3 files changed

Lines changed: 32 additions & 5 deletions

File tree

cmd/localenv/compute.go

Lines changed: 20 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import (
66
"strconv"
77

88
databricks "github.com/databricks/databricks-sdk-go"
9+
"github.com/databricks/databricks-sdk-go/service/jobs"
910
)
1011

1112
// sdkCompute adapts the Databricks SDK to the localenv.ComputeClient interface.
@@ -60,10 +61,16 @@ func (c sdkCompute) GetJobSparkVersion(ctx context.Context, jobID string) (spark
6061
// The serverless environment version (e.g. "4") is recorded on the job's
6162
// environment spec, unlike the bundle path where it is unavailable. Return
6263
// it so ResolveTarget pins the matching serverless-vN instead of defaulting
63-
// to v4. Spec/version may be empty on older jobs; that falls back to v4.
64-
version := ""
65-
if spec := job.Settings.Environments[0].Spec; spec != nil {
66-
version = spec.EnvironmentVersion
64+
// to v4. An empty version (older jobs) falls back to v4 in ResolveTarget.
65+
version := environmentVersion(job.Settings.Environments[0])
66+
// Tasks can reference any environment_key, so if the job's environments do
67+
// not all share one version there is no single correct local environment
68+
// (mirrors the job-cluster check below). Refuse rather than guess from the
69+
// first. A pinned-vs-unpinned mix is also ambiguous, so compare raw values.
70+
for _, e := range job.Settings.Environments[1:] {
71+
if environmentVersion(e) != version {
72+
return "", false, "", fmt.Errorf("job %d has serverless environments with differing environment_version; pass --serverless explicitly to disambiguate", id)
73+
}
6774
}
6875
return "", true, version, nil
6976
}
@@ -86,3 +93,12 @@ func (c sdkCompute) GetJobSparkVersion(ctx context.Context, jobID string) (spark
8693

8794
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)
8895
}
96+
97+
// environmentVersion returns the serverless environment_version recorded on a
98+
// job environment, or "" when the spec or version is absent.
99+
func environmentVersion(e jobs.JobEnvironment) string {
100+
if e.Spec == nil {
101+
return ""
102+
}
103+
return e.Spec.EnvironmentVersion
104+
}

libs/localenv/uv.go

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -317,7 +317,10 @@ func confirmUvInstall(ctx context.Context) bool {
317317
if optIn, ok := env.GetBool(ctx, EnvAutoInstallUv); ok && optIn {
318318
return true
319319
}
320-
if !cmdio.IsPromptSupported(ctx) {
320+
// EnsureAvailable is a library entry point reachable with a context that has
321+
// no cmdio (e.g. Pipeline built with context.Background()); IsPromptSupported
322+
// would panic there. Treat a missing cmdio as non-interactive and decline.
323+
if !cmdio.HasIO(ctx) || !cmdio.IsPromptSupported(ctx) {
321324
return false
322325
}
323326
ok, err := cmdio.AskYesOrNo(ctx, "uv is not installed. Download and run the official uv installer (https://astral.sh/uv/install.sh)?")

libs/localenv/uv_test.go

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -236,6 +236,14 @@ func TestConfirmUvInstall(t *testing.T) {
236236
assert.False(t, confirmUvInstall(ctx))
237237
})
238238

239+
t.Run("missing_cmdio_declines_without_panic", func(t *testing.T) {
240+
// A context with no cmdio (library entry point) must not panic in
241+
// IsPromptSupported; it declines like any other non-interactive run.
242+
assert.NotPanics(t, func() {
243+
assert.False(t, confirmUvInstall(t.Context()))
244+
})
245+
})
246+
239247
t.Run("falsey_opt_in_does_not_consent_when_non_interactive", func(t *testing.T) {
240248
ctx, _ := cmdio.SetupTest(t.Context(), cmdio.TestOptions{})
241249
ctx = env.Set(ctx, EnvAutoInstallUv, "0")

0 commit comments

Comments
 (0)