fix(reload): store paths to config files #1214
Jade
nex
Labels
No labels
Blocked
Bug
Cherry-picking
Database
Dependencies
Dependencies/Renovate
Difficulty
Easy
Difficulty
Hard
Difficulty
Medium
Documentation
Enhancement
Good first issue
Help wanted
Inherited
Matrix/Administration
Matrix/Appservices
Matrix/Auth
Matrix/Client
Matrix/Core
Matrix/Federation
Matrix/Hydra
Matrix/MSC
Matrix/Media
Meta
Meta/CI
Meta/Packaging
Priority
Blocking
Priority
High
Priority
Low
Security
Status
Confirmed
Status
Duplicate
Status
Invalid
Status
Needs Investigation
Support
To-Merge
Wont fix
old/ci/cd
old/rust
No project
No assignees
3 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
continuwuation/continuwuity!1214
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "oddlid/continuwuity:reload-fix"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Paths given via --config are now stored inside the config struct at
runtime, to make it possible to reload config without setting an env
var for the config file location.
This is working now with my latest changes. I've built and run it locally to verify.
I rebased on main before I pushed, which seems to have messed up the display of what's really happened here a bit. Is there anything I should do about that?
0b4bf89a2e84a5283b8dThis looks pretty much good to me!
@ -23,0 +22,4 @@#[arg(short,long,env = "CONDUIT_CONFIG",I'm not entirely sure this is necessary or how it interacts with the order of config merges? Might belong as a separate change
It’s not necessary. I just added it for consistency. But if my assumptions are correct, adding this here, could enable us to remove the env merging in Config::load, since any of these env vars would now result in the —config option being set.
But yeah, maybe it’s better to have it in another PR that adds these env vars to clap and removes the merging in Config::load. Or I could add that too to this PR. You decide how you want it, and if you want to split it, I can adjust and make another PR.
As for the order of config merges, it’s now sorted alphabetically in the reload admin command, with duplicate files removed, since I discovered that they would add up on each reload otherwise (if a file argument was given in the reload command).
Given reload on SIGUSR1, the file list stays the same, and no filtering should be needed. There still is a way to have duplicate files though, if they’re given at startup in duplicate ways through either/or cli flags and/or env vars. So if you send SIGUSR1 with such a list, the config merging will go through the same files several times. This is as it was since before.
Maybe we should look into filtering out duplicates in all code paths?
a15083efd2894eb47862I’ve now cleaned up and squashed. Please have another look, and if you’re satisfied, go ahead and merge. We can look more into issues discussed outside this scope later :)
894eb47862867d0ab671Looks good to me :)