-
Notifications
You must be signed in to change notification settings - Fork 150
feat(router): hot config reloading #1746
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
8a15b9e
to
7348dd5
Compare
Router image scan passed✅ No security vulnerabilities found in image:
|
0259b29
to
2b9f831
Compare
15fa6b5
to
e3c5c15
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces hot configuration reloading for the router by adding file watching and SIGHUP trigger support, while also refining the config loading responsibilities between main.go and config.go.
- Updated configuration to include hot-reload settings
- Modified LoadConfig signature and updated tests accordingly
- Enhanced shutdown error handling with a new concurrent error joiner and integrated config watcher in the main command
Reviewed Changes
Copilot reviewed 21 out of 26 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
router/pkg/config/fixtures/full.yaml | Added watch_config section for enabling hot config reloading |
router/pkg/config/config_test.go | Updated tests to use the new LoadConfig signature and added tests for watch configuration via environment variables |
router/core/router.go | Introduced a concurrent error joiner and updated router shutdown logic with improvements |
router/internal/timex/duration_test.go | Added tests for RandomDuration function; contains an error in the loop iteration |
Other files | Minor adjustments (e.g., quoting in YAML/compose files, dotenv usage, test env helpers) to support the overall changes |
Files not reviewed (5)
- docker/nats/Dockerfile: Language not supported
- docker/nats/build-push.sh: Language not supported
- docker/nats/nats-server.conf: Language not supported
- router-tests/go.mod: Language not supported
- router/pkg/config/config.schema.json: Language not supported
049cc58
to
90a6c00
Compare
7a82e01
to
812ab58
Compare
f257a58
to
10c406b
Compare
3654e1c
to
6215c28
Compare
avoid shutdown data race
3dd9987
to
d6611cb
Compare
d6611cb
to
aad72f2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Motivation and Context
As a user who deploys the router with a dynamic config from the filesystem, I'd like it such that when the file changes on disk, there is the option to have the router automatically reload itself when it detects this change.
Changes
cmd/router/main.go
androuter/pkg/config/config.go
config.LoadConfig
no longer loads dotenv into the environment or decides the config path under any circumstancemain.Main
nowrandomDuration
utility out ofcontrolplane
package intointernal/timex
for reuseConfig responsibility change
As a side effect of drawing a clearer boundary between the responsibilities of
main.go
andconfig.go
for determining the config file to read, direct calls toLoadConfig
will no longer checkCONFIG_FILE
environment variable. This is intentional, as this responsibility now falls to the caller.LoadConfig
continues to default toconfig.DefaultConfigPath
when the provided path is""
.Limitations
The config path loading and the file watcher are outside the reload loop, so:
Configuration Example
Checklist