fix(config): seed --catalog-name from default-catalog in config file by rasta-rocket · Pull Request #1319 · apache/iceberg-go

rasta-rocket · 2026-06-26T10:25:16Z

Problem

The CLI --catalog-name flag defaults to "default". When a config file used a different name under default-catalog, that value was never read ‚ ParseConfig was called with the flag's default before the config file had any chance to influence it. The result was a silent fallback to the rest catalog type with no URI, producing a confusing error:

Get "v1/config": unsupported protocol scheme ""

Example config that triggered the bug:

default-catalog: my-catalog
catalog:
  my-catalog:
    type: glue
    warehouse: s3://my-bucket

Running iceberg list would fail because my-catalog was never found in the catalog map, mergeConf was never called, and the catalog type stayed as rest.

Fix

Add config.ParseDefaultCatalog to extract default-catalog from the YAML without requiring a catalog name upfront.
In main, load the config bytes once, then seed args.CatalogName from default-catalog when --catalog-name was not explicitly passed on the command line. The explicit-flag guard ensures CLI flags still take precedence.

How to test

Create ~/.iceberg-go.yaml with a non-"default" catalog name:

default-catalog: my-glue
catalog:
  my-glue:
    type: glue
    warehouse: s3://my-bucket

Run without --catalog-name:

AWS_PROFILE=my-profile AWS_REGION=my-region iceberg list

Before: Get "v1/config": unsupported protocol scheme ""
After: connects to the Glue catalog correctly.
Verify explicit flag still wins:

iceberg --catalog-name other-catalog list

Should use other-catalog, not my-glue.

When a config file set `default-catalog` to a non-"default" name, the CLI would silently ignore it because ParseConfig was called with the flag's default value of "default" before the config was consulted. Add ParseDefaultCatalog and apply it in main before the catalog lookup, so the config file's default-catalog seeds args.CatalogName when the flag is not explicitly passed on the command line.

zeroshade

LGTM. Precedence is right: explicit --catalog-name wins, then the config file's default-catalog, then the built-in default; the nil-config and empty-default-catalog cases resolve correctly too. A couple of small nits inline, none blocking.

zeroshade · 2026-06-26T17:10:44Z

 	return file
 }

+func ParseDefaultCatalog(file []byte) string {


nit: this unmarshals the same bytes that ParseConfig re-unmarshals on the very next line - two parses of the same config per run. Harmless for a CLI, but you could return both the *CatalogConfig and the default-catalog name from a single parse and drop this helper. Also, since it's newly exported, a one-line doc comment would be good.

zeroshade · 2026-06-26T17:10:44Z


-	fileCfg := config.ParseConfig(config.LoadConfig(args.Config), args.CatalogName)
+	configData := config.LoadConfig(args.Config)
+	if !explicitFlags["catalog-name"] {


nit: this seeding path (seed from default-catalog when the flag is absent, explicit flag wins) is exactly what the PR fixes, but it isn't covered by a test. A small args_test.go case asserting args.CatalogName is seeded when --catalog-name is omitted (and left alone when it's explicit) would guard against regressions.

rasta-rocket · 2026-06-28T14:49:43Z

Thanks @zeroshade for the review! I addressed the doc-comment nit and refactored to a single yaml.Unmarshal by folding ParseDefaultCatalog into ParseConfig: an empty catalogName now resolves via the file's default-catalog field, then falls back to the built-in "default".

For the args_test.go coverage you mentioned: the precedence logic (explicit flag wins, else pass "" to let ParseConfig resolve from the file) currently lives inline in main() and isn't reachable from a test. To cover it, I'd need to extract those few lines into a named helper. Happy to do that, do you have a name in mind? I was thinking something like catalogConfigFromFile or resolveCatalogConfig, but wanted your preference before adding a new exported/unexported symbol.

laskoviymishka

Nice catch on the root cause — seeding --catalog-name from default-catalog before ParseConfig runs is exactly the right fix for the empty-URI chicken-and-egg, and the !explicitFlags["catalog-name"] guard gets the precedence right (explicit flag beats config beats the "default" fallback).

The main thing I'd want before merge is a test — the precedence logic is the whole substance of the change and there's nothing covering it. A future refactor of the explicitFlags scan or the init ordering in main() could silently reintroduce the original bug with nothing to catch it, and the new exported ParseDefaultCatalog has no coverage either. Pulling the seeding into a small testable helper and covering all three tiers would close that.

The other thing worth fixing in the same pass is the silent error swallow in ParseDefaultCatalog: a malformed config returns "", falls through, and lands the user on the exact same unsupported protocol scheme "" error this PR is meant to eliminate — so a YAML typo is indistinguishable from the bug we just fixed. Logging or returning the error closes that.

The double-unmarshal and the naming are smaller — left a note inline about folding both parses through a shared parseFullConfig helper, which would also tidy the naming asymmetry. Not blocking. Thanks for chasing this one down.

laskoviymishka · 2026-06-29T09:52:06Z

 	return file
 }

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


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.

laskoviymishka · 2026-06-29T09:52:06Z


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


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?

laskoviymishka · 2026-06-29T09:52:06Z

 	var config Config
-	err := yaml.Unmarshal(file, &config)
-	if err != nil {
+	if err := yaml.Unmarshal(file, &config); err != nil {


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.

laskoviymishka · 2026-06-29T09:52:06Z

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


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.

laskoviymishka · 2026-06-29T09:52:06Z

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


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.

zeroshade · 2026-06-29T16:57:06Z

This currently has merge conflicts with main — could you rebase onto the latest main? (There's also an outstanding change-request review to resolve before it can merge.) Thanks @rasta-rocket!

rasta-rocket requested a review from zeroshade as a code owner June 26, 2026 10:25

rasta-rocket force-pushed the fix_catalog_name_cfg branch from d0f30d8 to f93dd0d Compare June 26, 2026 10:46

zeroshade approved these changes Jun 26, 2026

View reviewed changes

apply PR feedbacks

e0c19e2

rasta-rocket force-pushed the fix_catalog_name_cfg branch from ff3d142 to e0c19e2 Compare June 28, 2026 14:53

laskoviymishka requested changes Jun 29, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

fix(config): seed --catalog-name from default-catalog in config file#1319
rasta-rocket wants to merge 2 commits into
apache:mainfrom
rasta-rocket:fix_catalog_name_cfg

rasta-rocket commented Jun 26, 2026

Uh oh!

zeroshade left a comment

Uh oh!

zeroshade Jun 26, 2026

Uh oh!

zeroshade Jun 26, 2026

Uh oh!

rasta-rocket commented Jun 28, 2026

Uh oh!

laskoviymishka left a comment

Uh oh!

laskoviymishka Jun 29, 2026

Uh oh!

laskoviymishka Jun 29, 2026

Uh oh!

laskoviymishka Jun 29, 2026

Uh oh!

laskoviymishka Jun 29, 2026

Uh oh!

laskoviymishka Jun 29, 2026

Uh oh!

zeroshade commented Jun 29, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

rasta-rocket commented Jun 26, 2026

Problem

Fix

How to test

Uh oh!

zeroshade left a comment

Choose a reason for hiding this comment

Uh oh!

zeroshade Jun 26, 2026

Choose a reason for hiding this comment

Uh oh!

zeroshade Jun 26, 2026

Choose a reason for hiding this comment

Uh oh!

rasta-rocket commented Jun 28, 2026

Uh oh!

laskoviymishka left a comment

Choose a reason for hiding this comment

Uh oh!

laskoviymishka Jun 29, 2026

Choose a reason for hiding this comment

Uh oh!

laskoviymishka Jun 29, 2026

Choose a reason for hiding this comment

Uh oh!

laskoviymishka Jun 29, 2026

Choose a reason for hiding this comment

Uh oh!

laskoviymishka Jun 29, 2026

Choose a reason for hiding this comment

Uh oh!

laskoviymishka Jun 29, 2026

Choose a reason for hiding this comment

Uh oh!

zeroshade commented Jun 29, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants