laskoviymishka commented on code in PR #2294:
URL: https://github.com/apache/iceberg-go/pull/2294#discussion_r4368745663
##########
config/config.go:
##########
@@ -55,18 +55,11 @@ type CatalogConfig struct {
RestOptions *RestOptions `yaml:"rest,omitempty"`
}
-// LoadConfig returns configuration data when it can be read. It retains the
-// historical best-effort behavior; callers that must distinguish a missing
-// default from a broken explicit file should use LoadConfigFile.
-func LoadConfig(configPath string) []byte {
- data, _ := LoadConfigFile(configPath)
-
- return data
-}
-
-// LoadConfigFile loads configuration data and reports path and read failures.
-// A missing implicit home-directory configuration is not an error.
-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
+// the YAML and select a catalog.
+func LoadConfig(configPath string) ([]byte, error) {
Review Comment:
What tipped this toward the `LoadConfig` name over `LoadConfigFile`? Both
shipped in
[v0.7.0](https://github.com/apache/iceberg-go/blob/89a9104ec3639d38ab07e4461721cb5270e95571/config/config.go#L58-L69),
and keeping `LoadConfigFile` instead would break only `LoadConfig` callers and
leave the [CLI
call](https://github.com/apache/iceberg-go/blob/84c6fd693fe25bc9f6e3c9d99ef733930a6bc30f/cmd/iceberg/main.go#L874-L878)
untouched. Either name works for me; a line in the description on why would
settle it.
##########
config/config.go:
##########
@@ -132,7 +125,10 @@ func fromConfigFiles() Config {
}
var cfg Config
- 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)
Review 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](https://github.com/apache/iceberg-go/blob/61388426608d90e7b7833019e85fffee9ef7c84c/config/config.go#L67-L71)
as well.
##########
config/config.go:
##########
@@ -55,18 +55,11 @@ type CatalogConfig struct {
RestOptions *RestOptions `yaml:"rest,omitempty"`
}
-// LoadConfig returns configuration data when it can be read. It retains the
-// historical best-effort behavior; callers that must distinguish a missing
-// default from a broken explicit file should use LoadConfigFile.
-func LoadConfig(configPath string) []byte {
- data, _ := LoadConfigFile(configPath)
-
- return data
-}
-
-// LoadConfigFile loads configuration data and reports path and read failures.
-// A missing implicit home-directory configuration is not an error.
-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
Review Comment:
Should the doc also say what a missing implicit file returns, and that
`GOICEBERG_HOME` isn't consulted? `EnvConfig` [honors
it](https://github.com/apache/iceberg-go/blob/61388426608d90e7b7833019e85fffee9ef7c84c/config/config.go#L121-L131),
so a `LoadConfig("")` caller could expect the same.
```suggestion
// home-directory configuration is not an error; LoadConfig returns nil data.
// Unlike [EnvConfig], it ignores GOICEBERG_HOME. Use [ParseConfig] to decode
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
