From ebb36c7f35d97e22b8aabd25f8e271b916b698aa Mon Sep 17 00:00:00 2001 From: Clay McGinnis Date: Thu, 28 May 2026 11:12:17 -0700 Subject: [PATCH] fix(jobs): correct config-hint path and honor --dry-run on curated commands MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both bugs surfaced while smoke-testing the BYOC runtime/region default rollout in PR #26. 1. Shell completion for --runtime and --run-region looked up the wrong API path (/notebooks/config-hint instead of /me/jupyter/lab/config-hint), so findOperation returned nil and completion silently produced no suggestions. 2. The global --dry-run flag was respected only by dynamic API commands. Curated job-runs commands (create, list, logs, metrics) called execOnce/execWithRetry directly and so submitted real requests even with --dry-run set — meaning "wherobots job-runs create ... --dry-run" could submit a real job. Introduce an executeOrDryRun helper that prints the curl equivalent and returns (nil, nil) when --dry-run is set; route all curated commands through it. For create, also skip the script-upload step so dry-run produces no side effects at all. --- internal/commands/builder.go | 2 +- internal/commands/jobs.go | 67 +++++++++++++++++++++++------ internal/commands/jobs_test.go | 78 +++++++++++++++++++++++++++++++--- 3 files changed, 128 insertions(+), 19 deletions(-) diff --git a/internal/commands/builder.go b/internal/commands/builder.go index f7edfa6..a0a2cd0 100644 --- a/internal/commands/builder.go +++ b/internal/commands/builder.go @@ -112,7 +112,7 @@ func BuildRootCommand(cfg config.Config, runtimeSpec *spec.RuntimeSpec) *cobra.C parent.AddCommand(methodCommand) } - addJobsCustomCommands(root, cfg, runtimeSpec, client) + addJobsCustomCommands(root, cfg, runtimeSpec, client, flags) return root } diff --git a/internal/commands/jobs.go b/internal/commands/jobs.go index 98573c0..ddcc651 100644 --- a/internal/commands/jobs.go +++ b/internal/commands/jobs.go @@ -43,6 +43,7 @@ type jobsRunner struct { cfg config.Config runtime *spec.RuntimeSpec client *http.Client + flags *GlobalFlags createRun *spec.Operation createUploadURL *spec.Operation getDirectory *spec.Operation @@ -56,8 +57,8 @@ type jobsRunner struct { listRuns *spec.Operation } -func addJobsCustomCommands(root *cobra.Command, cfg config.Config, runtimeSpec *spec.RuntimeSpec, client *http.Client) { - runner, ok := newJobsRunner(cfg, runtimeSpec, client) +func addJobsCustomCommands(root *cobra.Command, cfg config.Config, runtimeSpec *spec.RuntimeSpec, client *http.Client, flags *GlobalFlags) { + runner, ok := newJobsRunner(cfg, runtimeSpec, client, flags) if !ok { return } @@ -81,7 +82,7 @@ func addJobsCustomCommands(root *cobra.Command, cfg config.Config, runtimeSpec * root.AddCommand(jobsCmd) } -func newJobsRunner(cfg config.Config, runtimeSpec *spec.RuntimeSpec, client *http.Client) (*jobsRunner, bool) { +func newJobsRunner(cfg config.Config, runtimeSpec *spec.RuntimeSpec, client *http.Client, flags *GlobalFlags) (*jobsRunner, bool) { if runtimeSpec == nil { return nil, false } @@ -90,11 +91,12 @@ func newJobsRunner(cfg config.Config, runtimeSpec *spec.RuntimeSpec, client *htt cfg: cfg, runtime: runtimeSpec, client: client, + flags: flags, createRun: findOperation(runtimeSpec, "POST", "/runs"), createUploadURL: findOperation(runtimeSpec, "POST", "/files/upload-url"), getDirectory: findOperation(runtimeSpec, "GET", "/files/dir"), getIntegration: findOperation(runtimeSpec, "GET", "/files/integration-dir"), - getConfigHint: findOperation(runtimeSpec, "GET", "/notebooks/config-hint"), + getConfigHint: findOperation(runtimeSpec, "GET", "/me/jupyter/lab/config-hint"), getMe: findOperation(runtimeSpec, "GET", "/users/me"), getOrganization: findOperation(runtimeSpec, "GET", "/organization"), getRun: findOperation(runtimeSpec, "GET", "/runs/{run_id}"), @@ -156,9 +158,18 @@ func (r *jobsRunner) newCreateCommand() *cobra.Command { return fmt.Errorf("invalid --output %q (expected text|json)", output) } - resolvedScript, err := r.prepareScript(cmd.Context(), script, name, noUpload, uploadPath) - if err != nil { - return err + dryRun := r.flags != nil && r.flags.DryRun + + // Skip script upload in dry-run mode: prepareScript would issue + // org/file-store lookups and a presigned-URL upload, none of which + // belong in a "no side effects" preview. + resolvedScript := script + if !dryRun { + var err error + resolvedScript, err = r.prepareScript(cmd.Context(), script, name, noUpload, uploadPath) + if err != nil { + return err + } } payload, err := buildRunPayload(resolvedScript, name, runtimeID, timeoutSec, argsRaw, sparkConfigs, depPypi, depFiles, jarMainClass) @@ -178,10 +189,13 @@ func (r *jobsRunner) newCreateCommand() *cobra.Command { query = append(query, executor.QueryPair{Key: "region", Value: runRegion}) } - respBody, err := r.execOnce(cmd.Context(), r.createRun, nil, query, string(payloadJSON)) + respBody, err := r.executeOrDryRun(cmd.Context(), cmd.OutOrStdout(), r.createRun, nil, query, string(payloadJSON)) if err != nil { return err } + if respBody == nil { + return nil + } runID := strings.TrimSpace(gjson.GetBytes(respBody, "id").String()) if runID == "" { @@ -612,15 +626,20 @@ func (r *jobsRunner) newLogsCommand() *cobra.Command { return fmt.Errorf("invalid --output %q (expected text|json)", output) } - if !follow { + dryRun := r.flags != nil && r.flags.DryRun + + if !follow || dryRun { size := 1000 if tail > 0 { size = tail } - respBody, err := r.execWithRetry(cmd.Context(), r.getRunLogs, []string{runID}, []executor.QueryPair{{Key: "cursor", Value: "0"}, {Key: "size", Value: fmt.Sprintf("%d", size)}}, "") + respBody, err := r.executeOrDryRun(cmd.Context(), cmd.OutOrStdout(), r.getRunLogs, []string{runID}, []executor.QueryPair{{Key: "cursor", Value: "0"}, {Key: "size", Value: fmt.Sprintf("%d", size)}}, "") if err != nil { return err } + if respBody == nil { + return nil + } if output == outputJSON { _, err = fmt.Fprintln(cmd.OutOrStdout(), string(respBody)) @@ -784,10 +803,13 @@ func (r *jobsRunner) newMetricsCommand() *cobra.Command { return fmt.Errorf("invalid --output %q (expected text|json)", output) } - respBody, err := r.execWithRetry(cmd.Context(), r.getRunMetrics, []string{runID}, nil, "") + respBody, err := r.executeOrDryRun(cmd.Context(), cmd.OutOrStdout(), r.getRunMetrics, []string{runID}, nil, "") if err != nil { return err } + if respBody == nil { + return nil + } if output == outputJSON { _, err = fmt.Fprintln(cmd.OutOrStdout(), string(respBody)) @@ -915,10 +937,13 @@ func (r *jobsRunner) executeList(cmd *cobra.Command, statuses []string, name, af query = append(query, executor.QueryPair{Key: "status", Value: normalized}) } - respBody, err := r.execWithRetry(cmd.Context(), r.listRuns, nil, query, "") + respBody, err := r.executeOrDryRun(cmd.Context(), cmd.OutOrStdout(), r.listRuns, nil, query, "") if err != nil { return err } + if respBody == nil { + return nil + } if output == outputJSON { _, err = fmt.Fprintln(cmd.OutOrStdout(), string(respBody)) @@ -1044,6 +1069,24 @@ func (r *jobsRunner) execOnce(ctx context.Context, op *spec.Operation, pathArgs return executor.Do(r.client, req) } +// executeOrDryRun routes the request through execWithRetry, unless the global +// --dry-run flag is set, in which case it prints the equivalent curl to w and +// returns (nil, nil). Callers MUST short-circuit when respBody is nil to avoid +// parsing a missing response. +func (r *jobsRunner) executeOrDryRun(ctx context.Context, w io.Writer, op *spec.Operation, pathArgs []string, query []executor.QueryPair, body string) ([]byte, error) { + if r.flags != nil && r.flags.DryRun { + req, err := executor.BuildRequest(ctx, r.cfg, r.runtime, op, pathArgs, query, body) + if err != nil { + return nil, err + } + if _, err := fmt.Fprintln(w, executor.RenderCurl(req, body)); err != nil { + return nil, err + } + return nil, nil + } + return r.execWithRetry(ctx, op, pathArgs, query, body) +} + func (r *jobsRunner) execWithRetry(ctx context.Context, op *spec.Operation, pathArgs []string, query []executor.QueryPair, body string) ([]byte, error) { const maxAttempts = 6 diff --git a/internal/commands/jobs_test.go b/internal/commands/jobs_test.go index e3e8126..7cfd34e 100644 --- a/internal/commands/jobs_test.go +++ b/internal/commands/jobs_test.go @@ -9,6 +9,7 @@ import ( "net/http/httptest" "os" "strings" + "sync/atomic" "testing" "time" @@ -832,6 +833,71 @@ func TestFormatMetricValue(t *testing.T) { } } +func TestJobsCreateDryRunSkipsRequest(t *testing.T) { + t.Parallel() + + var serverHits int32 + server := httptest.NewServer(http.HandlerFunc(func(_ http.ResponseWriter, _ *http.Request) { + atomic.AddInt32(&serverHits, 1) + })) + defer server.Close() + + root := buildJobsTestRoot(server.URL) + var out bytes.Buffer + root.SetOut(&out) + root.SetErr(&bytes.Buffer{}) + root.SetArgs([]string{ + "job-runs", "create", "s3://fake-bucket/x.py", + "--name", "dryrun-test", + "--dry-run", + }) + + if err := root.Execute(); err != nil { + t.Fatalf("Execute() error = %v", err) + } + + if hits := atomic.LoadInt32(&serverHits); hits != 0 { + t.Fatalf("expected no HTTP requests during --dry-run, got %d", hits) + } + + got := strings.TrimSpace(out.String()) + if !strings.HasPrefix(got, "curl -X POST '"+server.URL+"/runs") { + t.Fatalf("expected curl POST %s/runs, got %q", server.URL, got) + } + if !strings.Contains(got, "dryrun-test") { + t.Fatalf("expected curl output to include job name, got %q", got) + } +} + +func TestJobsListDryRunSkipsRequest(t *testing.T) { + t.Parallel() + + var serverHits int32 + server := httptest.NewServer(http.HandlerFunc(func(_ http.ResponseWriter, _ *http.Request) { + atomic.AddInt32(&serverHits, 1) + })) + defer server.Close() + + root := buildJobsTestRoot(server.URL) + var out bytes.Buffer + root.SetOut(&out) + root.SetErr(&bytes.Buffer{}) + root.SetArgs([]string{"job-runs", "list", "--dry-run"}) + + if err := root.Execute(); err != nil { + t.Fatalf("Execute() error = %v", err) + } + + if hits := atomic.LoadInt32(&serverHits); hits != 0 { + t.Fatalf("expected no HTTP requests during --dry-run, got %d", hits) + } + + got := strings.TrimSpace(out.String()) + if !strings.HasPrefix(got, "curl -X GET '"+server.URL+"/runs") { + t.Fatalf("expected curl GET %s/runs, got %q", server.URL, got) + } +} + func TestBuildRunPayloadRejectsBadDependency(t *testing.T) { t.Parallel() @@ -893,7 +959,7 @@ func TestCompleteRuntimesSkipsDisabled(t *testing.T) { t.Parallel() server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if r.Method == http.MethodGet && r.URL.Path == "/notebooks/config-hint" { + if r.Method == http.MethodGet && r.URL.Path == "/me/jupyter/lab/config-hint" { w.Header().Set("Content-Type", "application/json") _, _ = io.WriteString(w, `{"runtimes":{"hint":[{"id":"tiny","enabled":true},{"id":"small"},{"id":"x-large","enabled":false}]}}`) return @@ -913,7 +979,7 @@ func TestCompleteRuntimesSkipsDisabled(t *testing.T) { func newJobsRunnerForTest(baseURL string) *jobsRunner { cfg := config.Config{AppName: "wherobots", APIKey: "test-key", HTTPTimeout: time.Second} - runner, _ := newJobsRunner(cfg, jobsTestRuntimeSpec(baseURL), http.DefaultClient) + runner, _ := newJobsRunner(cfg, jobsTestRuntimeSpec(baseURL), http.DefaultClient, nil) return runner } @@ -975,10 +1041,6 @@ func jobsTestRuntimeSpec(baseURL string) *spec.RuntimeSpec { {Name: "key", Location: "query", Required: true, Type: "string"}, }, }, - { - Method: "GET", - Path: "/notebooks/config-hint", - }, { Method: "POST", Path: "/runs", @@ -1010,6 +1072,10 @@ func jobsTestRuntimeSpec(baseURL string) *spec.RuntimeSpec { Method: "GET", Path: "/runs", }, + { + Method: "GET", + Path: "/me/jupyter/lab/config-hint", + }, }, } }