feat: add support for loading multiple config files and staleness check#908
feat: add support for loading multiple config files and staleness check#908Elia-Renzoni wants to merge 2 commits into
Conversation
Signed-off-by: Elia Renzoni <elia.renzoni03@gmail.com>
Pull Request Test Coverage Report for Build 20577480854Details
💛 - Coveralls |
Signed-off-by: Elia Renzoni <elia.renzoni03@gmail.com>
|
@Elia-Renzoni thanks so much for this, we haven't forgotten about it. We were already feature-locked and in the release candidate phase for v2.0.0 when this PR was opened. We are targeting this for v2.1.0. I'll review and test it out this week. Thanks again! |
houyuwushang
left a comment
houyuwushang
left a comment
There was a problem hiding this comment.
I rebased these two commits onto current main and traced the directory path through daemon.Hup. The basic case with two existing files still works, but I found several cases that can miss configuration changes or break existing callers. I left the reproductions and affected behavior inline. The main structural issue is that staleness is represented as parallel slices for only the files present at startup, rather than as a snapshot of the directory contents.
| if len(c.Main.configFilesPath) > 0 { | ||
| for index, file := range c.Main.configFilesPath { | ||
| t := c.CheckFileLastModified(file) | ||
| if t.IsZero() { |
There was a problem hiding this comment.
This only walks configFilesPath captured during the initial load. A newly added YAML file is never examined, while a removed file returns a zero timestamp and is skipped here. daemon.Hup calls this method before loading again, so both changes make an explicit reload report "not stale" and leave the running config unchanged. I reproduced both cases after rebasing onto current main. Could this compare a fresh directory snapshot (names and modification times) with the loaded snapshot instead?
| if c.Main == nil || | ||
| (len(c.Main.configFilesPath) == 0 && | ||
| (c.Main.configFilePath == "" || | ||
| time.Now().Before(c.Main.configRateLimitTime))) { |
There was a problem hiding this comment.
For directory-backed configs, len(c.Main.configFilesPath) == 0 is false, so the rate-limit check is never evaluated. After an existing file changes, IsStale() scans the files and returns true on every call during the rate-limit window. The rate-limit condition should apply to both single-file and directory modes.
| // CheckFileLastModified returns the last modified date of the running config file, if present | ||
| func (c *Config) CheckFileLastModified() time.Time { | ||
| if c.Main == nil || c.Main.configFilePath == "" { | ||
| func (c *Config) CheckFileLastModified(confFile string) time.Time { |
There was a problem hiding this comment.
This changes an exported v2 method from CheckFileLastModified() to CheckFileLastModified(string), which breaks external callers at compile time. Could the path-taking helper stay private while retaining the existing no-argument method for compatibility?
| if len(c.Main.configFilesPath) == 0 { | ||
| return c.Main.configFilePath | ||
| } | ||
| return c.Flags.ConfigPath |
There was a problem hiding this comment.
Clone() copies configFilesPath but does not copy Flags, so cfg.Clone().ConfigFilePath() panics for every directory-backed config at this dereference. I reproduced that on current main. Storing the directory path in MainConfig and deep-copying the directory snapshot would avoid making this accessor depend on runtime flags.
|
|
||
| // loadAndMergeFiles loads application configuration from multiple YAML files | ||
| func (c *Config) loadAndMergeFiles(flags *Flags) error { | ||
| files, err := fs.Glob(os.DirFS(flags.ConfigPath), "*.yaml") |
There was a problem hiding this comment.
This only matches .yaml. A directory containing trickster.yml loads no files and eventually returns no valid backends configured, although the PR describes loading all YAML files. Is .yml intentionally unsupported? If not, this should enumerate both extensions and return a specific error when no matching files are present.
| return err | ||
| } | ||
|
|
||
| if err = c.loadYAMLConfig(string(data)); err != nil { |
There was a problem hiding this comment.
Repeated unmarshalling keeps distinct map keys, but a later file replaces an entire same-named backend. In a local case where the second fragment only overrode origin_url, the loaded backend lost its provider. Since the PR says files are merged in order, this needs an explicit contract: deep-merge fields, reject duplicate resource names, or document that a repeated resource must be complete.
4 participants
@jranson @crandles
This PR improves Trickster’s configuration system by extending support from a single configuration file to multiple YAML configuration files loaded from a directory.
The implementation assumes that users who want to use multiple configuration files will provide only the directory path as a flag
(e.g., /etc/trickster) and the code will automatically look for all YAML files in that directory and load them in order, merging the configurations.
To support this feature, some methods were refactored, any feedback on these changes would be appreciated.
Related Issue: #493