Repository navigation
Conversation
Signed-off-by: Minh Vu <vuhoangminh97@gmail.com>
Signed-off-by: Minh Vu <vuhoangminh97@gmail.com>
Add coverage for registries sharing a type and zero-value registration. Clarify that named catalog configuration remains process-wide. Signed-off-by: Minh Vu <vuhoangminh97@gmail.com>
zeroshade
left a comment
zeroshade
left a comment
There was a problem hiding this comment.
The registry itself looks right: it's concurrency-safe, factories are invoked without the lock, invalid registrations return errors, and go test -race ./catalog/ passes on a merge with current main. But #2291 and #2296 merged today with the shared registry model @laskoviymishka asked for, and this one diverges in three places (inline). Could you align it so all three registries behave the same? Duplicate-replace semantics can stay, since that's what catalog.Register has always done.
| // NewRegistry returns an empty registry. Importing a catalog backend registers | ||
| // it with the package-level default, not with independently created registries. | ||
| // Use CloneDefaultRegistry to start with those registrations. | ||
| func NewRegistry() *Registry { |
There was a problem hiding this comment.
io.NewRegistry() and encryption.NewKMSRegistry() both return a snapshot of the default registry, including backends registered by side-effect imports. Returning an empty registry here means reg.Load(ctx, "rest", ...) fails unless the caller knows to use CloneDefaultRegistry(). Could NewRegistry() clone the default like the other two, and drop CloneDefaultRegistry?
| } | ||
|
|
||
| // GetRegisteredCatalogs lists catalog types in sorted order. | ||
| func (r *Registry) GetRegisteredCatalogs() []string { |
There was a problem hiding this comment.
The other two registries call this Names(). Could the method be Names() too? The package-level GetRegisteredCatalogs() can stay as it is.
| // Register adds a factory to the process-wide default registry. An existing | ||
| // catalog type is replaced. Invalid registrations return an error instead of | ||
| // panicking. Use MustRegister for package initialization. | ||
| func Register(catalogType string, reg Registrar) error { |
There was a problem hiding this comment.
io.Register and encryption.RegisterKMS kept their signatures (still panicking) and added MustRegister, so they aren't breaking. Making package-level catalog.Register return an error is the only breaking part of this PR. Could it keep its old behavior, with errors only from (*Registry).Register?
zeroshade
left a comment
zeroshade
left a comment
There was a problem hiding this comment.
Thanks @fallintoplace, all three points are addressed, and the catalog registry now matches io and encryption: NewRegistry() snapshots the default (including side-effect-registered backends), Clone/Register/MustRegister/Unregister/Names/Load on the registry, and package-level Register keeps its old panicking behavior. CI is green, and go test -race ./catalog/... passes on a merge with current main.
One thing before merge: since nothing is breaking anymore, could you drop the ! from the title and update the description? It still says "catalog.Register now returns an error". The squash commit should read refactor(catalog): support isolated catalog registries.
zeroshade
changed the title
What
Add isolated registries for catalog factories.
Why
Avoid sharing catalog registrations across applications and tests.
Implementation
catalog.Registrywith its ownLoadmethod.NewRegistry()returns a snapshot of the default registry, matchingio.NewRegistry()andencryption.NewKMSRegistry().(*Registry).Registerreturns an error for invalid registrations;MustRegisterpanics. Built-in catalogs usecatalog.MustRegister.catalog.Register,Unregister,GetRegisteredCatalogsandLoadkeep their existing behavior and use the default registry.Not a breaking change: the package-level API is unchanged.
Related: #2291 and #2296 for IO and KMS registries.