Repository navigation
Conversation
laskoviymishka
left a comment
laskoviymishka
left a comment
There was a problem hiding this comment.
Only library callers of config.LoadConfig see a runtime change: an explicit file that can't be read, or an unset $HOME, now comes back as an error instead of nil data. The CLI and EnvConfig behave as they do on main.
| func LoadConfigFile(configPath string) ([]byte, error) { | ||
| // LoadConfig reads configuration data and reports path and read failures. | ||
| // An empty configPath selects ~/.iceberg-go.yaml. A missing implicit | ||
| // home-directory configuration is not an error. Use [ParseConfig] to decode |
There was a problem hiding this comment.
Should the doc also say what a missing implicit file returns, and that GOICEBERG_HOME isn't consulted? EnvConfig honors it, so a LoadConfig("") caller could expect the same.
| // home-directory configuration is not an error. Use [ParseConfig] to decode | |
| // home-directory configuration is not an error; LoadConfig returns nil data. | |
| // Unlike [EnvConfig], it ignores GOICEBERG_HOME. Use [ParseConfig] to decode |
| // An empty configPath selects ~/.iceberg-go.yaml. A missing implicit | ||
| // home-directory configuration is not an error. Use [ParseConfig] to decode | ||
| // the YAML and select a catalog. | ||
| func LoadConfig(configPath string) ([]byte, error) { |
There was a problem hiding this comment.
| if err := yaml.Unmarshal(LoadConfig(dir), &cfg); err != nil { | ||
| // EnvConfig keeps its historical best-effort initialization behavior. | ||
| // Callers reading an explicit file should handle errors from LoadConfig. | ||
| data, _ := LoadConfig(dir) |
There was a problem hiding this comment.
Could we pin this drop with a fromConfigFiles test? With GOICEBERG_HOME set this reads an explicit path, so it drops real read errors too, yet go test ./config/ still passes with the _ swapped for a panic(err). A directory at that path should still give the defaults, and a t.Setenv("HOME", "") case for LoadConfig("") would pin the UserHomeDir error as well.
Signed-off-by: Minh Vu <vuhoangminh97@gmail.com>
2967c26 to
8327a5e
Compare
zeroshade
left a comment
zeroshade
left a comment
There was a problem hiding this comment.
LoadConfig now reports read failures for explicit files and an unavailable home directory, while a missing default config stays nil, nil. The CLI was already on the error-returning path, and EnvConfig stays best-effort, which TestFromConfigFilesIgnoresReadErrors pins. I merged it with current main locally and the build, vet and config/CLI tests pass.
Nit: LoadConfigFile and the old LoadConfig signature are both released API, but releases.md has no entry under "Unreleased compatibility notes". As with #2290, it looks like the "sync release notes with main" commit dropped it. Could you add one (LoadConfig returns ([]byte, error), and LoadConfigFile callers switch to LoadConfig) in a follow-up? LGTM.
…pache#2307, apache#2310 and apache#2313 Requested in review: these merged without compatibility notes.
…g changes Add release notes for the rename batch (apache#2308, apache#2314, apache#2315, apache#2316, apache#2317, apache#2319, apache#2322, apache#2323) and for apache#2067, apache#2290, apache#2294, apache#2307, apache#2310 and apache#2313. Remove stray blank lines inside the compatibility-notes list.
What
Use one error-returning config file loader.
Why
The old
LoadConfighides read errors for explicitly selected files.Implementation
LoadConfigreturn([]byte, error)and removeLoadConfigFile.EnvConfigbest-effort.Breaking change:
config.LoadConfignow returns an error, andLoadConfigFileis removed.