feat(rover-ctl): add EventSpec handler, env placeholder substitution, and HTTP client improvements#264
Conversation
ron96g
commented
Mar 5, 2026
- Add EventSpecification handler with JSON schema resolution and file:// ref support
- Introduce ${VAR} placeholder substitution in resource files during parsing
- Add static header HTTP client wrapper with User-Agent and local access token support
- Replace port subscription type with event subscription type in Rover handler
- Fix ensureCorrectBasePath to treat "/" as empty and stop polling on failed state
… and HTTP client improvements
- Add EventSpecification handler with JSON schema resolution and file:// ref support
- Introduce ${VAR} placeholder substitution in resource files during parsing
- Add static header HTTP client wrapper with User-Agent and local access token support
- Replace port subscription type with event subscription type in Rover handler
- Fix ensureCorrectBasePath to treat "/" as empty and stop polling on failed state
There was a problem hiding this comment.
Pull request overview
This PR extends rover-ctl with support for EventSpecification resources, introduces ${VAR} environment placeholder substitution during YAML/JSON parsing, and improves HTTP client behavior (static headers/User-Agent + local token support) alongside a few handler/poller fixes.
Changes:
- Add placeholder substitution (
${VAR}) to the parser and tests for placeholder behavior and parsing. - Add an EventSpecification handler with JSON schema
$refresolution (includingfile://references) and register it. - Add a static-header HTTP client wrapper and update readiness polling/base path handling.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| rover-ctl/pkg/parser/testdata/placeholder-resource.yaml | New test resource containing ${VAR} placeholders. |
| rover-ctl/pkg/parser/placeholders_test.go | Unit tests for placeholder substitution behavior. |
| rover-ctl/pkg/parser/placeholders.go | Implements ${VAR} environment substitution with unresolved-variable errors. |
| rover-ctl/pkg/parser/parser_test.go | Integration tests verifying placeholder substitution during file parsing. |
| rover-ctl/pkg/parser/parser.go | Applies placeholder substitution during file parse; preserves filename property without relying on *os.File. |
| rover-ctl/pkg/handlers/v0/rover_test.go | Updates rover subscription tests from port to eventType. |
| rover-ctl/pkg/handlers/v0/rover.go | Updates subscription patching logic to detect eventType subscriptions. |
| rover-ctl/pkg/handlers/v0/eventspec.go | New EventSpecification handler with JSON schema parsing and file:// $ref support. |
| rover-ctl/pkg/handlers/registry.go | Registers the new EventSpecification handler. |
| rover-ctl/pkg/handlers/common/status_poller.go | Stops polling on failed processing state as well as done. |
| rover-ctl/pkg/handlers/common/httpclient.go | Adds static header HTTP client wrapper (WithStaticHeaders). |
| rover-ctl/pkg/handlers/common/base_handler.go | Adds User-Agent header and optional local access-token path for HTTP client setup. |
| rover-ctl/pkg/config/token.go | Treats expected base path of / as empty. |
| rover-ctl/pkg/config/config.go | Adds access.token default (env: ROVER_ACCESS_TOKEN). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…ient, and status poller
…directly from spec map
BjoernKarma
left a comment
There was a problem hiding this comment.
LGTM
The only question in my mind was whether or not to use a standard library for the env variable replacement. But I think for our purpose the custom implementation works good and it is already very well covered with tests.
| ext := strings.ToLower(filepath.Ext(filePath)) | ||
|
|
||
| file, err := os.OpenFile(filePath, os.O_RDONLY, 0o644) | ||
| content, err := os.ReadFile(filePath) |
There was a problem hiding this comment.
i can send you big file and it will be whole allocated in memory. Do you have some extra protection above?
There was a problem hiding this comment.
This will run client-side anyways and there is a request-size-limit on server-side, so I dont think this should be a big issue. Normally these files are fairly small anyways
| } | ||
| defer file.Close() | ||
|
|
||
| content, err = SubstitutePlaceholders(content) |
There was a problem hiding this comment.
cannot Env variables affect json or yaml format? shouldn't be replace logic be executed after parsing or do you need replace some values in middle of string or keys?
There was a problem hiding this comment.
If someone puts some invalid stuff in there which breaks the parsing, then its their problem. The idea of this feature is just to allow secrets aka strings to be replaced and that should not be a big issue imo
# [0.18.0](v0.17.0...v0.18.0) (2026-03-18) ### Bug Fixes * **common-server:** race-condition and security improvements ([#272](#272)) ([9d08e02](9d08e02)) * **gateway:** add deny-all sentinel to ACL plugin when no consumers exist ([#277](#277)) ([0dd6017](0dd6017)) ### Features * **common-libs:** improve error handling, add feature flags, and fix logger type ([#260](#260)) ([03165a0](03165a0)) * **common-server:** unify latency format in logs ([#267](#267)) ([5fbb19e](5fbb19e)) * **gateway:** add buffering config, dynamic upstream, access control fix, and secret-manager integration ([#259](#259)) ([719c58f](719c58f)) * introduce event-driven communication to the control plane ([#265](#265)) ([f1ed1f5](f1ed1f5)) * **rover-ctl:** add EventSpec handler, env placeholder substitution, and HTTP client improvements ([#264](#264)) ([f15e0f7](f15e0f7)) * **rover-ctl:** support for get-info-many cmd ([#246](#246)) ([1d9f194](1d9f194)) * **secret-manager:** add write strategy support and improve caching ([#263](#263)) ([3f311bf](3f311bf))