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..5f36c6caa7 100644 --- a/libs/localenv/pipeline.go +++ b/libs/localenv/pipeline.go @@ -90,7 +90,22 @@ 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 { + // Put the validation detail in Msg (which is serialized) and do not wrap + // err: PipelineError.Error() appends a non-nil Err, which would duplicate + // the detail in text output, and E_USAGE — not the raw error — is the + // stable contract callers match on. Without this the --output json + // consumer would see only "invalid compute target flags" (Err is + // json:"-") and lose which flags conflict. + return p.fail(PhasePreflight, false, NewError(ErrUsage, nil, "invalid compute target flags: %s", err)) + } // 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 1823bcdcc9..5c55ecbca2 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..9e0ef43e5d 100644 --- a/libs/localenv/result.go +++ b/libs/localenv/result.go @@ -64,6 +64,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"