feat(classic): add computer groups and mobile device groups commands#222
Conversation
Adds Classic API support for /computergroups and /mobiledevicegroups via the classic generator manifest. Both expose full CRUD (list/get/create/ update/delete) plus name-based apply and --name lookup, surfacing the classic XML group model (smart criteria, static membership, and the *_additions/*_deletions update semantics) not available through the modern smart/static group endpoints. - specs/classic/resources.yaml: classic-computer-groups (computer_group), classic-mobile-device-groups (mobile_device_group) - internal/commands/groups.go: map both into the Classic Computers / Classic Mobile Devices help groups - regenerated classic command + registry files Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ktn-jamf
left a comment
There was a problem hiding this comment.
Tip
✅ Merge-ready — Adds Classic API classic-computer-groups and classic-mobile-device-groups commands (full CRUD + name-based apply/--name lookup) via two declarative entries in the classic manifest. No critical or important issues found.
This is a 1029-line diff, but 1006 of those lines are mechanically generated. The actual decision surface is 14 hand-written lines: a 12-line spec addition in specs/classic/resources.yaml and 2 lines of help-group wiring in groups.go. I verified the generated output rather than reviewing it line-by-line.
What I verified:
- No generator drift —
make verify-generatedpasses: the committed generated files (classic_computer_groups.go,classic_mobile_device_groups.go, and the three registry files) exactly match whatmake generateproduces from the new spec. Nothing undergenerated/was hand-edited, per the CLAUDE.md boundary. - Build + tests pass —
go build ./...clean;TestApplyProGroups/grouping/classic tests pass. The CLAUDE.md tripwire (TestApplyProGroups_AllCommandsGroupedfailing when a new resource isn't grouped) is satisfied because both resources were correctly mapped into the Classic Computers / Classic Mobile Devices help groups. - Spec correctness — Explicit
singular: computer_group/mobile_device_groupis necessary and correct; the parser's auto-derivation (TrimSuffix(name,"s")) would otherwise produce the wrongcomputergroup. Omittingoperations/lookupscorrectly defaults to full CRUD +[id, name], which is what yieldsapplyand--namelookup. The two entries match the citedadvancedcomputersearchesprecedent field-for-field. - No injection / no conflict — generated paths use
url.PathEscapeon all user input;get/update --nameroute through the API's/name/{name}endpoint. The new standalonemobiledevicegroupsresource is orthogonal to the pre-existinggroups_path: mobiledevicegroupsonclassic-mobile-devices(that field drives a--groupbulk-delete-of-members flag — different command, no name clash).
What's done well
✅ The PR description documents a non-obvious design decision well — it explains why the combined classic /computergroups endpoint is worth adding despite the modern split smart/static group commands (it surfaces the XML group model with *_additions/*_deletions static-membership semantics), cites the advanced-searches precedent, and flags the overlap as intentional. The classic- prefix cleanly avoids collision with the modern smart-computer-groups/static-computer-groups commands. The (smart and static) descriptions are accurate per docs/GLOSSARY.md.
✅ Live-tested against a real platform-gateway instance (283 computer / 35 mobile groups, get by id and --name) — the one thing generated-code review can't fully substitute for.
Review coverage
- Design and architecture: combined classic endpoint, justified overlap with modern split groups, follows advanced-searches precedent,
classic-prefix avoids command collision - Correctness: spec fields correct (explicit singular avoids bad auto-derivation), defaults yield CRUD + name + apply, generated code matches spec (no drift), build + tests pass, name lookup uses API
/name/endpoint - Security: paths are constant strings; user input passes through
url.PathEscape— no injection vector; no auth/secrets surface - [na] Performance: list is a single GET identical to 50+ other classic resources; no hot paths or queries introduced
- Test coverage: generated commands covered by generator tests + grouping test + smoke registry; author live-tested; no bespoke logic to unit-test
- [na] Reliability: no new error handling/retry logic; inherits
internal/clientbehavior - Code quality: spec entries match precedent field-for-field; consistent naming and descriptions
- [na] Simplification: minimal by construction (declarative 12-line spec change)
- [na] Frontend concerns: no UI changes
- Project rules compliance: no manual edits to
generated/(verified via no-drift), spec-driven workflow per CLAUDE.md, resources grouped ingroups.go
Scope of review
- Reviewed in full:
specs/classic/resources.yaml(+12),internal/commands/groups.go(+2), and all 5 generated files (registry/smoke/backup/provenance + spot-check ofclassic_computer_groups.go). - Related files checked:
generator/classic/parser.go(defaults for operations/lookups/singular),generator/classic/generator.go(groups_path/GroupPathsemantics),docs/GLOSSARY.md(smart/static terminology). - Verification run:
make verify-generated,go build ./...,go test ./internal/commands/... -run 'TestApplyProGroups|Grouped|Classic'. - No prior reviews on this PR.
- Specialist agents: not dispatched — no security/concurrency/performance surface in a declarative spec + generated CRUD wrappers; would have been noise.
- Note on generated
Short/Exampletext ("List all computergroups" uses the raw resource name): template-driven and identical across all 55 classic resources — not flagged, since fixing it means editing a generator template (out of scope) and it's a consistent existing convention.
Generated by pr-review v1.7.0, a Jamf Claude Code skill
Resolve generated file conflicts by running make generate against the auto-merged spec sources, combining fallback path fields from main (PR #223) with the new classic-computer-groups and classic-mobile-device-groups entries from this branch. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Summary
Adds Classic API support for
/computergroupsand/mobiledevicegroupsvia the classic generator manifest:classic-computer-groups(computer_group)classic-mobile-device-groups(mobile_device_group)Both expose full CRUD (
list/get/create/update/delete) plus name-basedapplyand--namelookup.Why, given modern smart/static group endpoints exist
The classic combined endpoint surfaces the XML group model — smart criteria, static membership, and the
*_additions/*_deletionsupdate semantics — which is better suited to manipulating static group membership than the modern split smart/static endpoints. Overlap is intentional. Precedent: advanced computer/mobile searches were added to the classic manifest the same way.Changes
specs/classic/resources.yaml— two new resource entriesinternal/commands/groups.go— map both into the Classic Computers / Classic Mobile Devices help groupsmake generate)Verification
make lint— 0 issuesmake test— all packages passlist(283 computer / 35 mobile groups),getby id,get --name— all return correct XML with criteria🤖 Generated with Claude Code