laskoviymishka commented on code in PR #1319:
URL: https://github.com/apache/iceberg-go/pull/1319#discussion_r3490791485
##########
config/config.go:
##########
@@ -69,12 +69,23 @@ func LoadConfig(configPath string) []byte {
return file
}
+// ParseConfig unmarshals the config file once and resolves the catalog to use.
+// When catalogName is empty, it falls back to the file's default-catalog
field and
+// then to the built-in "default" name. It returns the matching CatalogConfig,
or
Review Comment:
This swallows the unmarshal error and returns `""`, which then falls through
to `ParseConfig` (also returning nil on the same malformed bytes) and lands the
user back on `unsupported protocol scheme ""` — the exact error this PR is
fixing.
So a YAML typo becomes indistinguishable from the original bug. I'd at
minimum log the error here, or better return `(string, error)` so the caller in
`main()` can surface "config failed to parse" instead of letting it masquerade
as a missing catalog. wdyt?
##########
config/config.go:
##########
@@ -69,12 +69,23 @@ func LoadConfig(configPath string) []byte {
return file
}
+// ParseConfig unmarshals the config file once and resolves the catalog to use.
Review Comment:
This is a new exported function with no test, and `TestParseConfig` doesn't
cover `default-catalog` either.
I'd add a small table-driven `TestParseDefaultCatalog` parallel to the
existing one: nil input, malformed YAML, `default-catalog` absent, and
`default-catalog: my-catalog`. All four return paths matter here since they
each feed the precedence decision.
##########
cmd/iceberg/main.go:
##########
@@ -236,7 +236,14 @@ func main() {
}
}
- fileCfg := config.ParseConfig(config.LoadConfig(args.Config),
args.CatalogName)
+ configData := config.LoadConfig(args.Config)
+ // Pass the catalog name only when it was set explicitly on the command
line; otherwise
+ // pass "" so ParseConfig resolves it from the file's default-catalog.
Explicit wins.
Review Comment:
The precedence behavior here — `default-catalog` seeds `args.CatalogName`
only when the flag wasn't passed, explicit flag wins — is the whole point of
the fix, and it's untested.
I'd pull the seeding into a testable helper, something like
`resolveCatalogName(args Args, explicitFlags map[string]bool, configData
[]byte) string`, and cover all three tiers: explicit flag, config
`default-catalog`, and the bare `"default"` fallback. Otherwise an
init-ordering or scanner refactor can quietly bring the bug back.
##########
cmd/iceberg/main.go:
##########
@@ -236,7 +236,14 @@ func main() {
}
}
- fileCfg := config.ParseConfig(config.LoadConfig(args.Config),
args.CatalogName)
+ configData := config.LoadConfig(args.Config)
+ // Pass the catalog name only when it was set explicitly on the command
line; otherwise
+ // pass "" so ParseConfig resolves it from the file's default-catalog.
Explicit wins.
+ lookupName := ""
+ if explicitFlags["catalog-name"] {
+ lookupName = args.CatalogName
+ }
Review Comment:
One gap worth a thought: if `default-catalog: prod` is set but no `prod`
entry exists in the map, `ParseConfig` returns nil here, `mergeConf` is
skipped, and we're back to the empty-URI error — except now it's more
surprising because we did pick up the user's value and then silently produced
nothing.
A warning like `catalog %q (from default-catalog) not found in config` would
make that case debuggable. Could fold into the same error-handling cleanup as
the swallow above.
##########
config/config.go:
##########
@@ -69,12 +69,23 @@ func LoadConfig(configPath string) []byte {
return file
}
+// ParseConfig unmarshals the config file once and resolves the catalog to use.
+// When catalogName is empty, it falls back to the file's default-catalog
field and
+// then to the built-in "default" name. It returns the matching CatalogConfig,
or
+// nil if no such catalog is defined in the file.
func ParseConfig(file []byte, catalogName string) *CatalogConfig {
var config Config
- err := yaml.Unmarshal(file, &config)
- if err != nil {
+ if err := yaml.Unmarshal(file, &config); err != nil {
Review Comment:
This unmarshals the full `Config` and discards everything but one field,
then `ParseConfig` unmarshals the same bytes again right after. Beyond the
redundancy, the two parses can drift if `ParseConfig` is ever restructured.
I'd factor out a shared `parseFullConfig(file []byte) (*Config, error)` that
both call — then this reads `.DefaultCatalog` and `ParseConfig` reads
`.Catalogs[name]`, one parse, consistent error surface. While we're here, the
name reads oddly next to `ParseConfig` (which returns a catalog entry, not a
name) — `ParseDefaultCatalogName` would say what it does, and since the only
caller is the CLI it could even be unexported. Not blocking, but the shared
helper feels worth it.
--
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]
