From 46d62e4546261eca2b5caf893938c2336cc2c88e Mon Sep 17 00:00:00 2001 From: Lukas Wuttke Date: Fri, 10 Jul 2026 21:38:49 +0200 Subject: [PATCH] fix(data): deprecation notices on renamed aliases + clear no-client message (#879) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two remaining acceptance items for the RFC-0001 data-verb rename (backend#879); the rename + active-client binding already shipped via cli#128. - Deprecation notices: data's PersistentPreRunE warns once on stderr when a command is invoked via a deprecated alias — push→"data ingest", rm→"data delete", bare dataset→"data". Detection uses cobra's reliable Command.CalledAs() on the executed command (no reflection); the verb notices point at the full canonical form so they nudge the group rename too. The already-migrated `dataset ` case is intentionally unwarned (documented + pinned in the table test). - Clear no-client message: when the cluster-wide scan finds NO tracebloc client (one-machine-one-client, §7.10 — machine not provisioned), discoverRelease now returns a "run the installer / point at the right cluster with --context" hint instead of the bare namespace error. Still wraps ErrNoParentRelease (exit 4); the scanErr, >1 "pick one", and active-client-elsewhere paths are untouched. Tests: alias-path table test (every alias + canonical + the accepted gap); no-client exit-4 + scan-unavailable + multi-client regression. build/vet/gofmt clean; full suite green. Refs tracebloc/backend#879. Co-Authored-By: Claude Opus 4.8 --- internal/cli/clustertarget.go | 34 +++++++- internal/cli/clustertarget_test.go | 116 +++++++++++++++++++++++++- internal/cli/data.go | 38 +++++++++ internal/cli/data_deprecation_test.go | 108 ++++++++++++++++++++++++ 4 files changed, 291 insertions(+), 5 deletions(-) create mode 100644 internal/cli/data_deprecation_test.go diff --git a/internal/cli/clustertarget.go b/internal/cli/clustertarget.go index 49fb1c5..2ebe2f8 100644 --- a/internal/cli/clustertarget.go +++ b/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. "+ diff --git a/internal/cli/clustertarget_test.go b/internal/cli/clustertarget_test.go index a044526..1887818 100644 --- a/internal/cli/clustertarget_test.go +++ b/internal/cli/clustertarget_test.go @@ -9,7 +9,10 @@ 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" @@ -17,6 +20,72 @@ import ( "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,8 +261,12 @@ 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") @@ -201,12 +274,51 @@ func TestDiscoverRelease_ScanFindsNothingKeepsOriginalError(t *testing.T) { 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). diff --git a/internal/cli/data.go b/internal/cli/data.go index f857aba..22c1ac3 100644 --- a/internal/cli/data.go +++ b/internal/cli/data.go @@ -57,6 +57,15 @@ 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 ` — 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()) @@ -64,6 +73,35 @@ before the first ingest.`, 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 ` (cobra doesn't expose an +// ancestor's invoked-as name without reaching into its internals); the verb +// notices already point at the full `data ` 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 `. // // Phase 3 scope (now complete across PR-a + PR-b): diff --git a/internal/cli/data_deprecation_test.go b/internal/cli/data_deprecation_test.go new file mode 100644 index 0000000..5156078 --- /dev/null +++ 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 ` (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 ` 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()) + } + }) + } +}