Skip to content

Commit dfa9753

Browse files
authored
refactor: architecture hardening + template edit command (#34)
Correctness fixes: Surface SSH key resolution warnings, fix os.Stderr bypass, fix context.Background() in pricing cache, fix hostname validation message, reject trailing hyphens in template names, add JSON tags to Entry, fix duplicate {random} expansion Structural refactors: Extract WithSpinner helper (removes 11 boilerplate sites), extract shared fetchInstances, extract TemplatesBaseDir, split wizard.go (1355→776 lines), document createOptions lifecycle, parallelize volume fetching, atomic template writes Test infrastructure: httptest-based mock API harness, runCreate orchestration tests, action tests, template command tests Template UX: New template edit command with field menu, interactive picker for show/delete, show displays all fields
1 parent be03cc3 commit dfa9753

35 files changed

Lines changed: 2203 additions & 929 deletions

.ai/skills/update-command-knowledge.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ This skill generates and updates per-subcommand knowledge docs (README.md + CLAU
1010
## Step 1: Determine which command directories to process
1111

1212
If the argument contains `--all`, process ALL command directories:
13-
- auth, vm, sshkey, startupscript, volume, settings, version, update
13+
- auth, vm, template, sshkey, startupscript, volume, settings, version, update, status, ssh, cost, images, instancetypes, locations, availability, completion, mcp
1414

1515
Otherwise, detect changed dirs from staged files:
1616

internal/verda-cli/cmd/template/CLAUDE.md

Lines changed: 58 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -2,12 +2,13 @@
22

33
## Quick Reference
44
- Parent: `verda template` (alias: `tmpl`)
5-
- Subcommands: `create`, `list` (alias `ls`), `show`, `delete` (alias `rm`)
5+
- Subcommands: `create`, `edit`, `list` (alias `ls`), `show`, `delete` (alias `rm`)
66
- Files:
77
- `template.go` -- Parent command, registers subcommands
88
- `create.go` -- Create command, name validation, runs VM wizard in template mode
9+
- `edit.go` -- Edit command, field menu with API-backed editors, `matchKind`
910
- `list.go` -- List command with `--type` filter and structured output
10-
- `show.go` -- Show command with field display and structured output
11+
- `show.go` -- Show command with full field display, `pickTemplateEntry`, `parseRef`
1112
- `delete.go` -- Delete command with confirmation prompt
1213
- `types.go` -- Re-exports types/functions from shared `internal/verda-cli/template/`
1314

@@ -21,10 +22,10 @@ All fields except `resource` are optional. Stored at `~/.verda/templates/<resour
2122
| `resource` | string | Resource type, currently only `"vm"` |
2223
| `billing_type` | string | `"on-demand"` or `"spot"` |
2324
| `contract` | string | `"PAY_AS_YOU_GO"`, `"SPOT"`, `"LONG_TERM"` |
24-
| `kind` | string | `"GPU"` or `"CPU"` |
25+
| `kind` | string | `"GPU"` or `"CPU"` (lowercase in template, case-insensitive in matching) |
2526
| `instance_type` | string | e.g. `"1V100.6V"` |
2627
| `location` | string | e.g. `"FIN-01"` |
27-
| `image` | string | OS image slug |
28+
| `image` | string | OS image slug or ID |
2829
| `os_volume_size` | int | GiB |
2930
| `storage` | []StorageSpec | Each has `type` and `size` |
3031
| `storage_skip` | bool | Skip storage step in wizard |
@@ -34,7 +35,7 @@ All fields except `resource` are optional. Stored at `~/.verda/templates/<resour
3435
| `hostname_pattern` | string | Pattern with `{random}` and `{location}` placeholders |
3536

3637
### Name Validation and Auto-Reformatting
37-
- Valid names match `^[a-z0-9][a-z0-9-]*$`
38+
- Valid names match `^[a-z0-9]([a-z0-9-]*[a-z0-9])?$` (no trailing hyphens)
3839
- `normalizeName()` auto-formats: lowercase, replace spaces/underscores with hyphens, strip invalid chars, collapse consecutive hyphens, trim leading/trailing hyphens
3940
- Create command re-prompts on invalid names and on name collisions with existing templates
4041

@@ -52,33 +53,78 @@ All fields except `resource` are optional. Stored at `~/.verda/templates/<resour
5253

5354
### Hostname Pattern Expansion
5455
- `{random}` -> 3 petname words joined by hyphens (via `github.com/dustinkirkland/golang-petname`)
56+
- Each `{random}` occurrence generates different words (uses `strings.Replace` with count=1 in a loop)
5557
- `{location}` -> lowercased location code (e.g. `"FIN-01"` -> `"fin-01"`)
5658
- Only expanded when `hostname_pattern` is set AND `opts.Hostname` is empty (no `--hostname` flag)
5759

5860
### SSH Keys and Startup Scripts
5961
- Stored in template by **name**, not ID
6062
- Resolved to IDs at `vm create --from` time via `resolveSSHKeyNames()` and `resolveStartupScriptName()`
61-
- On API error or name not found, produces a warning and the wizard prompts later
63+
- On API error or name not found, produces a warning to `ioStreams.ErrOut` (no longer silently swallowed)
6264
- Names are stored in `opts.sshKeyNames` / `opts.startupScriptName` for template-saving round-trip
6365

66+
## Edit Command
67+
68+
The `template edit` command uses a field menu approach (not the full wizard):
69+
70+
1. Load existing template, display "Editing template: resource/name"
71+
2. Show menu of all editable fields with current values
72+
3. User picks a field to change
73+
4. Run appropriate editor for that field:
74+
- **Simple fields** (billing type, kind, OS volume size, hostname pattern): inline prompts
75+
- **API-backed fields** (instance type, location, image, SSH keys, startup script): fetch choices from API with spinner, show select/multi-select
76+
5. Return to menu — repeat until "Save & exit"
77+
6. Save template to disk (atomic write)
78+
79+
### Edit field editors
80+
- **Billing Type**: static select (on-demand / spot). Clears contract when switching to spot.
81+
- **Kind**: static select (gpu / cpu)
82+
- **Instance Type**: API call to `InstanceTypes.Get`, filtered by current kind, shows price
83+
- **Location**: API call to `Locations.Get`
84+
- **Image**: API call to `Images.Get`, excludes cluster images
85+
- **OS Volume Size**: text input with current value as default
86+
- **SSH Keys**: API call to `SSHKeys.GetAllSSHKeys`, multi-select with current keys pre-selected
87+
- **Startup Script**: API call to `StartupScripts.GetAllStartupScripts`, includes "None (clear)" option
88+
- **Hostname Pattern**: text input with placeholder hint `{random}-{location}`
89+
90+
## Show Command
91+
92+
Displays all template fields, including those previously hidden:
93+
- Fields with empty values show `-`
94+
- `storage_skip: true` shows `None (skipped)`
95+
- `startup_script_skip: true` shows `None (skipped)`
96+
- `hostname_pattern` always displayed
97+
- Wider label column (`%-18s`) for alignment
98+
99+
## Interactive Picker
100+
101+
`pickTemplateEntry` (in `show.go`) is shared by show, delete, and edit:
102+
- Calls `ListAll(baseDir)` to get templates across all resource types
103+
- Shows select prompt with `resource/name` + description
104+
- Returns `nil` on user cancel (Ctrl+C)
105+
- Returns error if no templates found
106+
64107
## Gotchas & Edge Cases
65108

66109
- **Import cycle**: `cmd/template/` cannot import `cmd/vm/` for the Template type (circular dependency). Shared types live in `internal/verda-cli/template/`, re-exported by `cmd/template/types.go` via type aliases and `var` bindings.
67110
- **`billingTypeSet` / `locationSet` flags**: Needed because `IsSet` in the wizard can't distinguish `"on-demand"` (falsy `IsSpot=false`) from "unset". When a template sets billing type or location, these booleans are set to `true` so the wizard skips those steps.
68111
- **`NoOptDefVal` on `--from` flag**: Set to `" "` (space) so `--from` without a value is recognized as "flag changed but empty". When the user writes `verda vm create --from gpu-training`, cobra parses `gpu-training` as a positional arg; `RunE` recombines it into `opts.From`.
69112
- **Startup script "None (skip)" label**: The wizard presents "None (skip)" as a selectable option. Previously, this label text was captured as the script name. Fixed by checking `Value != ""` before storing the name.
70-
- **`ensurePricingCache`**: The confirm-deploy step calls this to fetch instance type and volume type pricing when the cache is empty. This happens when a template pre-filled earlier steps (skipping the steps that normally populate the cache).
113+
- **`ensurePricingCache`**: The confirm-deploy step calls this (with parent context, not `context.Background()`) to fetch pricing when the cache is empty from template pre-fill.
71114
- **Only first storage entry applied**: `applyTemplate()` only reads `tmpl.Storage[0]` because the wizard's convenience fields (`StorageSize`/`StorageType`) support a single additional volume.
72115
- **AutoDescription**: `Template.AutoDescription()` joins non-empty `InstanceType`, `Image`, and `Location` with `", "` for the list view.
73116
- **Directory permissions**: Template directories created with `0700`, files with `0644`.
74117
- **Non-existent directory**: `List()` and `ListAll()` return `nil, nil` (not an error) when the templates directory doesn't exist yet.
118+
- **Atomic file writes**: `Save` writes to `.tmp` file then renames to prevent corruption on crash.
119+
- **Entry JSON tags**: `Entry` struct has `json`/`yaml` tags for consistent lowercase keys in `-o json` output.
120+
- **Edit matchKind**: `edit.go` has its own `matchKind` function (separate from `wizard_cache.go`'s `matchesKind`) for filtering instance types by kind in the edit field editor.
75121

76122
## Relationships
77123

78124
- **`internal/verda-cli/template/`** -- Shared types (`Template`, `StorageSpec`, `Entry`) and I/O functions (`Save`, `Load`, `LoadFromPath`, `Resolve`, `List`, `ListAll`, `Delete`, `ValidateName`, `ExpandHostnamePattern`). Breaks the import cycle between `cmd/template/` and `cmd/vm/`.
79-
- **`cmd/vm/wizard.go`** -- `WizardMode` (Deploy vs Template), `RunTemplateWizard()` (runs wizard without hostname/description/confirm steps), `TemplateResult` struct, `ensurePricingCache()`
80-
- **`cmd/vm/template_apply.go`** -- `loadTemplateRef()`, `applyTemplate()`, `resolveTemplateNames()`, `resolveSSHKeyNames()`, `resolveStartupScriptName()`, `printTemplateSummary()`, `pickTemplate()`
81-
- **`cmd/vm/create.go`** -- `--from` flag definition, `resolveCreateInputs()` orchestrates template loading + wizard invocation, `createOptions` struct with template-related internal fields (`billingTypeSet`, `locationSet`, `storageSkip`, `startupScriptSkip`, `sshKeyNames`, `startupScriptName`)
82-
- **`cmdutil`** -- `Factory`, `IOStreams`, `LongDesc`, `Examples`, `DefaultSubCommandRun`, `WriteStructured`
83-
- **`clioptions`** -- `VerdaDir()` for resolving `~/.verda/` base path
125+
- **`cmd/vm/wizard.go`** -- `WizardMode` (Deploy vs Template), `RunTemplateWizard()`, `TemplateResult` struct
126+
- **`cmd/vm/wizard_cache.go`** -- `ensurePricingCache()` (accepts parent context)
127+
- **`cmd/vm/template_apply.go`** -- `loadTemplateRef()`, `applyTemplate()`, `resolveTemplateNames()` (with warnings), `resolveSSHKeyNames()`, `resolveStartupScriptName()`, `printTemplateSummary()`, `pickTemplate()`
128+
- **`cmd/vm/create.go`** -- `--from` flag definition, `resolveCreateInputs()` orchestrates template loading + wizard invocation, `createOptions` struct with template-related internal fields
129+
- **`cmdutil`** -- `Factory`, `IOStreams`, `WithSpinner`, `TemplatesBaseDir`, `LongDesc`, `Examples`, `DefaultSubCommandRun`, `WriteStructured`
84130
- **`petname`** -- `github.com/dustinkirkland/golang-petname` for `{random}` hostname expansion

internal/verda-cli/cmd/template/README.md

Lines changed: 47 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -1,18 +1,21 @@
11
# verda template -- Manage reusable resource templates
22

3-
Save, list, show, and delete reusable resource configuration templates. Templates pre-fill the `vm create` wizard so you don't repeat the same settings.
3+
Save, list, show, edit, and delete reusable resource configuration templates. Templates pre-fill the `vm create` wizard so you don't repeat the same settings.
44

55
## Commands
66

77
| Command | Description | Key Flags |
88
|---------|-------------|-----------|
99
| `verda template create [name]` | Interactive wizard to save a template | _(none)_ |
10+
| `verda template edit [resource/name]` | Edit specific fields of a template | _(none)_ |
1011
| `verda template list` | List all saved templates | `--type` |
11-
| `verda template show <resource/name>` | Display template details | `-o json` |
12-
| `verda template delete <resource/name>` | Delete a template (with confirmation) | _(none)_ |
12+
| `verda template show [resource/name]` | Display template details | `-o json` |
13+
| `verda template delete [resource/name]` | Delete a template (with confirmation) | _(none)_ |
1314

1415
Aliases: `verda tmpl`, `verda tmpl ls` (list), `verda tmpl rm` (delete)
1516

17+
All commands with `[resource/name]` show an interactive picker when the argument is omitted.
18+
1619
## Usage Examples
1720

1821
### Create
@@ -23,13 +26,22 @@ verda template create
2326

2427
# Create a template with a specific name
2528
verda template create gpu-training
26-
27-
# Short alias
28-
verda tmpl create my-template
2929
```
3030

3131
The create command runs the VM wizard in **template mode** -- the same 10 configuration steps (billing type through startup script) but without hostname, description, or confirm-deploy. The resulting settings are saved to disk.
3232

33+
### Edit
34+
35+
```bash
36+
# Interactive picker, then field menu
37+
verda template edit
38+
39+
# Edit a specific template
40+
verda template edit vm/gpu-training
41+
```
42+
43+
Shows a menu of all template fields with their current values. Pick a field to change, edit it with the appropriate prompt (static choices for simple fields, API-backed selection for instance type/location/image/SSH keys/startup script). Repeat until "Save & exit".
44+
3345
### List
3446

3547
```bash
@@ -39,38 +51,45 @@ verda template list
3951
# List only VM templates
4052
verda template list --type vm
4153

42-
# Short alias
43-
verda tmpl ls
54+
# JSON output
55+
verda template list -o json
4456
```
4557

4658
Output shows `NAME` (as `resource/name`) and an auto-generated `DESCRIPTION` built from instance type, image, and location.
4759

4860
### Show
4961

5062
```bash
63+
# Interactive picker
64+
verda template show
65+
5166
# Show a VM template
5267
verda template show vm/gpu-training
5368

5469
# Output as JSON
5570
verda template show vm/gpu-training -o json
5671
```
5772

73+
Displays all template fields including hostname pattern, storage skip, and startup script skip status. Unset fields show `-`, explicitly skipped fields show `None (skipped)`.
74+
5875
### Delete
5976

6077
```bash
61-
# Delete a VM template (prompts for confirmation)
62-
verda template delete vm/gpu-training
78+
# Interactive picker with confirmation
79+
verda template delete
6380

64-
# Short alias
65-
verda tmpl rm vm/gpu-training
81+
# Delete a specific template
82+
verda template delete vm/gpu-training
6683
```
6784

6885
## Template Storage
6986

7087
- Files stored at `~/.verda/templates/<resource>/<name>.yaml`
88+
- Base directory resolved via `cmdutil.TemplatesBaseDir()`
7189
- Organized by resource type subdirectory (currently only `vm/`)
72-
- Names must be lowercase alphanumeric with hyphens (regex: `^[a-z0-9][a-z0-9-]*$`)
90+
- Names must be lowercase alphanumeric with hyphens, no trailing hyphens (regex: `^[a-z0-9]([a-z0-9-]*[a-z0-9])?$`)
7391
- Auto-reformats invalid names: spaces and underscores become hyphens, uppercase becomes lowercase, other invalid characters are stripped, consecutive hyphens are collapsed
92+
- Atomic writes: saves to `.tmp` file then renames to prevent corruption
7493

7594
## Template YAML Format
7695

@@ -111,7 +130,7 @@ verda vm create --from gpu-training --hostname my-vm --description "test"
111130

112131
1. Template values pre-fill the wizard's `createOptions`
113132
2. A summary of template values is printed to stderr
114-
3. SSH keys and startup scripts are resolved by name to ID via the API; unresolved names produce warnings
133+
3. SSH keys and startup scripts are resolved by name to ID via the API; unresolved names produce warnings (no longer silently swallowed)
115134
4. Only unfilled steps are prompted (hostname, description, confirm-deploy are always prompted; other steps only if the template didn't fill them)
116135
5. The confirm-deploy step fetches pricing for the deployment summary (via `ensurePricingCache` if earlier pricing steps were skipped)
117136

@@ -123,13 +142,11 @@ The `--from` flag uses `NoOptDefVal` so it can be used in three ways:
123142
- `--from ./path/to/template.yaml` -- treated as a file path (contains `/` or ends with `.yaml`)
124143
- `--from` (no value) -- shows an interactive picker of saved VM templates
125144

126-
When `--from` consumes no value, the template name may appear as a positional arg (e.g., `verda vm create --from gpu-training`). The `RunE` handler recombines it.
127-
128145
## Hostname Pattern
129146

130147
The `hostname_pattern` field supports two placeholders:
131148

132-
- `{random}` -- replaced with 3 random petname words joined by hyphens (e.g., `cold-cable-smiles`)
149+
- `{random}` -- replaced with 3 random petname words joined by hyphens (e.g., `cold-cable-smiles`). Each `{random}` occurrence generates different words.
133150
- `{location}` -- replaced with the lowercased location code (e.g., `fin-01`)
134151

135152
Example: `"gpu-{random}-{location}"` expands to something like `"gpu-cold-cable-smiles-fin-01"`.
@@ -150,18 +167,26 @@ These are captured when the user selects "None (skip)" during template creation
150167
### Files
151168

152169
- **template.go** -- Parent command definition (`verda template`), registers subcommands
153-
- **create.go** -- `template create` command; prompts for resource type and name, runs VM wizard in template mode, saves result
170+
- **create.go** -- `template create` command; prompts for resource type and name, runs VM wizard in template mode, saves result. Contains `normalizeName`, `vmResultToTemplate`
171+
- **edit.go** -- `template edit` command; field menu loop with API-backed editors for instance type, location, image, SSH keys, startup script. Contains `matchKind` for filtering
154172
- **list.go** -- `template list` command; lists entries with auto-description, supports `--type` filter and structured output
155-
- **show.go** -- `template show` command; displays template fields, supports `-o json` structured output
173+
- **show.go** -- `template show` command; displays all fields (including skip flags and hostname pattern), supports `-o json`. Contains shared `pickTemplateEntry` helper and `parseRef`
156174
- **delete.go** -- `template delete` command; loads template to verify existence, confirms, then deletes
157175
- **types.go** -- Re-exports types and functions from `internal/verda-cli/template/` to avoid import cycles
158176

159177
### Shared Package
160178

161-
- **`internal/verda-cli/template/template.go`** -- Core types (`Template`, `StorageSpec`, `Entry`), I/O functions (`Save`, `Load`, `LoadFromPath`, `Resolve`, `List`, `ListAll`, `Delete`), name validation, hostname pattern expansion
179+
- **`internal/verda-cli/template/template.go`** -- Core types (`Template`, `StorageSpec`, `Entry` with JSON tags), I/O functions (`Save` with atomic write, `Load`, `LoadFromPath`, `Resolve`, `List`, `ListAll`, `Delete`), name validation (`ValidateName`), hostname pattern expansion (`ExpandHostnamePattern`)
162180

163181
### Integration with `vm create`
164182

165183
- **`cmd/vm/create.go`** -- Defines `--from` flag, calls `resolveCreateInputs`
166-
- **`cmd/vm/template_apply.go`** -- `loadTemplateRef`, `applyTemplate`, `resolveTemplateNames`, `printTemplateSummary`
167-
- **`cmd/vm/wizard.go`** -- `WizardMode`, `RunTemplateWizard`, `TemplateResult`, `ensurePricingCache`
184+
- **`cmd/vm/template_apply.go`** -- `loadTemplateRef`, `applyTemplate`, `resolveTemplateNames` (with warnings to `ioStreams.ErrOut`), `printTemplateSummary`
185+
- **`cmd/vm/wizard.go`** -- `WizardMode`, `RunTemplateWizard`, `TemplateResult`, step Default functions
186+
- **`cmd/vm/wizard_cache.go`** -- `ensurePricingCache` (accepts parent context)
187+
188+
### Shared Helpers
189+
190+
- **`cmdutil.TemplatesBaseDir`** (`cmd/util/paths.go`) -- Centralized `~/.verda/templates` path
191+
- **`cmdutil.WithSpinner[T]`** (`cmd/util/spinner.go`) -- Used by edit command for API-backed field editors
192+
- **`pickTemplateEntry`** (`show.go`) -- Shared interactive template picker, used by show, delete, and edit commands

0 commit comments

Comments
 (0)