fix(data): deprecation notices on renamed aliases + clear no-client message (#879)

internal/cli/clustertarget.go

@@ -24,6 +24,17 @@ type noParentReleaseError struct{ err error }
 func (e *noParentReleaseError) Error() string { return e.err.Error() }
 func (e *noParentReleaseError) Unwrap() error { return e.err }
 
+// loadClusterFn / newClientsetFn are the kubeconfig-load + clientset-build
+// seams resolveClusterTarget goes through. Production points them at the real
+// cluster helpers; tests inject a fake ResolvedConfig + fake.NewSimpleClientset
+// so the discovery + exit-code contract can be exercised without a real
+// kubeconfig or apiserver. Same fn-var seam pattern used across this package
+// (listDatasetsFn, mintIngestorTokenFn, …).
+var (
+	loadClusterFn  = cluster.Load
+	newClientsetFn = cluster.NewClientset
+)
+
 // clusterTarget bundles the cluster handles the data commands resolve from a
 // kubeconfig before doing any work: the resolved config, a clientset, the
 // parent tracebloc release, and — when asked — the shared data PVC.
@@ -45,11 +56,11 @@ type clusterTarget struct {
 // contract (2/3 escalation, with discovery reported as a check Result rather
 // than a hard error).
 func resolveClusterTarget(ctx context.Context, p *ui.Printer, opts cluster.KubeconfigOptions, b activeClientBinding, needPVC bool) (*clusterTarget, error) {
-	resolved, err := cluster.Load(opts)
+	resolved, err := loadClusterFn(opts)
 	if err != nil {
 		return nil, &exitError{code: 3, err: fmt.Errorf("loading kubeconfig: %w", err)}
 	}
-	cs, err := cluster.NewClientset(resolved)
+	cs, err := newClientsetFn(resolved)
 	if err != nil {
 		return nil, &exitError{code: 3, err: err}
 	}
@@ -95,9 +106,26 @@ func discoverRelease(ctx context.Context, p *ui.Printer, cs kubernetes.Interface
 		return release, namespace, err
 	}
 	found, scanErr := cluster.FindClientNamespaces(ctx, cs)
-	if scanErr != nil || len(found) == 0 {
+	if scanErr != nil {
+		// The scan itself couldn't run (e.g. RBAC forbids the cluster-wide
+		// list — a different problem than "not provisioned"). Keep the
+		// original per-namespace discovery error rather than claiming the
+		// machine has no client.
 		return nil, namespace, err
 	}
+	if len(found) == 0 {
+		// The scan SUCCEEDED and turned up nothing: the cluster the kubeconfig
+		// reaches genuinely hosts no tracebloc client. Return a §7.10
+		// "this machine isn't provisioned" message rather than the bare
+		// per-namespace miss, which read like a namespace hunt. Still wraps
+		// ErrNoParentRelease so errors.Is stays true and resolveClusterTarget
+		// keeps mapping it to noParentReleaseError / exit 4.
+		return nil, namespace, fmt.Errorf(
+			"%w on the cluster your kubeconfig points at — if this machine should "+
+				"have one, run the installer to provision it; otherwise point at the "+
+				"right cluster with --context/--namespace",
+			cluster.ErrNoParentRelease)
+	}
 	if len(found) > 1 {
 		return nil, namespace, fmt.Errorf(
 			"%w in namespace %q, but tracebloc clients are running in: %s. "+

internal/cli/clustertarget_test.go

@@ -9,14 +9,83 @@ import (
 
 	appsv1 "k8s.io/api/apps/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/client-go/kubernetes"
 	"k8s.io/client-go/kubernetes/fake"
+	ktesting "k8s.io/client-go/testing"
 
 	"github.com/tracebloc/cli/internal/api"
 	"github.com/tracebloc/cli/internal/cluster"
 	"github.com/tracebloc/cli/internal/config"
 	"github.com/tracebloc/cli/internal/ui"
 )
 
+// withClusterSeams points resolveClusterTarget's kubeconfig-load + clientset
+// seams at a fixed ResolvedConfig (namespace "default") and the given fake
+// clientset, so the discovery + exit-code contract can be exercised without a
+// real kubeconfig or apiserver. It restores the originals on cleanup.
+func withClusterSeams(t *testing.T, cs kubernetes.Interface) {
+	t.Helper()
+	origLoad, origCS := loadClusterFn, newClientsetFn
+	t.Cleanup(func() { loadClusterFn, newClientsetFn = origLoad, origCS })
+	loadClusterFn = func(cluster.KubeconfigOptions) (*cluster.ResolvedConfig, error) {
+		return &cluster.ResolvedConfig{Namespace: "default", Context: "test-ctx"}, nil
+	}
+	newClientsetFn = func(*cluster.ResolvedConfig) (kubernetes.Interface, error) { return cs, nil }
+}
+
+// A reached cluster that hosts no tracebloc client anywhere must surface the
+// §7.10 "this machine isn't provisioned" guidance at exit 4, still
+// errors.Is-identifiable as ErrNoParentRelease and mapped to *noParentReleaseError.
+func TestResolveClusterTarget_NoClient_InstallerMessageExit4(t *testing.T) {
+	withClusterSeams(t, fake.NewSimpleClientset()) // empty cluster
+	_, err := resolveClusterTarget(context.Background(), nil,
+		cluster.KubeconfigOptions{}, activeClientBinding{}, true)
+	if err == nil {
+		t.Fatal("expected an error when the cluster hosts no client")
+	}
+	if got := ExitCodeFromError(err); got != 4 {
+		t.Fatalf("exit code = %d, want 4", got)
+	}
+	if !errors.Is(err, cluster.ErrNoParentRelease) {
+		t.Errorf("error should stay errors.Is(ErrNoParentRelease): %v", err)
+	}
+	var npr *noParentReleaseError
+	if !errors.As(err, &npr) {
+		t.Errorf("no-client miss should map to *noParentReleaseError, got %T", err)
+	}
+	for _, want := range []string{"run the installer", "--context/--namespace", "kubeconfig points at"} {
+		if !strings.Contains(err.Error(), want) {
+			t.Errorf("message missing %q:\n%s", want, err.Error())
+		}
+	}
+}
+
+// Regression: the >1 branch is untouched — several clients still get the
+// "pick one with --namespace" message (exit 4), NOT the installer rewrite.
+func TestResolveClusterTarget_MultipleClients_PickOneExit4(t *testing.T) {
+	withClusterSeams(t, fake.NewSimpleClientset(jmDep("alpha"), jmDep("beta")))
+	_, err := resolveClusterTarget(context.Background(), nil,
+		cluster.KubeconfigOptions{}, activeClientBinding{}, true)
+	if err == nil {
+		t.Fatal("expected an error when multiple clients are present")
+	}
+	if got := ExitCodeFromError(err); got != 4 {
+		t.Fatalf("exit code = %d, want 4", got)
+	}
+	if !errors.Is(err, cluster.ErrNoParentRelease) {
+		t.Errorf("expected ErrNoParentRelease, got: %v", err)
+	}
+	for _, want := range []string{"alpha", "beta", "--namespace"} {
+		if !strings.Contains(err.Error(), want) {
+			t.Errorf("pick-one message missing %q:\n%s", want, err.Error())
+		}
+	}
+	if strings.Contains(err.Error(), "run the installer") {
+		t.Errorf("multi-client branch must not be rewritten to the installer message:\n%s", err.Error())
+	}
+}
+
 func TestSetActiveClient_CachesNamespaceAndName(t *testing.T) {
 	p := &config.Profile{}
 	setActiveClient(p, &api.ProvisionedClient{ID: 7, Name: "Lab A", Namespace: "lab-a", Location: "FR"})
@@ -192,21 +261,64 @@ func TestDiscoverRelease_NoScanWhenExplicit(t *testing.T) {
 	}
 }
 
-func TestDiscoverRelease_ScanFindsNothingKeepsOriginalError(t *testing.T) {
-	cs := fake.NewSimpleClientset() // empty cluster
+// A successful scan that finds NO client anywhere means this machine isn't
+// provisioned — the error must say so (§7.10: run the installer / point
+// elsewhere) rather than read like a namespace hunt, while still wrapping
+// ErrNoParentRelease so the exit-4 mapping holds.
+func TestDiscoverRelease_ScanFindsNothing_InstallerGuidance(t *testing.T) {
+	cs := fake.NewSimpleClientset() // empty cluster — scan succeeds, finds nothing
 	_, _, err := discoverRelease(context.Background(), nil, cs, "default", true)
 	if err == nil {
 		t.Fatal("expected an error on an empty cluster")
 	}
 	if !errors.Is(err, cluster.ErrNoParentRelease) {
 		t.Errorf("expected ErrNoParentRelease, got: %v", err)
 	}
+	for _, want := range []string{
+		"on the cluster your kubeconfig points at",
+		"run the installer",
+		"--context/--namespace",
+	} {
+		if !strings.Contains(err.Error(), want) {
+			t.Errorf("message missing %q, got: %s", want, err.Error())
+		}
+	}
 	// The no-client error must stay customer-actionable without Helm.
 	if strings.Contains(err.Error(), "helm") {
 		t.Errorf("error must not tell customers to run helm: %s", err)
 	}
 }
 
+// When the cluster-wide scan itself can't run (e.g. RBAC forbids a cluster-scope
+// list), that's a DIFFERENT problem than "not provisioned" — keep the original
+// per-namespace discovery error rather than the "run the installer" rewrite.
+func TestDiscoverRelease_ScanUnavailable_KeepsOriginalError(t *testing.T) {
+	cs := fake.NewSimpleClientset() // no client in "default" → ErrNoParentRelease there
+	// Fail only the cluster-wide (NamespaceAll) list the scan issues; the
+	// namespaced discovery list in "default" still succeeds.
+	cs.PrependReactor("list", "deployments", func(action ktesting.Action) (bool, runtime.Object, error) {
+		if action.GetNamespace() == metav1.NamespaceAll {
+			return true, nil, errors.New("forbidden: cannot list deployments at the cluster scope")
+		}
+		return false, nil, nil
+	})
+	_, _, err := discoverRelease(context.Background(), nil, cs, "default", true)
+	if err == nil {
+		t.Fatal("expected an error when the scan can't run")
+	}
+	if !errors.Is(err, cluster.ErrNoParentRelease) {
+		t.Errorf("expected ErrNoParentRelease, got: %v", err)
+	}
+	// It must stay the ORIGINAL namespace-scoped discovery error, NOT the
+	// machine-not-provisioned rewrite (which only fires on a clean empty scan).
+	if !strings.Contains(err.Error(), `namespace "default"`) {
+		t.Errorf("scan-unavailable case should keep the namespaced discovery error, got: %s", err)
+	}
+	if strings.Contains(err.Error(), "on the cluster your kubeconfig points at") {
+		t.Errorf("scan-unavailable must NOT be rewritten to the installer message, got: %s", err)
+	}
+}
+
 // The §7.5 contract at the caller level: the scan may engage ONLY when nobody
 // chose the namespace — never for an explicit flag, never for a binding miss
 // (which could silently retarget a different machine's client).

internal/cli/data.go

@@ -57,13 +57,51 @@ before the first ingest.`,
 		RunE:                       runGroup,
 		SuggestionsMinimumDistance: 2,
 	}
+	// Deprecation notices (#879): the data verb was renamed (dataset→data,
+	// push→ingest, rm→delete). The old spellings still work as aliases for one
+	// cycle, but an aliased invocation warns once on stderr. root.go has no
+	// PersistentPreRunE, so this — the closest hook for any `data <sub>` — is
+	// where we detect and warn; cobra passes the executed (leaf) command in.
+	cmd.PersistentPreRunE = func(leaf *cobra.Command, _ []string) error {
+		warnDeprecatedAlias(leaf, leaf.ErrOrStderr())
+		return nil
+	}
 	cmd.AddCommand(newDataIngestCmd())
 	cmd.AddCommand(newDataListCmd())
 	cmd.AddCommand(newDataDeleteCmd())
 	cmd.AddCommand(newIngestValidateCmd())
 	return cmd
 }
 
+// deprecatedAliasCanonical maps each deprecated command alias to the canonical
+// invocation to steer the user to. Keyed by the alias token; the value is the
+// full canonical form so the notice nudges the whole rename (e.g. a `push`
+// steers to `data ingest`, covering the dataset→data group rename too).
+var deprecatedAliasCanonical = map[string]string{
+	"dataset": "data",
+	"push":    "data ingest",
+	"rm":      "data delete",
+}
+
+// warnDeprecatedAlias prints a one-line deprecation notice to w when the executed
+// command was invoked through a deprecated alias. It reads cobra's exported
+// Command.CalledAs(), which reliably reports the alias for the EXECUTED command —
+// so `data push` / `dataset push` warn (leaf `ingest` called as `push`), `data
+// rm` / `dataset rm` warn, and a bare `dataset` warns (the `data` group has a
+// RunE, so it is the executed command). We intentionally do NOT chase a parent
+// group's alias for `dataset <canonical-verb>` (cobra doesn't expose an
+// ancestor's invoked-as name without reaching into its internals); the verb
+// notices already point at the full `data <verb>` form, which nudges the group
+// rename. Canonical invocations warn for nothing.
+func warnDeprecatedAlias(cmd *cobra.Command, w io.Writer) {
+	invoked := cmd.CalledAs()
+	if canonical, ok := deprecatedAliasCanonical[invoked]; ok {
+		_, _ = fmt.Fprintf(w,
+			"%q is deprecated and will be removed in a future release — use %q instead.\n",
+			invoked, canonical)
+	}
+}
+
 // newDataIngestCmd implements `tracebloc data ingest <dataset>`.
 //
 // Phase 3 scope (now complete across PR-a + PR-b):

internal/cli/data_deprecation_test.go

@@ -0,0 +1,108 @@
+package cli
+
+import (
+	"bytes"
+	"fmt"
+	"strings"
+	"testing"
+)
+
+// TestDataDeprecationNotices_AliasPaths is the source-of-truth table test for
+// the #879 deprecation notices. It drives the REAL root command (so the
+// PersistentPreRunE on `data` fires exactly as in production) for every
+// alias/canonical path, captures stderr, and asserts the right notice appears —
+// and NONE on canonical invocations.
+//
+// The commands themselves fail fast after the pre-run (a nonexistent dataset
+// path for ingest, a bad kubeconfig for delete), so no network / cluster work
+// happens; the notice is printed by the pre-run BEFORE that failure.
+//
+// Detection uses cobra's exported Command.CalledAs() on the EXECUTED command, so
+// it warns for the deprecated verbs `push`/`rm` (the leaf) and for a bare
+// `dataset` (the `data` group is itself the executed command via its RunE). It
+// intentionally does NOT warn `dataset <canonical-verb>` (e.g. `dataset ingest`)
+// — cobra doesn't expose an ancestor's invoked-as name without reaching into its
+// internals, and the verb notices already point at the full `data <verb>` form,
+// which nudges the group rename. That accepted gap is pinned below.
+func TestDataDeprecationNotices_AliasPaths(t *testing.T) {
+	const notice = "is deprecated and will be removed"
+	badKC := "--kubeconfig=/tmp/tracebloc-cli-dep-nonexistent-" + t.Name()
+	noPath := "/tmp/tracebloc-cli-dep-no-such-dir-" + t.Name()
+
+	cases := []struct {
+		name string
+		args []string
+		// want maps each deprecated alias token expected to be warned to its
+		// canonical replacement. Empty = no notice.
+		want map[string]string
+	}{
+		{
+			name: "bare dataset — group alias warns",
+			args: []string{"dataset"},
+			want: map[string]string{"dataset": "data"},
+		},
+		{
+			name: "data push — verb alias warns (full canonical form)",
+			args: []string{"data", "push", noPath, badKC},
+			want: map[string]string{"push": "data ingest"},
+		},
+		{
+			name: "dataset push — verb alias warns (group nudge folded into it)",
+			args: []string{"dataset", "push", noPath, badKC},
+			want: map[string]string{"push": "data ingest"},
+		},
+		{
+			name: "data rm — verb alias warns",
+			args: []string{"data", "rm", "sometable", "--yes", badKC},
+			want: map[string]string{"rm": "data delete"},
+		},
+		{
+			name: "dataset rm — verb alias warns",
+			args: []string{"dataset", "rm", "sometable", "--yes", badKC},
+			want: map[string]string{"rm": "data delete"},
+		},
+		{
+			name: "dataset ingest — group alias + canonical verb: accepted gap, no notice",
+			args: []string{"dataset", "ingest", noPath, badKC},
+			want: nil,
+		},
+		{
+			name: "data ingest — canonical, no notice",
+			args: []string{"data", "ingest", noPath, badKC},
+			want: nil,
+		},
+		{
+			name: "data delete — canonical, no notice",
+			args: []string{"data", "delete", "sometable", "--yes", badKC},
+			want: nil,
+		},
+	}
+
+	for _, c := range cases {
+		t.Run(c.name, func(t *testing.T) {
+			root := NewRootCmd(BuildInfo{Version: "test"})
+			var so, se bytes.Buffer
+			root.SetOut(&so)
+			root.SetErr(&se)
+			root.SetArgs(c.args)
+			// The command may error (bad path / kubeconfig) or print help; the
+			// pre-run notice fires first, which is all we assert on here.
+			_ = root.Execute()
+
+			stderr := se.String()
+			if got := strings.Count(stderr, notice); got != len(c.want) {
+				t.Fatalf("notice count = %d, want %d\nstderr:\n%s", got, len(c.want), stderr)
+			}
+			for alias, canonical := range c.want {
+				line := fmt.Sprintf("%q is deprecated and will be removed in a future release — use %q instead.", alias, canonical)
+				if !strings.Contains(stderr, line) {
+					t.Errorf("stderr missing exact notice %q\nstderr:\n%s", line, stderr)
+				}
+			}
+			// A notice must never leak to stdout — scripts parse stdout.
+			if strings.Contains(so.String(), notice) {
+				t.Errorf("deprecation notice leaked to stdout:\n%s", so.String())
+			}
+		})
+	}
+}