Conversation
The View() method used the deprecated top-level HasConcurrency() check which always returned false. Add concurrency_mode and num_instances fields to the ServiceConfig RPC type, populate them from the ConfigSpec in GetConfiguration, and use per-service mode lookup in the TUI.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe ServiceConfig schema and generated RPC code were extended with two fields: 📝 Coding Plan
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/app/rpc.yml`:
- Around line 50-55: The RPC schema exposes concurrency_mode and num_instances
on ServiceConfig but servers/app/app.go only reads service_env from
cfg.Services(), so clients can submit edits that are ignored; either remove
concurrency_mode and num_instances from the mutable Configuration payload in
rpc.yml or wire them through AppInfo.SetConfiguration so updates persist: update
the setConfiguration handling to accept and persist these fields (reference
ServiceConfig, getConfiguration, setConfiguration, AppInfo.SetConfiguration, and
cfg.Services()) or remove the two fields from ServiceConfig until persistence is
implemented.
In `@servers/app/app.go`:
- Around line 359-360: Only call sc.SetConcurrencyMode and sc.SetNumInstances
when the source service actually carries those settings: check svc.Concurrency
for nil and use the presence checks (e.g., svc.Concurrency.HasMode() /
svc.Concurrency.HasNumInstances() or equivalent fields) before calling
SetConcurrencyMode(svc.Concurrency.Mode) or
SetNumInstances(int32(svc.Concurrency.NumInstances)); this preserves omission
semantics so HasConcurrencyMode()/HasNumInstances() on sc remain false when the
source had no explicit values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d8e0770a-da5f-474e-b282-c9c36c74bf97
📒 Files selected for processing (4)
api/app/app_v1alpha/rpc.gen.goapi/app/rpc.ymlcli/commands/app.goservers/app/app.go
…resent Preserves omission semantics so HasConcurrencyMode()/HasNumInstances() remain false when the config has no explicit concurrency settings.
Summary
concurrency_modeandnum_instancesfields to theServiceConfigRPC typeConfigSpecin theGetConfigurationhandlermiren appTUI View() to check per-service concurrency mode instead of the deprecated top-levelHasConcurrency()which always returned falseTest plan
go generate ./api/app/regenerates cleanlyhack/it servers/app— all 42 tests pass[services.web.concurrency] mode = "fixed" num_instances = 2, verifymiren appshows "(fixed)"m app liststill shows "(fixed)" (regression check)