Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions internal/cli/cluster.go
Original file line numberDiff line numberDiff line change
Expand Up@@ -119,8 +119,6 @@ func runClusterInfo(
kubeconfigPath, contextOverride, nsOverride string,
tokenExpiry int64,
) error {
p.Banner("tracebloc", "cluster diagnostics")

// Bind the active client's namespace exactly like the data commands do,
// so this pre-flight targets what `data ingest` will actually target —
// and so the multi-client "set your active client" remediation works
Expand Down
4 changes: 2 additions & 2 deletions internal/cli/cluster_info_test.go
Original file line numberDiff line numberDiff line change
Expand Up@@ -61,7 +61,7 @@ func mintTokenReactor(cs *fake.Clientset, token string, expiresAt time.Time) {

// A reached cluster hosting no tracebloc client exits 4 (distinct from the
// kubeconfig exit-3), still errors.Is-identifiable as ErrNoParentRelease. The
// banner + Kubeconfig section print before discovery fails.
// Kubeconfig section prints before discovery fails.
func TestRunClusterInfo_NoClientExit4(t *testing.T) {
out, err := runInfo(t, fake.NewSimpleClientset(), 600)
if got := ExitCodeFromError(err); got != 4 {
Expand All@@ -70,7 +70,7 @@ func TestRunClusterInfo_NoClientExit4(t *testing.T) {
if !errors.Is(err, cluster.ErrNoParentRelease) {
t.Errorf("want errors.Is(ErrNoParentRelease), got %v", err)
}
for _, want := range []string{"cluster diagnostics", "test-ctx"} {
for _, want := range []string{"test-ctx"} {
if !strings.Contains(out, want) {
t.Errorf("output missing %q:\n%s", want, out)
}
Expand Down
2 changes: 1 addition & 1 deletion internal/cli/data_delete_json_test.go
Original file line numberDiff line numberDiff line change
Expand Up@@ -226,7 +226,7 @@ func TestDataDeleteCmd_OutputJSONNeverPrompts(t *testing.T) {
if got["status"] != "error" {
t.Errorf("got %+v, want status=error", got)
}
// The human banner went to stderr, not stdout.
// The human output went to stderr, not stdout.
if strings.Contains(out.String(), "tracebloc") && !strings.Contains(out.String(), `"error"`) {
t.Errorf("human output leaked to stdout:\n%s", out.String())
}
Expand Down
4 changes: 2 additions & 2 deletions internal/cli/resources.go
Original file line numberDiff line numberDiff line change
Expand Up@@ -83,10 +83,10 @@ Exit codes:
// the jobs-manager env — the same source `cluster doctor` parses, so the two
// never disagree.
func runResourcesShow(ctx context.Context, p *ui.Printer, opts cluster.KubeconfigOptions) error {
p.Banner("tracebloc", "machine resources")
p.Newline()
Comment thread
LukasWodka marked this conversation as resolved.

binding := bindActiveClientNamespace(&opts)
target, err := resolveClusterTarget(ctx, p, opts, binding, false)
target, err := resolveClusterTargetFn(ctx, p, opts, binding, false)
if err != nil {
return binding.explain(err)
}
Expand Down
10 changes: 6 additions & 4 deletions internal/cli/resources_set.go
Original file line numberDiff line numberDiff line change
Expand Up@@ -131,8 +131,6 @@ Exit codes:
// clientset + fake helm Runner without the real kubeconfig path (the seam
// renderResources / ingestion_run_test already use).
func runResourcesSet(ctx context.Context, p *ui.Printer, pr prompter, opts cluster.KubeconfigOptions, req setReq) error {
p.Banner("tracebloc", "machine resources")

Comment thread
LukasWodka marked this conversation as resolved.
// Pure request checks first — no cluster needed, so a bad invocation fails
// fast with exit 2 (never touching a live cluster).
if err := validateRequestShape(req, pr != nil); err != nil {
Expand DownExpand Up@@ -234,6 +232,7 @@ func applyResourcesSet(ctx context.Context, p *ui.Printer, pr prompter, target *
// is still validated below, before anything mutates.
ceilingUnchanged := sameCeiling(desired, current)
if ceilingUnchanged && !phantomGPU {
p.Newline()
p.Successf("Each training run already uses up to %s — nothing to change.", perRunSize(desired))
return nil
}
Expand All@@ -243,6 +242,7 @@ func applyResourcesSet(ctx context.Context, p *ui.Printer, pr prompter, target *
// clean no-op — fall through to persist so BuildEnvSpec's explicit-empty
// GPU override lands and clears it; otherwise runs stay unschedulable /
// fall back to CPU while the heartbeat keeps advertising a GPU.
p.Newline()
Comment thread
cursor[bot] marked this conversation as resolved.
p.Infof("Your CPU and memory budget is unchanged — but this machine has no GPU while the cluster still requests one, so I'll clear that stale GPU setting so runs can schedule.")
}

Expand All@@ -266,7 +266,10 @@ func applyResourcesSet(ctx context.Context, p *ui.Printer, pr prompter, target *
return &exitError{code: exitFailure, err: fmt.Errorf(
"refusing to change the ceiling without confirmation: pass --yes, or run on a terminal")}
}
p.Newline()
// PromptHint self-leads with a blank line, so this opens with a single
// blank — no preceding Newline() (that stacked two: the #375 banner-
// removal regression Bugbot caught). Mirrors the dry-run path, which
// leans on Section's own leading newline.
p.PromptHint("tracebloc keeps about 1 core and 3 GiB for itself on top of this — it fits on this machine.")
proceed, cerr := pr.Confirm(fmt.Sprintf("Let each training run use up to %s?", perRunSize(desired)), true)
if cerr != nil {
Expand DownExpand Up@@ -535,7 +538,6 @@ func persistCeiling(ctx context.Context, p *ui.Printer, target *clusterTarget, o
}

if dryRun {
p.Newline()
p.Section("Dry run — nothing was changed")
p.Field("would set each run to", perRunSize(d))
if gpuRemoved {
Expand Down
38 changes: 38 additions & 0 deletions internal/cli/resources_set_test.go
Original file line numberDiff line numberDiff line change
Expand Up@@ -670,3 +670,41 @@ func TestSet_UntouchedGPUIsKept(t *testing.T) {
}

func boolPtr(b bool) *bool { return &b }

// proceedingPrompter answers the final confirm "yes"; the wizard prompts are
// unused on the flag-driven path.
type proceedingPrompter struct{}

func (proceedingPrompter) Input(string, string, string, func(string) error) (string, error) {
return "", errInteractiveCancelled
}
func (proceedingPrompter) Select(string, string, []string, string) (string, error) {
return "", errInteractiveCancelled
}
func (proceedingPrompter) Confirm(string, bool) (bool, error) { return true, nil }

// TestSet_ConfirmOpensWithSingleBlank: after the banner removal (#375) the
// flag-driven confirm path must still open with exactly ONE blank line.
// PromptHint self-leads with a newline, so a preceding Newline() stacked two —
// the command opened with a double blank (Bugbot #375). Pins the single-blank
// opening so the redundant Newline() can't creep back.
func TestSet_ConfirmOpensWithSingleBlank(t *testing.T) {
fakeHelm(t)
cs := csWith("8", "32Gi", map[string]string{"RESOURCE_LIMITS": "cpu=2,memory=8Gi"})
out, err := runSet(t, cs, proceedingPrompter{}, setReq{cores: "4", coresSet: true})
if err != nil {
t.Fatalf("flag-driven confirm + proceed should succeed: %v\n%s", err, out)
}
head := out
if len(head) > 48 {
head = head[:48]
}
// PromptHint emits "\n <hint>\n": exactly one leading newline, then two
// spaces. A double blank ("\n\n…") is the regression.
if !strings.HasPrefix(out, "\n ") {
t.Errorf("confirm path must open with a single blank line then the hint, got %q", head)
}
if strings.HasPrefix(out, "\n\n") {
t.Errorf("confirm path opens with a DOUBLE blank line (banner-removal regression): %q", head)
}
}
33 changes: 33 additions & 0 deletions internal/cli/resources_test.go
Original file line numberDiff line numberDiff line change
Expand Up@@ -85,6 +85,39 @@ func TestRenderResources_ShowsMachineAndTrainingCeiling(t *testing.T) {
}
}

// TestShow_OpensWithSingleBlank: after the banner removal (#375), the outer
// runResourcesShow must open with exactly ONE blank line before the view. The
// leading Newline() lives in runResourcesShow (before resolve, so a resolve-time
// redirect line also gets a blank) — a spot every renderResources-level test
// skips — so pin it by driving the outer function through the resolve seam.
// Mirrors TestSet_ConfirmOpensWithSingleBlank. (Asad review, #375.)
func TestShow_OpensWithSingleBlank(t *testing.T) {
t.Setenv("TRACEBLOC_CONFIG_DIR", t.TempDir())
orig := resolveClusterTargetFn
t.Cleanup(func() { resolveClusterTargetFn = orig })
cs := csWith("8", "32Gi", map[string]string{"RESOURCE_LIMITS": "cpu=4,memory=16Gi"})
resolveClusterTargetFn = func(_ context.Context, _ *ui.Printer, _ cluster.KubeconfigOptions, _ activeClientBinding, _ bool) (*clusterTarget, error) {
return resTarget(cs), nil
}

var buf bytes.Buffer
if err := runResourcesShow(context.Background(), ui.New(&buf, ui.WithColor(false)), cluster.KubeconfigOptions{Context: "my-ctx"}); err != nil {
t.Fatalf("runResourcesShow: %v\n%s", err, buf.String())
}
out := buf.String()
head := out
if len(head) > 48 {
head = head[:48]
}
// Stat does not self-lead, so the single leading Newline() is the only blank.
if !strings.HasPrefix(out, "\n ") {
t.Errorf("show must open with a single blank line then the view, got %q", head)
}
if strings.HasPrefix(out, "\n\n") {
t.Errorf("show opens with a DOUBLE blank line: %q", head)
}
}

// TestRenderResources_ChartDefaultWhenEnvUnset: with no RESOURCE_* env, the
// ceiling reported is the chart default (cpu=2,memory=8Gi), not "unknown".
func TestRenderResources_ChartDefaultWhenEnvUnset(t *testing.T) {
Expand Down
19 changes: 4 additions & 15 deletions internal/ui/ui.go
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
// Package ui renders installer-style terminal output for the tracebloc
// CLI — colored step headers, ✔/⚠/· status lines, dim hints, and a
// branded banner — matching the look of the tracebloc/client one-line
// installer (scripts/lib/common.sh).
// CLI — colored step headers, ✔/⚠/· status lines, and dim hints
// matching the look of the tracebloc/client one-line installer
// (scripts/lib/common.sh).
//
// Everything goes through a Printer, constructed with New. A Printer
// colorizes only when its writer is a real terminal and NO_COLOR is
Expand DownExpand Up@@ -243,19 +243,8 @@ func (p *Printer) out(format string, a ...any) {
_, _ = fmt.Fprintf(p.w, format, a...)
}

// Banner prints the branded intro block: a bold-cyan title, a dim rule,
// and an optional subtitle. Mirrors common.sh print_banner.
func (p *Printer) Banner(title, subtitle string) {
p.out("\n %s\n", p.hue(title, toneHeading))
p.out(" %s\n", p.paint("────────────────────────────────────────", color.Faint))
if subtitle != "" {
p.out(" %s\n", subtitle)
}
p.out("\n")
}

// Para prints a normal-weight paragraph, each line indented to match
// Banner/Section bodies. It splits on embedded newlines so multi-line
// Section bodies. It splits on embedded newlines so multi-line
// prose keeps the indent. Use for explanatory prose — distinct from
// Hintf (dim one-liners) and Infof (· bullets).
func (p *Printer) Para(text string) {
Expand Down
3 changes: 1 addition & 2 deletions internal/ui/ui_test.go
Original file line numberDiff line numberDiff line change
Expand Up@@ -35,15 +35,14 @@ func TestNew_BufferDefaultsToPlain(t *testing.T) {
func TestWithColorFalse_OmitsANSI(t *testing.T) {
var buf bytes.Buffer
p := New(&buf, WithColor(false))
p.Banner("tracebloc", "declarative ingestion")
p.Step(1, 3, "Discover cluster")
p.Warnf("PVC is %s", "ReadWriteOnce")
p.Hintf("pass --namespace to override")

if strings.Contains(buf.String(), esc) {
t.Errorf("WithColor(false) still emitted ANSI: %q", buf.String())
}
for _, want := range []string{"tracebloc", "Step 1/3", "Discover cluster", "ReadWriteOnce"} {
for _, want := range []string{"Step 1/3", "Discover cluster", "ReadWriteOnce"} {
if !strings.Contains(buf.String(), want) {
t.Errorf("output missing %q: %q", want, buf.String())
}
Expand Down
Loading