diff --git a/acceptance/localenv/cluster-name-ambiguous-json/out.test.toml b/acceptance/localenv/cluster-name-ambiguous-json/out.test.toml new file mode 100644 index 0000000000..d6187dcb04 --- /dev/null +++ b/acceptance/localenv/cluster-name-ambiguous-json/out.test.toml @@ -0,0 +1,3 @@ +Local = true +Cloud = false +EnvMatrix.DATABRICKS_BUNDLE_ENGINE = [] diff --git a/acceptance/localenv/cluster-name-ambiguous-json/output.txt b/acceptance/localenv/cluster-name-ambiguous-json/output.txt new file mode 100644 index 0000000000..ff557c87de --- /dev/null +++ b/acceptance/localenv/cluster-name-ambiguous-json/output.txt @@ -0,0 +1,42 @@ +{ + "schemaVersion": 1, + "command": "environments setup-local", + "ok": false, + "mode": "default", + "dryRun": true, + "greenfield": false, + "phases": [ + { + "phase": "preflight", + "status": "ok" + }, + { + "phase": "resolve", + "status": "error" + }, + { + "phase": "fetch", + "status": "pending" + }, + { + "phase": "merge", + "status": "pending" + }, + { + "phase": "provision", + "status": "pending" + }, + { + "phase": "validate", + "status": "pending" + } + ], + "warnings": [], + "error": { + "code": "E_RESOLVE", + "failurePhase": "resolve", + "message": "resolving cluster name \"dup\": there are 2 active clusters named \"dup\"; use --cluster-id to disambiguate", + "diskMutated": false + }, + "durationMs": 0 +} diff --git a/acceptance/localenv/cluster-name-ambiguous-json/script b/acceptance/localenv/cluster-name-ambiguous-json/script new file mode 100644 index 0000000000..434245a782 --- /dev/null +++ b/acceptance/localenv/cluster-name-ambiguous-json/script @@ -0,0 +1 @@ +musterr $CLI environments setup-local --cluster-name dup --dry-run --output json diff --git a/acceptance/localenv/cluster-name-ambiguous-json/test.toml b/acceptance/localenv/cluster-name-ambiguous-json/test.toml new file mode 100644 index 0000000000..d98e399ce1 --- /dev/null +++ b/acceptance/localenv/cluster-name-ambiguous-json/test.toml @@ -0,0 +1,20 @@ +EnvMatrix.DATABRICKS_BUNDLE_ENGINE = [] + +[Env] +DATABRICKS_LOCALENV_CONSTRAINT_SOURCE = "$DATABRICKS_HOST" + +# Two active clusters share the same name, so name→ID resolution is genuinely +# ambiguous and errors as an actionable E_RESOLVE at the resolve phase. +# (A terminated cluster sharing the name would be filtered out server-side via +# ListClustersFilterBy, so it does not cause a spurious collision — see +# GetClusterByName.) +[[Server]] +Pattern = "GET /api/2.1/clusters/list" +Response.Body = ''' +{ + "clusters": [ + {"cluster_id": "cid-123", "cluster_name": "dup", "state": "RUNNING", "spark_version": "15.4.x-scala2.12"}, + {"cluster_id": "cid-999", "cluster_name": "dup", "state": "RUNNING", "spark_version": "14.3.x-scala2.12"} + ] +} +''' diff --git a/acceptance/localenv/flag-conflict-json/out.test.toml b/acceptance/localenv/flag-conflict-json/out.test.toml new file mode 100644 index 0000000000..d6187dcb04 --- /dev/null +++ b/acceptance/localenv/flag-conflict-json/out.test.toml @@ -0,0 +1,3 @@ +Local = true +Cloud = false +EnvMatrix.DATABRICKS_BUNDLE_ENGINE = [] diff --git a/acceptance/localenv/flag-conflict-json/output.txt b/acceptance/localenv/flag-conflict-json/output.txt new file mode 100644 index 0000000000..84840872e3 --- /dev/null +++ b/acceptance/localenv/flag-conflict-json/output.txt @@ -0,0 +1,42 @@ +{ + "schemaVersion": 1, + "command": "environments setup-local", + "ok": false, + "mode": "default", + "dryRun": false, + "greenfield": false, + "phases": [ + { + "phase": "preflight", + "status": "error" + }, + { + "phase": "resolve", + "status": "pending" + }, + { + "phase": "fetch", + "status": "pending" + }, + { + "phase": "merge", + "status": "pending" + }, + { + "phase": "provision", + "status": "pending" + }, + { + "phase": "validate", + "status": "pending" + } + ], + "warnings": [], + "error": { + "code": "E_USAGE", + "failurePhase": "preflight", + "message": "invalid compute target flags: flags --cluster-id and --serverless-version are mutually exclusive; specify at most one", + "diskMutated": false + }, + "durationMs": 0 +} diff --git a/acceptance/localenv/flag-conflict-json/script b/acceptance/localenv/flag-conflict-json/script new file mode 100644 index 0000000000..a41f74e8de --- /dev/null +++ b/acceptance/localenv/flag-conflict-json/script @@ -0,0 +1 @@ +musterr $CLI environments setup-local --cluster-id abc --serverless-version v4 --output json diff --git a/acceptance/localenv/flag-conflict-json/test.toml b/acceptance/localenv/flag-conflict-json/test.toml new file mode 100644 index 0000000000..c63fe3fe10 --- /dev/null +++ b/acceptance/localenv/flag-conflict-json/test.toml @@ -0,0 +1 @@ +EnvMatrix.DATABRICKS_BUNDLE_ENGINE = [] diff --git a/acceptance/localenv/flag-conflict/output.txt b/acceptance/localenv/flag-conflict/output.txt index a6379fb10c..85a4bd285e 100644 --- a/acceptance/localenv/flag-conflict/output.txt +++ b/acceptance/localenv/flag-conflict/output.txt @@ -1 +1,7 @@ -Error: if any flags in the group [cluster-id cluster-name serverless-version job-id] are set none of the others can be; [cluster-id serverless-version] were all set +preflight error invalid compute target flags: flags --cluster-id and --serverless-version are mutually exclusive; specify at most one +resolve pending +fetch pending +merge pending +provision pending +validate pending +For more detail, re-run with --debug, or --output json to share a structured report. diff --git a/cmd/environments/sync.go b/cmd/environments/sync.go index cbe97da43d..fa2d450bcc 100644 --- a/cmd/environments/sync.go +++ b/cmd/environments/sync.go @@ -56,7 +56,10 @@ func addTargetFlags(cmd *cobra.Command) { cmd.Flags().String("constraint-source-url", "", "URL for the constraint source (overrides "+envConstraintSource+")") // Hide constraint-source-url from casual --help output; it is a power-user escape hatch. _ = cmd.Flags().MarkHidden("constraint-source-url") - cmd.MarkFlagsMutuallyExclusive("cluster-id", "cluster-name", "serverless-version", "job-id") + // The mutual exclusivity of the target flags is enforced in the pipeline's + // preflight (as E_USAGE) rather than via cmd.MarkFlagsMutuallyExclusive, so + // the conflict is reported through the phase/JSON contract the --output json + // consumer relies on, instead of a bare pre-RunE Cobra error. } // runPipeline builds and runs the setup-local Pipeline. @@ -77,12 +80,9 @@ func runPipeline(cmd *cobra.Command) error { Serverless: serverless, Job: job, } - // ValidateTargetFlags is kept despite MarkFlagsMutuallyExclusive above: - // it also validates the library path (no Cobra equivalent) and guards - // non-Cobra call paths such as tests that invoke runPipeline directly. - if err := libslocalenv.ValidateTargetFlags(targetFlags); err != nil { - return err - } + // Flag validation (including mutual exclusivity) happens in the pipeline's + // preflight, so a conflict is reported as E_USAGE through the phase/JSON + // contract rather than as a bare error here. mode := libslocalenv.ModeDefault if constraintsOnly { diff --git a/libs/localenv/pipeline.go b/libs/localenv/pipeline.go index c6f6f547d3..1c0df251ba 100644 --- a/libs/localenv/pipeline.go +++ b/libs/localenv/pipeline.go @@ -90,7 +90,16 @@ func (p *Pipeline) Run(ctx context.Context) (*Result, error) { // run drives the phases and returns the first phase error. Result bookkeeping // (phase status, error object) is handled by fail / markOK. func (p *Pipeline) run(ctx context.Context) error { - // Phase: preflight — manager detection, writability, package-manager availability. + // Phase: preflight — flag validation, manager detection, writability, + // package-manager availability. + // + // Incompatible target flags are a usage error (E_USAGE), reported at preflight + // before any other work so the failure flows through the phase/JSON reporting + // (a plain Cobra mutual-exclusion error would print no command JSON object, + // which the --output json consumer needs). + if err := ValidateTargetFlags(p.Flags); err != nil { + return p.fail(PhasePreflight, false, NewError(ErrUsage, err, "invalid compute target flags")) + } // P0 supports only uv; any other detected manager is a clean, non-blaming exit. if m := detectManager(p.ProjectDir); m != managerUv { return p.fail(PhasePreflight, false, NewError(ErrManagerUnsupported, nil, "%s", managerGuidance(m))) diff --git a/libs/localenv/pipeline_test.go b/libs/localenv/pipeline_test.go index 20263f70b7..d173f4645a 100644 --- a/libs/localenv/pipeline_test.go +++ b/libs/localenv/pipeline_test.go @@ -78,6 +78,25 @@ func newTestServer(t *testing.T) *httptest.Server { })) } +func TestPipelineRejectsConflictingTargetFlagsAtPreflight(t *testing.T) { + // Incompatible target flags are a usage error surfaced as E_USAGE at + // preflight, before any manager/writability/fetch work. + dir := writeProject(t) + p := &Pipeline{ + Mode: ModeDefault, Check: true, ProjectDir: dir, CacheDir: t.TempDir(), + Flags: TargetFlags{Cluster: "abc", Serverless: "v4"}, + Compute: stubCompute{}, PM: fakePM{py: "3.12", dbc: "17.2.0"}, + } + res, err := p.Run(t.Context()) + var pe *PipelineError + require.ErrorAs(t, err, &pe) + assert.Equal(t, ErrUsage, pe.Code) + assert.Equal(t, PhasePreflight, pe.FailurePhase) + assert.False(t, pe.DiskMutated) + require.NotNil(t, res.Error) + assert.Equal(t, ErrUsage, res.Error.Code) +} + func TestPipelineCheckMutatesNothing(t *testing.T) { dir := writeProject(t) before, _ := os.ReadFile(filepath.Join(dir, "pyproject.toml")) diff --git a/libs/localenv/result.go b/libs/localenv/result.go index f8d4919ad7..0d1e35ab0d 100644 --- a/libs/localenv/result.go +++ b/libs/localenv/result.go @@ -1,6 +1,9 @@ package localenv -import "fmt" +import ( + "encoding/json" + "fmt" +) // Command path components, defined once so a rename touches a single place // (spec §0 / invariant 8 / scenario 21). The verb is a subcommand of the @@ -64,6 +67,7 @@ const ( type ErrorCode string const ( + ErrUsage ErrorCode = "E_USAGE" ErrNoTarget ErrorCode = "E_NO_TARGET" ErrManagerUnsupported ErrorCode = "E_MANAGER_UNSUPPORTED" ErrUvMissing ErrorCode = "E_UV_MISSING" @@ -81,7 +85,8 @@ const ( // PipelineError is a failure carrying a stable code, the phase at which it // occurred, and whether disk was mutated before the failure. It marshals to the // --json error object (spec §6.2). Code and FailurePhase are the stable -// contract; Err holds the wrapped cause for errors.Is/As and is not serialized. +// contract; Err holds the wrapped cause for errors.Is/As and is not serialized +// directly (its text is folded into the "message" field — see MarshalJSON). type PipelineError struct { Code ErrorCode `json:"code"` FailurePhase PhaseName `json:"failurePhase"` @@ -101,6 +106,21 @@ func (e *PipelineError) Unwrap() error { return e.Err } +// MarshalJSON serializes the full message (Error(), i.e. Msg plus any wrapped +// cause) into the "message" field. Without this the --json error object would +// carry only Msg and drop the cause (Err is json:"-"), so a JSON consumer would +// get strictly less detail than the text output — e.g. "resolving cluster name" +// without the "ambiguous"/"not found" reason. Text and JSON must agree. +func (e *PipelineError) MarshalJSON() ([]byte, error) { + type alias PipelineError // avoid recursing into this method + return json.Marshal((*alias)(&PipelineError{ + Code: e.Code, + FailurePhase: e.FailurePhase, + Msg: e.Error(), + DiskMutated: e.DiskMutated, + })) +} + // NewError creates a PipelineError with a code and message. FailurePhase and // DiskMutated are filled in by the pipeline when it records the failure. The // message is formatted with fmt.Sprintf(format, args...); err may be nil.