Skip to content

fix(jobs): correct config-hint path and honor --dry-run on curated commands#29

Merged
ClayMav merged 1 commit into
mainfrom
clay/fix-completion-path-and-dryrun-curated
May 28, 2026
Merged

fix(jobs): correct config-hint path and honor --dry-run on curated commands#29
ClayMav merged 1 commit into
mainfrom
clay/fix-completion-path-and-dryrun-curated

Conversation

@ClayMav

@ClayMav ClayMav commented May 28, 2026

Copy link
Copy Markdown
Member

Summary

Two independent bugs surfaced while smoke-testing the BYOC runtime/region default rollout in PR #26. Neither was caught by CI because both fail silently.

BUG 1 — completion lookups used the wrong API path

internal/commands/jobs.go looked up /notebooks/config-hint for the --runtime/--run-region tab-completion. The actual studio-backend endpoint is /me/jupyter/lab/config-hint. findOperation returned nil, so completeConfigHint returned an empty slice — completion failed open and tab-completion silently produced no suggestions.

BUG 2 — curated job-runs create (and siblings) ignored --dry-run

The global --dry-run flag (registered in builder.go) was honored only by dynamic API commands. Curated job-runs create/list/logs/metrics called execOnce/execWithRetry directly, so wherobots job-runs create ... --dry-run would actually submit a job (confirmed empirically on staging).

The fix introduces a small executeOrDryRun helper on jobsRunner that prints the curl equivalent and returns (nil, nil) when --dry-run is set. All four curated entry points (create, logs, list, metrics) now route their primary request through it and short-circuit on nil respBody. For create specifically, prepareScript (org lookup + presigned-URL upload) is also skipped so dry-run produces no side effects at all.

Test plan

  • go test ./... passes.
  • TestJobsCompletionUsesConfigHintPath — asserts __complete job-runs create ... --run-region "" hits /me/jupyter/lab/config-hint and emits the seeded regions.
  • TestJobsCreateDryRunSkipsRequest — asserts job-runs create ... --dry-run issues zero HTTP requests and emits the expected curl -X POST '…/runs' line.
  • TestJobsListDryRunSkipsRequest — same shape for the read-only list path.
  • Manual smoke test on staging:
    • WHEROBOTS_API_URL=…staging… ./bin/wherobots __complete job-runs create s3://x.py --name x --run-region "" returns the four org regions + completion directive 4.
    • WHEROBOTS_API_URL=…staging… ./bin/wherobots job-runs create s3://fake-bucket/x.py --name dryrun-test --dry-run -y prints a curl line and creates no run.

@ClayMav ClayMav requested a review from sfishel18 May 28, 2026 18:19
@ClayMav ClayMav marked this pull request as ready for review May 28, 2026 18:19
@ClayMav ClayMav requested a review from a team as a code owner May 28, 2026 18:19
…mmands

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.
@ClayMav ClayMav force-pushed the clay/fix-completion-path-and-dryrun-curated branch from 4b5dcbd to ebb36c7 Compare May 28, 2026 18:23
@ClayMav ClayMav merged commit 80a4a6e into main May 28, 2026
3 checks passed
@ClayMav ClayMav deleted the clay/fix-completion-path-and-dryrun-curated branch May 28, 2026 18:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants