feat(registry): configurable driver registries via TOML config#360
feat(registry): configurable driver registries via TOML config#360
Conversation
5a8bf36 to
5ac1ddd
Compare
5ac1ddd to
0813c82
Compare
Add RegistryEntry and GlobalConfig structs with TOML tags, and loadGlobalConfig() function that parses config.toml from a directory, validates URLs with url.Parse (not mustParseURL), and returns nil,nil for missing files. 7 subtests cover valid configs, missing file, invalid URL, empty URL, and empty registries.
- Add SetProjectRegistries() to dbc package for project-level registry merging - Snapshot defaultRegistries in ConfigureRegistries for proper re-merge ordering - Inject SetProjectRegistries in sync driversListMsg handler before getDriverRegistry - Pre-load dbc.toml in add Init() to set project registries before getDriverRegistry - Add tests: TestSyncWithProjectRegistries, TestSyncWithProjectRegistriesBackwardCompat - Add tests: TestAddWithProjectRegistries, TestAddWithProjectRegistriesBackwardCompat, TestAddWithInvalidProjectRegistryURL
…leanup - Add u.Host == "" check after url.Parse in loadGlobalConfig and SetProjectRegistries to catch hostless URLs like "my-registry" - Add TestSetProjectRegistries with 6 subtests covering merge precedence, replace_defaults, DBC_BASE_URL no-op, and error cases - Update TestConfigureRegistries subtests to save/restore defaultRegistries and globalConfig alongside registries - Use defer preF.Close() in add.go instead of manual close in two branches
0813c82 to
79e6f5c
Compare
…ment toRegistry invariant
…ional SetProjectRegistries, integration test
…egration test baseline
…ad toRegistry check, strengthen test
…y state propagation
…est cleanup patterns
…hen wiring tests, fix test isolation
… wiring tests, fix origReg copy
…assumption in add/sync
…location in ConfigureRegistries
…origReg/origDefault in test
79e6f5c to
40e5ad2
Compare
amoeba
left a comment
There was a problem hiding this comment.
I did a quick code review over the new pieces and left some comments. Even if we don't have an immediate use case for this, I think having it in dbc is useful.
Some other more general thoughts:
- Would we want CLI integration for this? I think it would be useful for dbc to be able to do all the work to show/add/remove registries from the project and user config.
- I didn't see any tests for how
dbc installwill work with this. Presumably dbc install will respect the new fields in the user config. - Do we need to define a default priority order for registries to handle name conflicts? We have a lot of the same concerns as conda does with channels. I'm wishing for more tests in this PR for the interaction between the default, user, and project registries around name conflicts.
|
|
||
| func SetProjectRegistries(entries []RegistryEntry, replaceDefaults *bool) error { | ||
| if os.Getenv("DBC_BASE_URL") != "" { | ||
| return nil |
There was a problem hiding this comment.
I think we should warn the user here.
| return nil | ||
| } | ||
| if replaceDefaults != nil && *replaceDefaults && len(entries) == 0 { | ||
| return fmt.Errorf("replace_defaults = true requires at least one [[registries]] entry; omit replace_defaults or add a registry entry") |
There was a problem hiding this comment.
I feel like this error would be better surfaced earlier, such as when we initially parse config.toml or dbc.toml. How this is now, I think this would error like this,
error configuring project registries: replace_defaults = true requires at least one [[registries]] entry; omit replace_defaults or add a registry entry
As a user, it's unclear what I should do to fix it. Which file is my problem in?
If we validated this earlier, I bet we'd have enough context to know which file has the issue and we could tell them.
| } | ||
| for _, e := range entries { | ||
| if e.URL == "" { | ||
| return fmt.Errorf("registry entry has empty url") |
There was a problem hiding this comment.
Are these checks needed or is this the best place to handle this? I see loadGlobalConfig already does validation and that seems like a better place. i.e., I much prefer being able to know that any []RegistryEntry I might get is always valid so I'd rather error before creating a RegistryEntry.
| }) | ||
| } | ||
|
|
||
| func boolPtr(b bool) *bool { return &b } |
There was a problem hiding this comment.
is this the best place for this def?
| } | ||
|
|
||
| if configDir, err := internal.GetUserConfigPath(); err != nil { | ||
| fmt.Fprintf(os.Stderr, "warning: failed to locate config directory: %v\n", err) |
There was a problem hiding this comment.
Do we really want to emit a warning? Won't this warn if the folder isn't found? Seems like we should be okay with that.
The warning when the registry config fails to load once found is fine.
|
|
||
| # Sisyphus workflow artifacts (plans, evidence, notepads) | ||
| .sisyphus/ |
There was a problem hiding this comment.
is this just a tool you use? can we revert this?
| if configDir, err := internal.GetUserConfigPath(); err != nil { | ||
| fmt.Fprintf(os.Stderr, "warning: failed to locate config directory: %v\n", err) | ||
| } else if err := dbc.ConfigureRegistries(configDir); err != nil { | ||
| fmt.Fprintf(os.Stderr, "warning: failed to load registry config: %v\n", err) |
There was a problem hiding this comment.
I don't think the error here includes the file with the issue. Whenever we show an error about a TOML file, we need to show the user the path to it.
| if u.Scheme != "http" && u.Scheme != "https" { | ||
| return Registry{}, false |
There was a problem hiding this comment.
Is the validation in here even needed? In the parent call, we already error when a RegistryEntry is invalid.
Summary
Adds support for configuring driver registries via TOML config files, allowing users to add custom registries at both the global (
~/.config/columnar/dbc/config.toml) and project (dbc.toml) level.What's New
~/.config/columnar/dbc/config.toml): loaded on every command viaConfigureRegistries()called inmain()dbc.toml):[[registries]]section respected bysyncandaddcommandsreplace_defaults = true: opt-in flag to suppress built-in registries entirelyDBC_BASE_URLenv var: still overrides everything (unchanged behavior)dbc.tomlfiles without[[registries]]work unchangedConfig Format
Global (
~/.config/columnar/dbc/config.toml):Project (
dbc.toml):Implementation Notes
http/httpsscheme and non-empty hostdefaultRegistriesis snapshotted eagerly ininit()(afterdrivers.goinit runs); env-sensitivity documented in commentsadd.godecodesdbc.tomlonce (single file open) for both registry config and driver listremoveintentionally omits registry wiring — it never queries a registry