Add Redis service CLI support - #97
Conversation
jwfing
left a comment
There was a problem hiding this comment.
Review — Add Redis service CLI support
Summary: Adds redis to the service-type allowlist, the interactive Add-Service kind list, and list rendering; a clean, consistent, well-scoped 14/-9 change. npm run typecheck passes and the targeted test/resolve-service.test.ts suite is green (17/17) locally.
Requirements context: No matching spec/plan found — this repo has no /docs/superpowers/ (or docs/specs/) directory. Assessed against the PR title/body and the surrounding code conventions alone. The change mirrors the platform dashboard's Add Service menu, which the code comments cite as the source of truth (add-service-button.tsx).
Critical
(none)
Suggestion
Software engineering — src/commands/services.ts:128 (row rendering has no redis test)
serviceListLine gains a new redis branch ( tcp/${s.port ?? 6379}${volume}), but test/services.test.ts (describe('serviceListLine', …)) has a dedicated assertion for every other row type — compute (with/without image, with volume), storage, postgres — and this PR adds none for redis. By the repo's own pattern every rendered row type is unit-tested; a redis case (e.g. asserting tcp/6379, the ?? 6379 fallback when the API omits port, and the optional vol NGi suffix) would keep parity and lock in the display contract. The resolve-service side is well-covered — this is the one behavior added without a matching test.
Information
Functionality — cross-repo dependency (src/commands/services.ts:112)
servicesAdd passes type: 'redis' straight through to POST /projects/:id/services. This assumes the platform already accepts redis; if the backend support isn't deployed, insta services add redis … will surface a server error rather than a client-side rejection. That's the correct layering for a thin CLI (validation stays an allowlist, the platform owns provisioning), just worth confirming the platform side lands first/together.
Functionality — --port not settable for redis (src/commands/services.ts:100-103)
--port is rejected for any non-compute type, so redis always provisions on the platform default and the list row falls back to 6379 for display via s.port ?? 6379. That's a reasonable default for a managed cache; flagging only so it's an intentional choice rather than an oversight. The --region help text was correctly extended to include redis, and servicesAdd already allows region for non-storage types, so that path is consistent.
Security: No security-relevant changes — new input is validated against a fixed allowlist (assertType), no secrets/PII touched, no auth paths modified, no new dependencies.
Performance: No performance-relevant changes — no new queries, loops, or per-request allocations; the additions are constant-time string/branch logic.
Verdict
approved (informational — human approval via the separate approve flow). No blocking issues; the one Suggestion is a test-parity gap worth closing before merge but not required.
Adds Redis to service type validation, interactive service selection, list rendering, and CLI tests. Validation: npm run typecheck; targeted vitest suite.
Summary by cubic
Adds Redis as a first-class service type in the CLI so users can provision and list Redis services alongside Postgres, Storage, and Compute. This aligns the CLI with the dashboard service kinds and surfaces Redis-specific details in list output.
Written for commit 9e0217d. Summary will update on new commits.