From a369cfeed161d2d6413c1960a43a76181d756f4a Mon Sep 17 00:00:00 2001 From: Lukas Wuttke Date: Thu, 9 Jul 2026 13:59:33 +0200 Subject: [PATCH 1/3] feat(data ingest): plain-language copy + hide k8s ceremony + progress on every wait (#179) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RFC-0002 phase 1. Make the `tracebloc data ingest` run speak the user's language, stop showing Kubernetes on the happy path, and never block silently. Copy / presentation / progress only — behavior, exit codes, guards, and the destination-exists (exit 6) check are unchanged. - Rewrite the intro paragraph on-prem and jargon-free: data never leaves the user's own infrastructure; drop "upload"/"Kubernetes"/"cluster"; use "workspace" as the single deployment noun. - Reword the file move to "copy into your workspace's storage" / "Copying" / "Copied" (was upload channel / Uploading / Uploaded). - Collapse the "Step 2/4 Connect to your workspace" screen: connect quietly on the happy path (all logic + the exit-6 guard intact), narrate under --verbose. Renumber the visible steps to three: 1/3 Check your data, 2/3 Copy into your workspace, 3/3 Validate and load. - Move the cluster summary (release / jobs-manager / shared PVC) and the RWO-PVC note to --verbose-only (Detailf); they no longer print on the happy path. - Progress on every wait: spinners now wrap the submit-connect port-forward, the local file walk, the --overwrite teardown, and the staging reclaim. No silent blocking call remains on the happy path. Tests: update the stage-copy + cluster-summary assertions to the new copy; add TestPrintClusterSummary_VerboseOnly (fields hidden without --verbose, shown with it) and TestRunIngestionRun_SubmitConnectUsesSpinner (nil-safe spinner on the connect wait). Co-Authored-By: Claude Opus 4.8 --- internal/cli/coverage_test.go | 39 ++++++++++++- internal/cli/data.go | 92 ++++++++++++++++++------------ internal/cli/ingestion_run_test.go | 53 +++++++++++++++++ internal/push/stage.go | 10 ++-- internal/push/stage_test.go | 2 +- 5 files changed, 153 insertions(+), 43 deletions(-) diff --git a/internal/cli/coverage_test.go b/internal/cli/coverage_test.go index 850dd448..586ee645 100644 --- a/internal/cli/coverage_test.go +++ b/internal/cli/coverage_test.go @@ -48,8 +48,10 @@ func TestPrintPushPreflight_RendersKeyFacts(t *testing.T) { "label": "label", } + // Verbose so the cluster block renders too — it's --verbose-only now (see + // TestPrintClusterSummary_VerboseOnly). The local summary shows regardless. var buf bytes.Buffer - p := ui.New(&buf, ui.WithColor(false)) + p := ui.New(&buf, ui.WithColor(false), ui.WithVerbose(true)) printLocalSummary(p, layout, spec) printClusterSummary(p, release, pvc) out := buf.String() @@ -64,6 +66,41 @@ func TestPrintPushPreflight_RendersKeyFacts(t *testing.T) { } } +// TestPrintClusterSummary_VerboseOnly pins that the Kubernetes cluster detail +// (release / jobs-manager / shared PVC) is hidden on the default happy path and +// surfaces only under --verbose — the RFC-0002 §6 ceremony-hiding contract. +func TestPrintClusterSummary_VerboseOnly(t *testing.T) { + release := &cluster.ParentRelease{ + ReleaseName: "ingdemo", + ChartVersion: "1.4.2", + JobsManagerService: "http://jobs-manager.ingdemo.svc.cluster.local:8080", + } + pvc := &cluster.SharedPVC{ + ClaimName: "client-pvc", + MountPath: "/data/shared", + Phase: corev1.ClaimBound, + AccessModes: []corev1.PersistentVolumeAccessMode{corev1.ReadWriteMany}, + } + + // Default (non-verbose): none of the cluster plumbing leaks. + var quiet bytes.Buffer + printClusterSummary(ui.New(&quiet, ui.WithColor(false)), release, pvc) + for _, hidden := range []string{"ingdemo", "1.4.2", "client-pvc", "jobs-manager", "Target cluster"} { + if strings.Contains(quiet.String(), hidden) { + t.Errorf("non-verbose output leaked cluster detail %q:\n%s", hidden, quiet.String()) + } + } + + // --verbose: the same facts are shown. + var loud bytes.Buffer + printClusterSummary(ui.New(&loud, ui.WithColor(false), ui.WithVerbose(true)), release, pvc) + for _, want := range []string{"ingdemo", "1.4.2", "client-pvc"} { + if !strings.Contains(loud.String(), want) { + t.Errorf("verbose output missing cluster detail %q:\n%s", want, loud.String()) + } + } +} + // TestWritePushJSON checks the --output-json result serializes to // valid JSON with the expected fields. func TestWritePushJSON(t *testing.T) { diff --git a/internal/cli/data.go b/internal/cli/data.go index 6794b4b8..f96daf65 100644 --- a/internal/cli/data.go +++ b/internal/cli/data.go @@ -388,11 +388,10 @@ func runDataIngest(ctx context.Context, out, errOut io.Writer, a runDataIngestAr a.Printer.Banner("tracebloc", "data ingest") a.Printer.Para(strings.TrimSpace(` -This uploads a dataset from your machine into your tracebloc workspace so models -can be trained on it. Your files are sent to the Kubernetes cluster your -workspace was installed on — tracebloc checks them and loads them into a table -your training runs read from. Your data stays on that cluster the whole time; -other collaborators train against it without ever seeing the raw files.`)) +This ingests a dataset so models can train on it. Your files never leave your +own infrastructure — tracebloc copies them into your workspace's storage, +checks them, and loads them into a table your training runs read from. Other +collaborators can train against that table without ever seeing the raw files.`)) a.Printer.Hintf("Learn more: https://docs.tracebloc.io") // 0. Guided mode: prompt for any missing core inputs before @@ -471,8 +470,10 @@ other collaborators train against it without ever seeing the raw files.`)) // resolution below needs (the image list for target-size, the // CSV for schema inference). // err is the function's named return (see the --output-json defer - // at the top), so it's not redeclared here. + // at the top), so it's not redeclared here. The walk can take a moment + // on a large tree, so it gets a spinner — no blocking wait stays silent. var layout *push.LocalLayout + walkSpin := a.Printer.Spinner("Reading your files", "") switch { case push.IsTabular(a.Spec.Category): layout, err = push.DiscoverTabular(a.LocalPath) @@ -484,12 +485,13 @@ other collaborators train against it without ever seeing the raw files.`)) // image_classification + keypoint_detection: labels.csv + images/. layout, err = push.Discover(a.LocalPath) } + walkSpin.Stop() if err != nil { return &exitError{code: 3, err: err} } - a.Printer.Step(1, 4, "Check your dataset") - a.Printer.Hintf("Reading your files locally first — nothing has touched the cluster yet — so a layout or settings problem shows up right away.") + a.Printer.Step(1, 3, "Check your data") + a.Printer.Hintf("Reading your files locally first — nothing has touched your workspace yet — so a layout or settings problem shows up right away.") // 3a. Per-category spec resolution from the local data, so the // synthesized spec carries the right fields before validation. @@ -634,8 +636,12 @@ other collaborators train against it without ever seeing the raw files.`)) // Errors mirror that command's exit-code contract (3 for // kubeconfig, 4 for missing release) so behaviour is // consistent across pre-flight commands. - a.Printer.Step(2, 4, "Connect to your workspace") - a.Printer.Hintf("Finding your tracebloc workspace and the shared storage your dataset will live on.") + // Connecting to the workspace + discovering its shared storage is + // Kubernetes plumbing (release / PVC / jobs-manager) the happy path keeps + // quiet — it's no longer a numbered step (RFC-0002 §6). --verbose narrates + // it. ALL the logic below (discovery + the exit-6 destination guard) is + // unchanged; only the presentation moved. + a.Printer.Detailf("Connecting to your workspace and finding the shared storage your data will live on…") // 6. PVC discovery (needPVC) confirms the chart's shared-data PVC is // Bound before we waste time provisioning a Pod that can't mount it. opts := cluster.KubeconfigOptions{Path: a.Kubeconfig, Context: a.Context, Namespace: a.Namespace} @@ -649,8 +655,9 @@ other collaborators train against it without ever seeing the raw files.`)) // DiscoverParentRelease (#7) and flows into the stage/teardown pods + the // jobs-manager token mint below — no --ingestor-sa override. - // 7. Show what we found on the cluster — the customer's last look - // before any bytes move. + // 7. Under --verbose, show what we found on the cluster; the happy path + // keeps this Kubernetes detail hidden (printClusterSummary is a no-op + // without --verbose). printClusterSummary(a.Printer, release, pvc) // 8a. Destination guard (cli#70, P4-lite): re-ingesting an existing @@ -679,8 +686,8 @@ other collaborators train against it without ever seeing the raw files.`)) // live-only steps (stage + ingest) the customer just skipped. if a.DryRun { a.Printer.Newline() - a.Printer.Successf("Dry-run complete — your dataset and cluster check out; nothing was created.") - a.Printer.Hintf("A real run continues with step 3 (stage your files) and step 4 (run the ingestion).") + a.Printer.Successf("Dry-run complete — your data and workspace check out; nothing was created.") + a.Printer.Hintf("A real run continues with step 2 (copy into your workspace) and step 3 (validate and load).") if a.OutputJSON { writePushJSON(a.JSONOut, "dry-run", spec, nil, "", "") jsonEmitted = true @@ -695,16 +702,18 @@ other collaborators train against it without ever seeing the raw files.`)) // are case-sensitive on Linux MySQL and PVC paths always are, so // acting on a differently-cased --table would silently no-op the // DROP/rm and then "succeed". - a.Printer.Infof("Removing the existing %q first…", existingTable) plan := push.PlanTeardown(existingTable) - if _, terr := push.Teardown(ctx, cs, &push.SPDYExecutor{Config: resolved.RestConfig, Client: cs}, resolved.Namespace, plan, push.PodSpecOptions{ + rmSpin := a.Printer.Spinner(fmt.Sprintf("Removing the existing %q first", existingTable), "") + _, terr := push.Teardown(ctx, cs, &push.SPDYExecutor{Config: resolved.RestConfig, Client: cs}, resolved.Namespace, plan, push.PodSpecOptions{ Namespace: resolved.Namespace, PVCClaimName: pvc.ClaimName, PVCMountPath: pvc.MountPath, Table: existingTable, ServiceAccountName: release.IngestorSAName, Image: a.StagePodImage, - }); terr != nil { + }) + rmSpin.Stop() + if terr != nil { // The teardown drops the table before removing files, so a // partial failure can leave files the DB-backed guard can no // longer see — a plain re-run would upload everything and then @@ -726,10 +735,10 @@ other collaborators train against it without ever seeing the raw files.`)) // Exit code 7 ("staging failed") is distinct from the // pre-flight codes so customers can branch on whether the // failure was their environment vs the actual data transfer. - a.Printer.Step(3, 4, "Stage your files") - a.Printer.Hintf("Your files upload securely into your workspace's storage — set up and cleaned up for you.") + a.Printer.Step(2, 3, "Copy into your workspace") + a.Printer.Hintf("Your files are copied securely into your workspace's storage — set up and cleaned up for you.") progress := push.NewProgress(out, layout.TotalBytes, - fmt.Sprintf("Staging %s", a.Spec.Table)) + fmt.Sprintf("Copying %s", a.Spec.Table)) // Defer Finish so a failure path that returns BEFORE // StreamLayout (e.g. CreateStagePod fails on PSA rejection, // WaitForStagePodReady times out) still clears the TTY @@ -794,7 +803,7 @@ func runIngestionRun(ctx context.Context, out io.Writer, a runDataIngestArgs, ta // min) because the full Phase 4 lifecycle — submit + watch // + log stream — can run that long for large ingestions. // The chart's helm flow uses the same token-mint code path. - a.Printer.Step(4, 4, "Run the ingestion") + a.Printer.Step(3, 3, "Validate and load") if a.Detach { a.Printer.Hintf("Submitting the run — with --detach it keeps running on your workspace after this command returns; the reconnect command is shown below.") } else { @@ -814,9 +823,13 @@ func runIngestionRun(ctx context.Context, out io.Writer, a runDataIngestArgs, ta // through the kubeconfig-authenticated apiserver, same as // `kubectl port-forward`. Bugbot PR #10 r3 caught the // original broken-by-design direct-URL POST. - a.Printer.Infof("Connecting to your workspace to submit the run…") + // Connecting means opening a port-forward and blocking on a POST that can + // take ~30s, so it runs under a spinner — no wait on the happy path stays + // silent (RFC-0002 "progress on every wait"). + connectSpin := a.Printer.Spinner("Connecting to your workspace to submit the run", "") pf, err := portForwardJobsManagerFn(ctx, cs, resolved.RestConfig, resolved.Namespace, release.JobsManagerServiceName, release.JobsManagerPort) + connectSpin.Stop() if err != nil { return false, &exitError{code: 8, err: fmt.Errorf("setting up jobs-manager port-forward: %w", err)} } @@ -881,8 +894,8 @@ func runIngestionRun(ctx context.Context, out io.Writer, a runDataIngestArgs, ta // (push.StagingCleanupTimeout): a failed or slow reclaim must not // fail — or noticeably delay — a successful ingest. if shouldReclaimStaging(status) { - a.Printer.Infof("Reclaiming the temporary staging copy on the cluster…") - if cerr := cleanStagingFn(ctx, cs, + reclaimSpin := a.Printer.Spinner("Reclaiming the temporary copy", "") + cerr := cleanStagingFn(ctx, cs, &push.SPDYExecutor{Config: resolved.RestConfig, Client: cs}, resolved.Namespace, a.Spec.Table, push.PodSpecOptions{ Namespace: resolved.Namespace, @@ -891,8 +904,10 @@ func runIngestionRun(ctx context.Context, out io.Writer, a runDataIngestArgs, ta Table: a.Spec.Table, ServiceAccountName: release.IngestorSAName, Image: a.StagePodImage, - }); cerr != nil { - a.Printer.Warnf("Couldn't reclaim the temporary staging copy (%v). It's harmless — the next re-ingest of %q or a `tracebloc data delete %s` will clear it.", + }) + reclaimSpin.Stop() + if cerr != nil { + a.Printer.Warnf("Couldn't reclaim the temporary copy (%v). It's harmless — the next re-ingest of %q or a `tracebloc data delete %s` will clear it.", cerr, a.Spec.Table, a.Spec.Table) } } @@ -963,9 +978,8 @@ func classifyPushOutcome(res *submit.Result, err error) (string, *exitError) { } // printLocalSummary shows what the CLI found on disk plus the ingest -// settings it assembled — the detail under step 1 ("Check your -// dataset"). Split from the cluster summary so each sits under its own -// numbered step. Mirrors `cluster info`'s section/Field layout. +// settings it assembled — the detail under step 1 ("Check your data"). +// Mirrors `cluster info`'s section/Field layout. func printLocalSummary(p *ui.Printer, layout *push.LocalLayout, spec map[string]any) { cat, _ := spec["category"].(string) @@ -1017,17 +1031,23 @@ func printLocalSummary(p *ui.Printer, layout *push.LocalLayout, spec map[string] p.Field("destination", push.FinalDestPrefix(spec["table"].(string))) } -// printClusterSummary shows the discovered workspace cluster target — -// the detail under step 2 ("Connect to your workspace"). +// printClusterSummary shows the discovered workspace target. It's Kubernetes +// plumbing (release / jobs-manager / shared PVC) the happy path hides, so the +// whole block — header, fields, and the RWO-PVC note — prints only under +// --verbose (RFC-0002 §6). Discovery + guards are unchanged; this is +// presentation only. func printClusterSummary(p *ui.Printer, release *cluster.ParentRelease, pvc *cluster.SharedPVC) { + if !p.Verbose() { + return + } p.Section("Target cluster") - p.Field("release", fmt.Sprintf("%s (chart %s)", release.ReleaseName, release.ChartVersion)) - p.Field("jobs-manager", release.JobsManagerService) - p.Field("shared PVC", fmt.Sprintf("%s (%s)", pvc.ClaimName, pvc.Phase)) + p.Detailf("release: %s (chart %s)", release.ReleaseName, release.ChartVersion) + p.Detailf("jobs-manager: %s", release.JobsManagerService) + p.Detailf("shared PVC: %s (%s)", pvc.ClaimName, pvc.Phase) if !pvc.IsReadWriteMany() { - // Warn but don't block — RWO clusters still work; the scheduler + // Note but don't block — RWO clusters still work; the scheduler // co-locates the stage Pod with the existing mounter. - p.Warnf("PVC is %v, not ReadWriteMany — the stage Pod will co-locate with the existing mounter", pvc.AccessModes) + p.Detailf("PVC is %v, not ReadWriteMany — the stage Pod will co-locate with the existing mounter", pvc.AccessModes) } } diff --git a/internal/cli/ingestion_run_test.go b/internal/cli/ingestion_run_test.go index c7eb7af9..4320e7c7 100644 --- a/internal/cli/ingestion_run_test.go +++ b/internal/cli/ingestion_run_test.go @@ -6,6 +6,7 @@ import ( "encoding/json" "errors" "io" + "strings" "testing" "github.com/tracebloc/cli/internal/cluster" @@ -160,6 +161,58 @@ func TestRunIngestionRun_Matrix(t *testing.T) { } } +// TestRunIngestionRun_SubmitConnectUsesSpinner pins the RFC-0002 "progress on +// every wait" rule for the submit-connect step: opening the port-forward blocks +// on a POST that can take ~30s, so it must run under a live spinner, not a +// silent or plain line. Colour is forced on so the animated path (which emits +// the \r redraw) renders against a buffer; the returned handle is always +// safe to Stop (the nil-safe static path is exercised by the matrix test, which +// runs with colour off). +func TestRunIngestionRun_SubmitConnectUsesSpinner(t *testing.T) { + origMint, origPF, origRun, origClean := mintIngestorTokenFn, portForwardJobsManagerFn, submitRunFn, cleanStagingFn + defer func() { + mintIngestorTokenFn, portForwardJobsManagerFn, submitRunFn, cleanStagingFn = origMint, origPF, origRun, origClean + }() + mintIngestorTokenFn = func(context.Context, kubernetes.Interface, string, string, int64, []string) (*cluster.IngestorToken, error) { + return &cluster.IngestorToken{Token: "tok"}, nil + } + portForwardJobsManagerFn = func(context.Context, kubernetes.Interface, *rest.Config, string, string, int) (*submit.ForwardedConnection, error) { + return &submit.ForwardedConnection{LocalPort: 12345}, nil + } + submitRunFn = func(context.Context, submit.Options) (*submit.Result, error) { + return succeededResult(), nil + } + cleanStagingFn = func(context.Context, kubernetes.Interface, push.Executor, string, string, push.PodSpecOptions) error { + return nil + } + + target := &clusterTarget{ + Resolved: &cluster.ResolvedConfig{Namespace: "tracebloc"}, + Release: &cluster.ParentRelease{IngestorSAName: "ingestor", JobsManagerServiceName: "jm", JobsManagerPort: 8080}, + PVC: &cluster.SharedPVC{ClaimName: "pvc", MountPath: "/data/shared"}, + } + spec := map[string]any{"table": "t", "category": "image_classification", "intent": "train", "label": "label"} + + var buf bytes.Buffer + a := runDataIngestArgs{ + Spec: push.SpecArgs{Table: "t"}, + Printer: ui.New(&buf, ui.WithColor(true)), + } + if _, err := runIngestionRun(context.Background(), io.Discard, a, target, []byte("yaml"), spec); err != nil { + t.Fatalf("runIngestionRun: %v", err) + } + out := buf.String() + if !strings.Contains(out, "Connecting to your workspace to submit the run") { + t.Errorf("submit-connect wait is missing its status message:\n%q", out) + } + // A spinner redraws/clears its line with a carriage return; a plain Infof + // never would. This is what distinguishes "shows a spinner" from "prints a + // line". + if !strings.Contains(out, "\r") { + t.Errorf("submit-connect wait didn't render as a spinner (no \\r redraw):\n%q", out) + } +} + // TestSeamsWiredToRealFns guards that the indirection didn't accidentally // leave a seam nil (a nil seam would panic the money path in production). func TestSeamsWiredToRealFns(t *testing.T) { diff --git a/internal/push/stage.go b/internal/push/stage.go index 8cbc9172..e25d24c2 100644 --- a/internal/push/stage.go +++ b/internal/push/stage.go @@ -50,7 +50,7 @@ type StageOptions struct { Progress Progress // Out is where Stage prints non-progress diagnostic output - // (orphan warnings, upload-channel status, etc.). Nil = io.Discard. + // (orphan warnings, copy-channel status, etc.). Nil = io.Discard. // Progress bar output is separate — schollz writes to its own // configured writer (typically the same one). Out io.Writer @@ -106,7 +106,7 @@ func Stage(ctx context.Context, opts StageOptions) error { if err != nil { return err } - _, _ = fmt.Fprintf(opts.Out, "Opened a secure upload channel to your workspace.\n") + _, _ = fmt.Fprintf(opts.Out, "Opened a secure channel to your workspace's storage.\n") // 3. Defer cleanup. The deferred call uses a FRESH context with // its own deadline — if the parent ctx is cancelled (SIGINT, @@ -130,14 +130,14 @@ func Stage(ctx context.Context, opts StageOptions) error { // accept exec). Times out at StagePodReadyTimeout with // diagnostic hints from container statuses (image pull, // scheduling) when we have them. - _, _ = fmt.Fprintf(opts.Out, "Preparing the upload channel (up to %s)…\n", StagePodReadyTimeout) + _, _ = fmt.Fprintf(opts.Out, "Preparing the copy (up to %s)…\n", StagePodReadyTimeout) if _, err := WaitForStagePodReady(ctx, opts.Client, opts.Namespace, podName); err != nil { return err } // 5. Stream the tar. This is where actual bytes flow. The // progress bar (if TTY) renders during this call. - _, _ = fmt.Fprintf(opts.Out, "Uploading %d files (%s) into %q…\n", + _, _ = fmt.Fprintf(opts.Out, "Copying %d files (%s) into %q…\n", opts.Layout.FileCount(), HumanBytes(opts.Layout.TotalBytes), opts.Table) if err := StreamLayout(ctx, opts.Executor, @@ -147,7 +147,7 @@ func Stage(ctx context.Context, opts StageOptions) error { } // 6. Print "done" message. The deferred cleanup runs after this. - _, _ = fmt.Fprintf(opts.Out, "Uploaded %d files into %q.\n", + _, _ = fmt.Fprintf(opts.Out, "Copied %d files into %q.\n", opts.Layout.FileCount(), opts.Table) return nil } diff --git a/internal/push/stage_test.go b/internal/push/stage_test.go index c43adcd9..8de0bacd 100644 --- a/internal/push/stage_test.go +++ b/internal/push/stage_test.go @@ -82,7 +82,7 @@ func TestStage_HappyPath(t *testing.T) { // Diagnostic output: customer should see the lifecycle // breadcrumbs. Pin a few key phrases so a regression that // silences them is caught. - for _, want := range []string{"Opened a secure upload channel", "Preparing the upload channel", "Uploading", "Uploaded"} { + for _, want := range []string{"Opened a secure channel", "Preparing the copy", "Copying", "Copied"} { if !strings.Contains(out.String(), want) { t.Errorf("output missing %q:\n%s", want, out.String()) } From 6ec82cd20b4276218652b6ac6897de72a9b3929b Mon Sep 17 00:00:00 2001 From: Asad Iqbal Date: Thu, 9 Jul 2026 17:57:13 +0500 Subject: [PATCH 2/3] fix(data ingest): don't run workspace discovery silently on the happy path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bugbot #196: collapsing the "Connect to your workspace" step routed its only status line through Detailf (verbose-only), so the default happy path sat silent through several blocking apiserver round-trips — kubeconfig load, release + shared-PVC discovery (incl. the cluster-wide fallback scan), and the destination-exists check — until "2/3 Copy into your workspace" appeared. That contradicts the PR's own "progress on every wait" goal. Restore a visible status line ("Connecting to your workspace…") on the default path. Deliberately a plain line, not a spinner: discoverRelease can emit its own namespace-fallback note mid-call, which a spinner's \r redraw would clobber. Discovery + the exit-6 destination guard are unchanged; presentation only. Co-Authored-By: Claude Opus 4.8 --- internal/cli/data.go | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/internal/cli/data.go b/internal/cli/data.go index f96daf65..66e9d7e2 100644 --- a/internal/cli/data.go +++ b/internal/cli/data.go @@ -638,10 +638,16 @@ collaborators can train against that table without ever seeing the raw files.`)) // consistent across pre-flight commands. // Connecting to the workspace + discovering its shared storage is // Kubernetes plumbing (release / PVC / jobs-manager) the happy path keeps - // quiet — it's no longer a numbered step (RFC-0002 §6). --verbose narrates + // quiet — it's no longer a numbered step (RFC-0002 §6), and --verbose adds + // the release/PVC detail below. But the discovery itself is several blocking + // apiserver round-trips (kubeconfig load, release + PVC discovery, then the + // destination-exists check), so it still needs a visible status line — no + // silent wait on the happy path (RFC-0002 "progress on every wait"). + // A plain line, not a spinner: discoverRelease can print its own + // namespace-fallback note mid-call, and a spinner's \r redraw would clobber // it. ALL the logic below (discovery + the exit-6 destination guard) is // unchanged; only the presentation moved. - a.Printer.Detailf("Connecting to your workspace and finding the shared storage your data will live on…") + a.Printer.Infof("Connecting to your workspace…") // 6. PVC discovery (needPVC) confirms the chart's shared-data PVC is // Bound before we waste time provisioning a Pod that can't mount it. opts := cluster.KubeconfigOptions{Path: a.Kubeconfig, Context: a.Context, Namespace: a.Namespace} From 24d2f685d6f7e3115745d0ec917d0b0765ad95b9 Mon Sep 17 00:00:00 2001 From: Asad Iqbal Date: Thu, 9 Jul 2026 18:05:57 +0500 Subject: [PATCH 3/3] fix(data ingest): spinner on the submit POST + Windows-safe spinner test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bugbot #196 (2 findings on 6ec82cd): 1. Submit POST ran silent. The submit-connect spinner wrapped only the port-forward and was stopped before the blocking jobs-manager POST — which is the real wait (submit.SubmitTimeout caps it at 30s of synchronous server-side validation). Moved a spinner to submit.Run, next to the POST it covers, so the longest wait on the submit path no longer sits on a blank line. Corrected the stale data.go comment that attributed the ~30s cost to the port-forward. 2. Spinner test was Windows-broken. TestRunIngestionRun_SubmitConnectUsesSpinner asserted a \r redraw unconditionally, but Printer.Spinner uses a static one-liner (no \r) on Windows by design. Guard the assertion on runtime.GOOS != "windows". Co-Authored-By: Claude Opus 4.8 --- internal/cli/data.go | 8 +++++--- internal/cli/ingestion_run_test.go | 7 +++++-- internal/submit/submit.go | 18 +++++++++++++----- 3 files changed, 23 insertions(+), 10 deletions(-) diff --git a/internal/cli/data.go b/internal/cli/data.go index 66e9d7e2..f1912d73 100644 --- a/internal/cli/data.go +++ b/internal/cli/data.go @@ -829,9 +829,11 @@ func runIngestionRun(ctx context.Context, out io.Writer, a runDataIngestArgs, ta // through the kubeconfig-authenticated apiserver, same as // `kubectl port-forward`. Bugbot PR #10 r3 caught the // original broken-by-design direct-URL POST. - // Connecting means opening a port-forward and blocking on a POST that can - // take ~30s, so it runs under a spinner — no wait on the happy path stays - // silent (RFC-0002 "progress on every wait"). + // Opening the port-forward is a blocking wait (tunnel setup through the + // apiserver), so it runs under a spinner — no wait on the happy path stays + // silent (RFC-0002 "progress on every wait"). The submit POST itself is a + // separate ~30s synchronous wait; its spinner lives in submit.Run, next to + // the POST it covers. connectSpin := a.Printer.Spinner("Connecting to your workspace to submit the run", "") pf, err := portForwardJobsManagerFn(ctx, cs, resolved.RestConfig, resolved.Namespace, release.JobsManagerServiceName, release.JobsManagerPort) diff --git a/internal/cli/ingestion_run_test.go b/internal/cli/ingestion_run_test.go index 4320e7c7..cf4639a7 100644 --- a/internal/cli/ingestion_run_test.go +++ b/internal/cli/ingestion_run_test.go @@ -6,6 +6,7 @@ import ( "encoding/json" "errors" "io" + "runtime" "strings" "testing" @@ -207,8 +208,10 @@ func TestRunIngestionRun_SubmitConnectUsesSpinner(t *testing.T) { } // A spinner redraws/clears its line with a carriage return; a plain Infof // never would. This is what distinguishes "shows a spinner" from "prints a - // line". - if !strings.Contains(out, "\r") { + // line". On Windows the spinner is deliberately a static one-liner with no + // \r redraw (ui.Printer.Spinner sidesteps escape garbage on legacy + // consoles), so the redraw is only guaranteed off Windows. + if runtime.GOOS != "windows" && !strings.Contains(out, "\r") { t.Errorf("submit-connect wait didn't render as a spinner (no \\r redraw):\n%q", out) } } diff --git a/internal/submit/submit.go b/internal/submit/submit.go index adc03c68..e25681b4 100644 --- a/internal/submit/submit.go +++ b/internal/submit/submit.go @@ -96,11 +96,6 @@ func Run(ctx context.Context, opts Options) (*Result, error) { return nil, fmt.Errorf("building submit request: %w", err) } - resp, err := opts.Submitter.Submit(ctx, req) - if err != nil { - return nil, err - } - // A Printer for the human-facing status lines (spinner, detach // notes, summary). nil-safe fallback so callers that didn't thread // the --plain decision still get sensible auto-detected rendering. @@ -109,6 +104,19 @@ func Run(ctx context.Context, opts Options) (*Result, error) { p = ui.New(opts.Out) } + // The POST validates synchronously server-side (schema re-check, + // idempotency lookup, Job creation) up to SubmitTimeout (30s) — the + // single longest blocking wait on the submit path. It lives here rather + // than in the caller, so its progress does too: a spinner keeps the wait + // from sitting silent. Stopped before the announcement below so the two + // don't fight over the line. + submitSpin := p.Spinner("Submitting the run", "") + resp, err := opts.Submitter.Submit(ctx, req) + submitSpin.Stop() + if err != nil { + return nil, err + } + // Submission announcement. Customers see this whether or not // --detach is set. The raw namespace/job identifiers are kept only // where they're actionable (detach + replay, which need them to