Follow-ups from PR #24 (localmode compose/db-auth)
- Dominant language
- Go
- Stars
- 2
- Forks
- 1
- Avg merge
- 13h 32m
- Merged PRs (30d)
- 45
Description
Tracking two non-blocking items surfaced during review of #24 (approved & merged separately).
### 1. `composeTemplateEnvValue` regex is brittle to inline comments
`internal/localmode/compose_test.go` — the value-extraction regex ends in `"?\s*$`, which breaks if an inline comment is ever added to one of the three timing lines:
- **Quoted + comment** (`REDIS_TIMEOUT: "60s" # note`) → `FindStringSubmatch` returns nil → `require.Len(match, 2)` fails with a misleading *"missing env var"*.
- **Unquoted + comment** (`USAGE_SYNC: 30s # note`) → the comment is swallowed into the capture group → `time.ParseDuration` errors.
The runtime YAML/compose parser strips inline comments fine (verified with `yaml.v3`), so this is test-only — but the template already uses inline comments heavily, so it's an easy trap. Suggested fix:
```go
re := regexp.MustCompile(`(?m)^\s*` + regexp.QuoteMeta(key) + `:\s*"?([^"#\n]+?)"?\s*(?:#.*)?$`)
```
Ref: https://github.com/Kong/volcano-cli/pull/24#discussion_r3523452497
### 2. Confirm `info_test.go` DB-URL fixture matches the server contract (cross-repo)
`internal/localmode/info_test.go` now asserts a new `database_url` shape:
```
postgres://volcano_client_:vpg_local_secret@localhost:8002/app?sslmode=disable&application_name=volcano_full_access
```
This is a pure passthrough unit test (no CLI bug — `info.go` forwards the URL verbatim to `psql`), but the format isn't what the server currently emits. In `volcano-hosting`, `local info` builds the URL via `localmode.DatabaseConnectionString`, which still returns the old shape (`volcano:volcano@... & application_name=volcano_full_access:`).
The new format drops the `:` suffix from `application_name` and introduces a scoped role (`volcano_client_` + real password). The server's pgproxy currently identifies full_access/admin connections by parsing that suffix — `strings.CutPrefix(appName, "volcano_full_access:")` (`pooling_proxy.go:847`). Confirm the paired server change (a) emits exactly this URL from `local info` and (b) moves routing/auth onto the scoped role, otherwise admin connections lose their target-DB routing.
Ref: https://github.com/Kong/volcano-cli/pull/24#discussion_r3523462009
Contributor guide
Research direction
Start with internal/localmode/compose_test.go and internal/localmode/info_test.go, then inspect the paired server contract at localmode.DatabaseConnectionString and pooling_proxy.go:847. Run the localmode tests to reproduce the inline-comment and database_url cases. Done means comment-tolerant extraction tests pass and the server emits the stated URL while preserving target-DB routing and scoped-role authentication.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go, postgresql
- Domain
- cli, databases
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 38/100