diff --git a/rest-api/cli/pkg/commands.go b/rest-api/cli/pkg/commands.go index 15b291f104..3ec4fc8836 100644 --- a/rest-api/cli/pkg/commands.go +++ b/rest-api/cli/pkg/commands.go @@ -158,14 +158,19 @@ func buildTagSubcommands(spec *Spec, ops []resolvedOp) []*cli.Command { } // Resolve action-name collisions symmetrically: when two or more - // operations under the same tag collapse to the same short action (e.g. - // `get-current-infrastructure-provider` and - // `get-current-infrastructure-provider-stats` both -> `get`), expand ALL - // of them to their full OperationID. The previous "first one wins" pass - // produced a different command surface depending on the order of map + // operations under the same tag collapse to the same short action, expand + // ALL of them to their full OperationID. The previous "first one wins" + // pass produced a different command surface depending on the order of map // iteration in collectOperations, which depended on whether the user's // config file had been loaded -- the same binary exposed different // commands in the two states. + // + // The formerly motivating case -- `get-current-infrastructure-provider` + // and `get-current-infrastructure-provider-stats` both collapsing to + // `get` -- no longer collides: get-current-* singletons now map to + // distinct `current` / `stats` actions in operationAction. This guard + // stays for any future tag whose operations still collide on a short + // action. actionCounts := make(map[string]int) for _, op := range primaryOps { actionCounts[op.action]++ @@ -806,31 +811,44 @@ func operationAction(opID string) string { patterns := []struct { prefix string action string + // bare is the action for a getter prefix (action == "") when the + // operation has no -stats / -status-history suffix. A plain + // get- is the `get` action; a get-current- + // singleton getter is the `current` action so the generated command + // matches the REST /current path and the interactive TUI command name + // (e.g. `tenant current`, `infrastructure-provider current`). + bare string }{ - {"batch-create-", "batch-create"}, - {"batch-update-", "batch-update"}, - {"get-all-", "list"}, - {"get-current-", "get"}, - {"create-", "create"}, - {"update-", "update"}, - {"delete-", "delete"}, - {"get-", ""}, + {prefix: "batch-create-", action: "batch-create"}, + {prefix: "batch-update-", action: "batch-update"}, + {prefix: "get-all-", action: "list"}, + {prefix: "get-current-", bare: "current"}, + {prefix: "create-", action: "create"}, + {prefix: "update-", action: "update"}, + {prefix: "delete-", action: "delete"}, + {prefix: "get-", bare: "get"}, } for _, p := range patterns { - if strings.HasPrefix(opID, p.prefix) { - if p.action != "" { - return p.action - } - suffix := opID[len(p.prefix):] - if strings.HasSuffix(suffix, "-status-history") { - return "status-history" - } - if strings.HasSuffix(suffix, "-stats") { - return "stats" - } - return "get" + if !strings.HasPrefix(opID, p.prefix) { + continue + } + if p.action != "" { + return p.action + } + // Getter prefix: a -stats or -status-history endpoint collapses to + // its own action so a resource's getter and its stats / history + // endpoints stay distinct commands instead of colliding on one action + // (which would force both to expand to their full operationId). This + // applies to both get- and get-current-. + suffix := opID[len(p.prefix):] + if strings.HasSuffix(suffix, "-status-history") { + return "status-history" + } + if strings.HasSuffix(suffix, "-stats") { + return "stats" } + return p.bare } return opID diff --git a/rest-api/cli/pkg/commands_test.go b/rest-api/cli/pkg/commands_test.go index 36ec5ad428..440991af6e 100644 --- a/rest-api/cli/pkg/commands_test.go +++ b/rest-api/cli/pkg/commands_test.go @@ -98,9 +98,9 @@ func TestOperationAction(t *testing.T) { {"get-all-site", "list"}, {"get-all-instance", "list"}, {"get-all-allocation-constraint", "list"}, - {"get-current-infrastructure-provider", "get"}, - {"get-current-tenant", "get"}, - {"get-current-service-account", "get"}, + {"get-current-infrastructure-provider", "current"}, + {"get-current-tenant", "current"}, + {"get-current-service-account", "current"}, {"create-site", "create"}, {"create-allocation-constraint", "create"}, {"update-site", "update"}, @@ -110,9 +110,13 @@ func TestOperationAction(t *testing.T) { {"get-site-status-history", "status-history"}, {"get-instance-status-history", "status-history"}, {"get-machine-status-history", "status-history"}, - // get-current-* matches before -stats suffix check - {"get-current-infrastructure-provider-stats", "get"}, - {"get-current-tenant-stats", "get"}, + // get-current- singletons resolve to `current`; their + // -stats endpoints resolve to `stats` (distinct actions, no collision). + // service-account has no -stats endpoint in the spec today, but the + // suffix mapping is resource-independent, so it is asserted here too. + {"get-current-infrastructure-provider-stats", "stats"}, + {"get-current-tenant-stats", "stats"}, + {"get-current-service-account-stats", "stats"}, {"batch-create-instance", "batch-create"}, {"batch-create-expected-machines", "batch-create"}, {"batch-update-expected-machines", "batch-update"}, @@ -674,6 +678,12 @@ func TestIsActionModifier(t *testing.T) { // short alias non-deterministically points at one of the two operations // depending on map iteration order, so the same binary exposes a different // command surface depending on whether the config file was loaded. +// +// NOTE: this exercises the collision resolver with synthetic ops (action +// "get" injected on both). In the embedded spec these two operations now map +// to distinct `current` / `stats` actions, so this exact collision no longer +// arises there; the live singleton-surface guard is +// TestNewApp_CurrentSingletonCommandSurface. func TestBuildTagSubcommands_AliasCollisionExpandsAllNames(t *testing.T) { infraProviderGet := &Operation{ OperationID: "get-current-infrastructure-provider", @@ -704,14 +714,16 @@ func TestBuildTagSubcommands_AliasCollisionExpandsAllNames(t *testing.T) { // TestBuildTagSubcommands_NonCollidingActionKeepsShortAlias is the negative // counterpart to the collision test above: when there is exactly one -// operation per short action, the short alias is preserved. +// operation per short action, the short alias is preserved. Uses a plain +// get- op (a genuine `get` action) so the fixture is self-consistent +// -- get-current-* singletons now map to `current`, not `get`. func TestBuildTagSubcommands_NonCollidingActionKeepsShortAlias(t *testing.T) { op := &Operation{ - OperationID: "get-current-infrastructure-provider", - Tags: []string{"Infrastructure Provider"}, + OperationID: "get-site", + Tags: []string{"Site"}, } ops := []resolvedOp{ - {tag: "Infrastructure Provider", action: "get", method: "GET", path: "/p1", op: op}, + {tag: "Site", action: "get", method: "GET", path: "/p1", op: op}, } cmds := buildTagSubcommands(&Spec{}, ops) @@ -726,6 +738,9 @@ func TestBuildTagSubcommands_NonCollidingActionKeepsShortAlias(t *testing.T) { // running the resolver against both possible orderings of the colliding // operations. The resulting command tree must be identical, so the binary's // command surface no longer depends on Go map iteration order. +// +// NOTE: synthetic action "get" is injected on both ops to force the collision; +// in the embedded spec these resolve to distinct `current` / `stats` actions. func TestBuildTagSubcommands_AliasCollisionIsOrderIndependent(t *testing.T) { infraProviderGet := &Operation{ OperationID: "get-current-infrastructure-provider", @@ -761,27 +776,27 @@ func TestBuildTagSubcommands_AliasCollisionIsOrderIndependent(t *testing.T) { "command surface must not depend on the order primaryOps is built in") } -// TestNewApp_NoConfigDependentCommandSurface walks the embedded production -// spec and asserts that the two known alias-collision pairs (Infrastructure -// Provider and Tenant) expose the full operationId-derived names with no -// short `get` alias. This is the regression guard that fires if a future -// change re-introduces a "first one wins" alias resolver. -func TestNewApp_NoConfigDependentCommandSurface(t *testing.T) { +// TestNewApp_CurrentSingletonCommandSurface walks the embedded production spec +// and asserts that the get-current- singletons expose the `current` +// action (and `stats` for their -stats endpoints) instead of the bare `get` +// action or the full operationId. Because `current` and `stats` are distinct +// actions there is no get-action collision, so the command surface is +// deterministic and never depends on Go map iteration order. This is the +// regression guard for NVBug 6100988 (`tenant current` must be runnable from +// the command line, matching what the interactive TUI prints) and replaces the +// old guard that asserted the colliding pair expanded to full operationIds. +func TestNewApp_CurrentSingletonCommandSurface(t *testing.T) { app, err := NewApp(openapi.Spec) require.NoError(t, err, "NewApp failed") - collidingPairs := map[string][]string{ - "infrastructure-provider": { - "get-current-infrastructure-provider", - "get-current-infrastructure-provider-stats", - }, - "tenant": { - "get-current-tenant", - "get-current-tenant-stats", - }, + // tag -> actions that must be present, with no bare `get` and no + // full-operationId leftovers. + want := map[string][]string{ + "infrastructure-provider": {"current", "stats"}, + "tenant": {"current", "stats"}, } - for tag, expected := range collidingPairs { + for tag, actions := range want { t.Run(tag, func(t *testing.T) { var found *cli.Command for _, c := range app.Commands { @@ -796,17 +811,84 @@ func TestNewApp_NoConfigDependentCommandSurface(t *testing.T) { for _, sc := range found.Subcommands { subNames[sc.Name] = true } - for _, want := range expected { - assert.Truef(t, subNames[want], - "tag %q must expose %q as a deterministic, full-name command", tag, want) + for _, a := range actions { + assert.Truef(t, subNames[a], "tag %q must expose the %q command", tag, a) } assert.Falsef(t, subNames["get"], - "tag %q must NOT expose `get` as a short alias when there are %d colliding operations", - tag, len(expected)) + "tag %q must NOT expose a bare `get`; the singleton getter is `current`", tag) + assert.Falsef(t, subNames["get-current-"+tag], + "tag %q must NOT expose the full-operationId command; it collapses to `current`", tag) }) } } +// TestBuildCommands_CurrentSingletonsAreRunnable asserts that every +// get-current- singleton in the embedded spec is reachable from the +// non-interactive CLI under the `current` action that the interactive TUI +// prints (NVBug 6100988). Driven off the spec so it stays honest as singletons +// are added or removed. +func TestBuildCommands_CurrentSingletonsAreRunnable(t *testing.T) { + spec, err := ParseSpec(openapi.Spec) + require.NoError(t, err) + cmds := BuildCommands(spec) + + cmdByName := func(list []*cli.Command, name string) *cli.Command { + for _, c := range list { + if c.HasName(name) { + return c + } + } + return nil + } + + for _, tag := range []string{"tenant", "infrastructure-provider", "service-account"} { + t.Run(tag, func(t *testing.T) { + parent := cmdByName(cmds, tag) + require.NotNilf(t, parent, "tag %q must be a top-level command", tag) + assert.NotNilf(t, cmdByName(parent.Subcommands, "current"), + "tag %q must expose a `current` command runnable from the CLI", tag) + }) + } +} + +// TestBuildCommands_AllocationConstraintIsUpdateOnly is the CLI-side guard for +// NVBug 6232163: the server only registers PATCH for the AllocationConstraint +// sub-resource, and the stale create/get/list/delete endpoints were removed +// from the OpenAPI spec. Because the CLI is generated from the embedded spec, +// the `allocation constraint` sub-resource must therefore expose only `update`. +// This test fails if the removed endpoints are ever reintroduced into the spec. +func TestBuildCommands_AllocationConstraintIsUpdateOnly(t *testing.T) { + spec, err := ParseSpec(openapi.Spec) + require.NoError(t, err) + cmds := BuildCommands(spec) + + var allocation *cli.Command + for _, c := range cmds { + if c.Name == "allocation" { + allocation = c + break + } + } + require.NotNil(t, allocation, "allocation command must exist") + + var constraint *cli.Command + for _, sc := range allocation.Subcommands { + if sc.Name == "constraint" { + constraint = sc + break + } + } + require.NotNil(t, constraint, "allocation `constraint` sub-resource must exist") + + actions := make([]string, 0, len(constraint.Subcommands)) + for _, sc := range constraint.Subcommands { + actions = append(actions, sc.Name) + } + assert.Equal(t, []string{"update"}, actions, + "allocation constraint must expose only `update`; create/get/list/delete were "+ + "removed from the OpenAPI spec because the server never registered those routes (NVBug 6232163)") +} + // sortStrings is a tiny stable sort used by the order-independence test so it // stays self-contained and does not pull in sort.Strings (which is already // used elsewhere; this just keeps the test readable). diff --git a/rest-api/cli/tui/commands.go b/rest-api/cli/tui/commands.go index 0de6014108..7f6642e6bc 100644 --- a/rest-api/cli/tui/commands.go +++ b/rest-api/cli/tui/commands.go @@ -2217,18 +2217,20 @@ func cmdVPCPrefixCreate(s *Session, _ []string) error { if prefixLen < 8 || prefixLen > 31 { return fmt.Errorf("prefix length must be between 8 and 31") } - ipBlockID, err := PromptText("IP block ID", true) + ipBlockID, err := promptVPCPrefixIPBlockID(s, context.Background()) if err != nil { return err } + // ipBlockID is already trimmed by promptVPCPrefixIPBlockID (picker IDs are + // clean; the manual-entry path trims), so no extra TrimSpace here. body := map[string]interface{}{ "name": name, "vpcId": vpc.ID, - "ipBlockId": strings.TrimSpace(ipBlockID), + "ipBlockId": ipBlockID, "prefixLength": prefixLen, } - LogCmd(s, "vpc-prefix", "create", "--name", name, "--vpc-id", vpc.ID, "--ip-block-id", strings.TrimSpace(ipBlockID), "--prefix-length", prefixLenText) + LogCmd(s, "vpc-prefix", "create", "--name", name, "--vpc-id", vpc.ID, "--ip-block-id", ipBlockID, "--prefix-length", prefixLenText) bodyJSON, _ := json.Marshal(body) resp, _, err := s.Client.Do("POST", apiPath(s, "vpc-prefix"), nil, nil, bodyJSON) if err != nil { @@ -2242,6 +2244,72 @@ func cmdVPCPrefixCreate(s *Session, _ []string) error { return nil } +// ipBlockManualEntrySentinel is the select ID used for the trailing "enter +// manually" option in the IP block picker, mirroring tenantManualEntrySentinel. +const ipBlockManualEntrySentinel = "__manual__" + +// promptVPCPrefixIPBlockID picks the IP block for a new VPC prefix. ipBlockId +// is required by the API (APIVpcPrefixCreateRequest.Validate), so rather than +// make the operator paste a raw UUID, list the IP blocks already scoped to the +// VPC's site and let them choose one. Falls back to manual entry when no IP +// blocks are visible, when listing fails, or when the operator opts out via +// the trailing sentinel (NVBug 6105076). +func promptVPCPrefixIPBlockID(s *Session, ctx context.Context) (string, error) { + blocks, err := s.Resolver.Fetch(ctx, "ip-block") + if err != nil { + fmt.Fprintf(os.Stderr, "%s could not list IP blocks (%v); falling back to manual entry\n", Dim("note:"), err) + return promptIPBlockIDRaw() + } + items := buildIPBlockSelectItems(blocks) + if len(items) == 1 { + // Only the manual-entry sentinel: no IP blocks for this site. + fmt.Fprintf(os.Stderr, "%s no IP blocks found for this site; enter an IP block ID manually\n", Dim("note:")) + return promptIPBlockIDRaw() + } + selected, err := Select("IP block:", items) + if err != nil { + return "", err + } + if selected.ID == ipBlockManualEntrySentinel { + return promptIPBlockIDRaw() + } + return selected.ID, nil +} + +func promptIPBlockIDRaw() (string, error) { + raw, err := PromptText("IP block ID", true) + if err != nil { + return "", err + } + return strings.TrimSpace(raw), nil +} + +// buildIPBlockSelectItems turns the resolver's IP block list into picker +// options whose ID is the IP block UUID and whose label surfaces the block +// name (falling back to the UUID when unnamed) plus status. A trailing +// manual-entry sentinel is always appended -- even for an empty list -- so the +// operator can still type a raw UUID for a block that isn't listed in the +// current scope. Blocks without an ID are skipped. +func buildIPBlockSelectItems(blocks []NamedItem) []SelectItem { + items := make([]SelectItem, 0, len(blocks)+1) + for _, b := range blocks { + id := strings.TrimSpace(b.ID) + if id == "" { + continue + } + label := strings.TrimSpace(b.Name) + if label == "" { + label = id + } + if strings.TrimSpace(b.Status) != "" { + label += " " + Dim(b.Status) + } + items = append(items, SelectItem{Label: label, ID: id}) + } + items = append(items, SelectItem{Label: "Enter IP block ID manually...", ID: ipBlockManualEntrySentinel}) + return items +} + func cmdVPCPrefixUpdate(s *Session, args []string) error { item, err := s.Resolver.ResolveWithArgs(context.Background(), "vpc-prefix", "VPC Prefix to update", args) if err != nil { diff --git a/rest-api/cli/tui/commands_test.go b/rest-api/cli/tui/commands_test.go index aa493c0153..76807d933c 100644 --- a/rest-api/cli/tui/commands_test.go +++ b/rest-api/cli/tui/commands_test.go @@ -1538,3 +1538,41 @@ func TestAllocationConstraintValueHint(t *testing.T) { assert.Contains(t, allocationConstraintValueHint("InstanceType"), "machine") assert.NotEmpty(t, allocationConstraintValueHint("Unknown"), "unknown types still get a generic hint") } + +// --- VPC prefix create IP block picker tests (NVBug 6105076) --- + +func TestBuildIPBlockSelectItems_MapsBlocksAndAppendsManualSentinel(t *testing.T) { + blocks := []NamedItem{ + {Name: "block-a", ID: "id-a", Status: "Ready"}, + {Name: "block-b", ID: "id-b"}, + } + + items := buildIPBlockSelectItems(blocks) + + require.Len(t, items, 3, "two IP blocks plus the manual-entry sentinel") + assert.Equal(t, "id-a", items[0].ID, "select ID must be the IP block UUID") + assert.Contains(t, items[0].Label, "block-a") + assert.Contains(t, items[0].Label, "Ready", "status should be surfaced in the label") + assert.Equal(t, "id-b", items[1].ID) + assert.Equal(t, ipBlockManualEntrySentinel, items[2].ID) +} + +func TestBuildIPBlockSelectItems_EmptyListReturnsOnlySentinel(t *testing.T) { + items := buildIPBlockSelectItems(nil) + require.Len(t, items, 1, "an empty list still offers manual entry") + assert.Equal(t, ipBlockManualEntrySentinel, items[0].ID) +} + +func TestBuildIPBlockSelectItems_SkipsBlocksWithoutIDAndFallsBackLabelToID(t *testing.T) { + blocks := []NamedItem{ + {Name: "no-id", ID: " "}, + {Name: " ", ID: "id-x"}, + } + + items := buildIPBlockSelectItems(blocks) + + require.Len(t, items, 2, "one usable block (id-x) plus the manual-entry sentinel") + assert.Equal(t, "id-x", items[0].ID) + assert.Equal(t, "id-x", items[0].Label, "blank name must fall back to the ID") + assert.Equal(t, ipBlockManualEntrySentinel, items[1].ID) +}