Skip to content

fix(config): seed --catalog-name from default-catalog in config file #1319

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
rasta-rocket wants to merge 2 commits into
base: main
Choose a base branch
from
+57 −6
Open

fix(config): seed --catalog-name from default-catalog in config file #1319

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f93dd0d
fix(config): seed --catalog-name from default-catalog in config file
rasta-rocket Jun 26, 2026
e0c19e2
apply PR feedbacks
rasta-rocket Jun 28, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion cmd/iceberg/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

@laskoviymishka laskoviymishka Jun 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

lookupName := ""
if explicitFlags["catalog-name"] {
lookupName = args.CatalogName
}

@laskoviymishka laskoviymishka Jun 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

fileCfg := config.ParseConfig(configData, lookupName)
if fileCfg != nil {
mergeConf(fileCfg, &args, explicitFlags)
}
Expand Down
15 changes: 13 additions & 2 deletions config/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,12 +69,23 @@ func LoadConfig(configPath string) []byte {
return file
}

// ParseConfig unmarshals the config file once and resolves the catalog to use.

@laskoviymishka laskoviymishka Jun 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// 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

@laskoviymishka laskoviymishka Jun 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

// 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 {

@laskoviymishka laskoviymishka Jun 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return nil
}

if catalogName == "" {
catalogName = config.DefaultCatalog
}
if catalogName == "" {
catalogName = "default"
}

res, ok := config.Catalogs[catalogName]
if !ok {
return nil
Expand Down
39 changes: 36 additions & 3 deletions config/config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,9 @@ var testArgs = []struct {
catName string
expected *CatalogConfig
}{
// config file does not exist
{nil, "default", nil},
// config does not have default catalog
// config file does not exist; empty name falls back to built-in "default"
{nil, "", nil},
// config does not have requested catalog
{[]byte(`
catalog:
custom-catalog:
Expand Down Expand Up @@ -102,6 +102,39 @@ catalog:
},
},
},
// empty name resolves via the file's default-catalog field
{
[]byte(`
default-catalog: custom-catalog
catalog:
custom-catalog:
type: rest
uri: http://localhost:8181/
warehouse: my_wh
`), "",
&CatalogConfig{
CatalogType: "rest",
URI: "http://localhost:8181/",
Warehouse: "my_wh",
},
},
// explicit name takes precedence over the file's default-catalog
{
[]byte(`
default-catalog: custom-catalog
catalog:
custom-catalog:
type: rest
uri: http://localhost:8181/
other:
type: rest
uri: http://other:8181/
`), "other",
&CatalogConfig{
CatalogType: "rest",
URI: "http://other:8181/",
},
},
}

func TestParseConfig(t *testing.T) {
Expand Down
Loading