cubemaster(cli): support positional template-id in tpl info/delete/redo#891
Conversation
The template subcommands (info, delete, redo) required --template-id as a flag, which contradicts the docker/kubectl convention where resource identifiers are positional arguments (e.g. 'docker rm <id>', 'kubectl delete pod <name>'). Extract a resolveTemplateID helper that checks --template-id first, then falls back to the first positional argument. All three commands now accept: cubemastercli tpl info <template-id> cubemastercli tpl delete <template-id> cubemastercli tpl redo <template-id> The --template-id flag still works unchanged (backward compatible, flag takes priority). The render command is intentionally untouched because its positional slot is already used for the file path argument. Assisted-by: Oh My Pi:zai/glm-5.2 Signed-off-by: sparkzky <sparkhhhhhhhhhh@outlook.com>
Update all 'tpl info' and 'tpl delete' examples to use the positional form (cubemastercli tpl info <template-id>) matching docker/kubectl conventions. 'tpl render' examples are left unchanged since render uses the positional slot for the file path argument. Updated in both EN and ZH docs: - guide/template-inspection-and-preview.md - guide/tutorials/template-from-image.md - zh/guide/tutorials/template-build-practice.md (quick reference) Added backward-compat notes mentioning --template-id equivalence. Assisted-by: Oh My Pi:zai/glm-5.2 Signed-off-by: sparkzky <sparkhhhhhhhhhh@outlook.com>
|
LGTM @fslongjin cc |
|
Please squash the commits according to the guidelines. |
Add TestResolveTemplateIDFromAllTemplateCommands to verify that
resolveTemplateID works correctly when called with contexts built
from each of the three template commands (info, delete, redo).
This guards against accidentally reverting one of the Actions back
to c.String("template-id"), which would silently drop positional
arg support since the Go compiler cannot catch the difference.
6 sub-tests cover: positional-only and flag-overrides-positional
scenarios for TemplateInfoCommand, TemplateDeleteCommand, and
TemplateRedoCommand.
Signed-off-by: jinlong <jinlong@tencent.com>
|
Thanks for your contribution~ |
Review: #891 — cubemaster(cli): support positional template-id in tpl info/delete/redoOverallA clean, well-scoped change that brings the Finding 1: Extra positional arguments silently ignored (moderate)
The second argument is swallowed without error. This can mask typos or confusion about the API. The current return type ( Option A — Return func resolveTemplateID(c *cli.Context) (string, error) {
if id := c.String("template-id"); id != "" {
return id, nil
}
switch n := c.NArg(); {
case n == 0:
return "", errors.New("template-id is required")
case n == 1:
return c.Args().First(), nil
default:
return "", fmt.Errorf("expected at most 1 positional argument, got %d", n)
}
}This also moves the "template-id is required" error to a single location (currently duplicated in every consumer). Option B (minimal) — At minimum, log a warning when Finding 2: Pre-existing — template ID not URL-encoded in query stringThe template ID is interpolated directly into the URL via url := fmt.Sprintf("http://%s/cube/template?template_id=%s", net.JoinHostPort(host, port), templateID)A template ID containing Minor: Documentation note format inconsistencyIn AssessmentThe PR is correct, well-tested, and improves the CLI UX meaningfully. The findings above are minor — none block merging. |
![cubesandboxbot[bot]](https://avatars.githubusercontent.com/u/20101770?s=60&v=4){kind=small}
| return id | ||
| } | ||
| if c.NArg() > 0 { | ||
| return c.Args().First() |
There was a problem hiding this comment.
c.Args().First() silently discards any additional positional arguments. A user typing cubemastercli tpl info tpl-abc extra-arg gets no feedback about the extra arg. Consider either:
Option A — Change signature to (string, error) and add an NArg() > 1 check.
Option B (minimal) — At minimum log a warning when extra args are detected.
| }, | ||
| Action: func(c *cli.Context) error { | ||
| templateID := c.String("template-id") | ||
| templateID := resolveTemplateID(c) |
There was a problem hiding this comment.
Pre-existing note (not blocking): templateID is interpolated directly via fmt.Sprintf("...?template_id=%s", ...) without URL encoding. A template ID containing &, =, or # could inject query parameters. Using net/url.Values would be more robust — the same pattern exists for jobID and buildID elsewhere in this file and would benefit from a separate cleanup pass.
3 participants
Motivation
The
cubemastercli tplsubcommands required--template-id <id>as a flag for every invocation. This contradicts the docker/kubectl convention where resource identifiers are positional arguments:This friction is a usability papercut — the codebase already had the correct pattern in
destroy(positional<sandbox-id>) andcreate/commit(flag-fallback-to-positional), but the template commands did not follow it.Changes
Code (
cubemaster:)resolveTemplateID(c)helper — checks--template-idflag first, falls back to the first positional arg (c.Args().First()), returns empty if neither is present.tpl info— accepts positional<template-id>, addedArgsUsage.tpl delete— accepts positional<template-id>, addedArgsUsage.tpl redo— accepts positional<template-id>, addedArgsUsage.tpl render— intentionally untouched. Its positional slot is already the file path, so--template-idis the correct interface there.Backward compatibility:
--template-idflag still works exactly as before and takes priority over the positional arg.Docs (
docs:)Updated all
tpl info/tpl deleteexamples in EN + ZH to positional syntax, with backward-compat notes.tpl renderexamples left unchanged.Tests
4 unit tests for
resolveTemplateID: flag-only, positional-only, flag-overrides-positional, empty.Verification
Unit tests:
go test -run TestResolveTemplateID ./cmd/cubemastercli/commands/cubebox/→ 4/4 PASS. Full package tests pass with no regressions.E2E on live PVM cluster (cubemaster on
0.0.0.0:8089, real templates):tpl info tpl-b394e8e5...(positional)tpl info --template-id tpl-b394e8e5...(flag)tpl info tpl-b394e8e5... --json --include-requestcreate_requesttpl info(no args)template-id is requiredtpl info tpl-doesnotexisttemplate not foundtpl redo tpl-b394e8e5... --detach(positional)job_idtpl delete/tpl redo(no args)template-id is required<template-id>in USAGEAssisted-by: Oh My Pi:zai/glm-5.2