fix(database): set default .dingo data dir #1088
Conversation
📝 WalkthroughWalkthroughAdds programmatic plugin configuration and test isolation support. New public APIs in the plugin package: Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (14)
🚧 Files skipped from review as they are similar to previous changes (7)
🧰 Additional context used🧬 Code graph analysis (4)database/plugin/plugin.go (2)
database/database.go (2)
internal/integration/integration_test.go (3)
database/plugin/plugin_test.go (3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
🔇 Additional comments (14)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
wolf31o2
force-pushed
the
fix/default-data-dir
branch
from
f681a6f to
13d0a67
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
database/plugin/plugin_test.go (1)
12-29: Good initial test coverage; consider adding a few more cases.The tests cover the core scenarios. Consider adding tests for:
PluginTypeBlobwith badger plugin (since it's imported)- Plugin not found error case (e.g.,
SetPluginOption(PluginTypeMetadata, "nonexistent", ...))- Uint option handling (e.g., badger's
block-cache-size)Also note that this test mutates global plugin state (
cmdlineOptions.dataDir). If tests run in parallel, consider resetting the value in a cleanup function.database/database.go (1)
136-144: Process‑wide side effects fromSetPluginOptioninNewWiring
DataDirviaplugin.SetPluginOptionis logically correct and matches the documented semantics (nil config →.dingo, empty string → in‑memory, non‑empty → explicit path). However,SetPluginOptionmutates global plugin option state, so eachNewcall updates process‑wide settings for the selected blob/metadata plugins.If callers ever construct multiple
Databaseinstances with differentDataDirvalues in the same process (or concurrently), they’ll be sharing and overwriting these global options. That’s probably fine for the current CLI/config usage, but it’d be good to either (a) document thatDataDiris effectively process‑wide and not concurrency‑safe, or (b) in the longer term, movedata-dirto per‑instance options passed intoblob.New/metadata.Newinstead of via global plugin state.internal/integration/integration_test.go (2)
18-18: YAML‑based config generation is cleaner; consider temp paths as a minor follow‑upBuilding the config via a
map[string]anyandyaml.Marshalis a nice improvement over hand‑written YAML, and the checks againstconfig.DefaultBlobPlugin/config.DefaultMetadataPluginvalidate the important parts of the loaded config without depending on formatting.One minor consideration: the hard‑coded
/tmp/test-config-badgerand/tmp/test-config-sqlite.dbpaths are a bit Unix‑specific. Since the test never actually instantiates plugins from this config, it’s not a correctness issue today, but if you later reuse this config for real plugin creation, switching those tot.TempDir()oros.TempDir()(and deriving filenames from that) would make the test more portable.Also applies to: 199-221, 230-230, 242-243, 245-245, 247-248
268-268: Switch toconfig.DefaultBlobPluginkeeps switching test aligned with configUsing
config.DefaultBlobPlugininfindPluginEntryavoids the literal"badger"here and keeps this test consistent with the configured default blob plugin name. The assertion message still mentions “badger”, but that’s only cosmetic and can be adjusted later if the default ever changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
PLUGIN_DEVELOPMENT.md(1 hunks)database/database.go(2 hunks)database/plugin/blob/badger/plugin.go(2 hunks)database/plugin/metadata/sqlite/plugin.go(2 hunks)database/plugin/plugin.go(1 hunks)database/plugin/plugin_test.go(1 hunks)internal/integration/integration_test.go(10 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
database/plugin/plugin_test.go (2)
database/plugin/plugin.go (1)
SetPluginOption(71-151)database/plugin/register.go (1)
PluginTypeMetadata(27-27)
database/plugin/plugin.go (2)
database/plugin/register.go (2)
PluginType(24-24)PluginTypeName(31-40)database/plugin/option.go (4)
PluginOptionTypeString(29-29)PluginOptionTypeBool(30-30)PluginOptionTypeInt(31-31)PluginOptionTypeUint(32-32)
database/database.go (2)
database/plugin/plugin.go (1)
SetPluginOption(71-151)database/plugin/register.go (2)
PluginTypeBlob(28-28)PluginTypeMetadata(27-27)
internal/integration/integration_test.go (3)
database/plugin/register.go (3)
GetPlugin(111-120)PluginTypeBlob(28-28)PluginTypeMetadata(27-27)internal/config/config.go (2)
DefaultBlobPlugin(50-50)DefaultMetadataPlugin(51-51)database/plugin/plugin.go (1)
SetPluginOption(71-151)
🪛 LanguageTool
PLUGIN_DEVELOPMENT.md
[style] ~294-~294: ‘prior to’ might be wordy. Consider a shorter alternative.
Context: ...'s destination variable in the registry prior to plugin instantiation. If the requested ...
(EN_WORDINESS_PREMIUM_PRIOR_TO)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (17)
- GitHub Check: go-test (1.24.x, windows-latest)
- GitHub Check: go-test (1.25.x, macos-latest)
- GitHub Check: go-test (1.25.x, windows-latest)
- GitHub Check: go-test (1.24.x, ubuntu-latest)
- GitHub Check: go-test (1.24.x, macos-latest)
- GitHub Check: nilaway
- GitHub Check: docker (ubuntu-latest, amd64)
- GitHub Check: lint
- GitHub Check: Analyze (go)
- GitHub Check: go-test (1.24.x, macos-latest)
- GitHub Check: go-test (1.24.x, ubuntu-latest)
- GitHub Check: go-test (1.25.x, windows-latest)
- GitHub Check: go-test (1.24.x, windows-latest)
- GitHub Check: lint
- GitHub Check: Analyze (go)
- GitHub Check: docker (ubuntu-latest, amd64)
- GitHub Check: nilaway
🔇 Additional comments (5)
PLUGIN_DEVELOPMENT.md (1)
281-299: Good documentation for the new programmatic option override feature.The documentation clearly explains how to use
SetPluginOptionfor tests, includes practical examples, and appropriately notes the non-fatal behavior for undefined options. The recommendation to uset.TempDir()for test isolation is valuable guidance.database/plugin/metadata/sqlite/plugin.go (1)
34-34: Default data directory now set to ".dingo" - consistent implementation.The default value is defined in two places:
initCmdlineOptions()for the runtime default andDefaultValuefor the plugin option metadata. Both are correctly set to.dingo.Also applies to: 51-51
database/plugin/blob/badger/plugin.go (1)
46-46: Both plugins safely share the ".dingo" directory through distinct storage patterns.The Badger blob plugin creates a
blobsubdirectory within the data directory, while SQLite stores its data asmetadata.sqlitewithin the same directory. These non-overlapping storage locations prevent file conflicts or data corruption.internal/integration/integration_test.go (2)
44-71: Good use of config defaults and per‑test data‑dirs for plugin lifecycleSwitching to
config.DefaultBlobPlugin/config.DefaultMetadataPluginhere keeps the tests aligned with the central configuration, and theSetPluginOption(..., "data-dir", t.TempDir())calls beforeStart()give each run an isolated on‑disk location. This avoids pollution of the new.dingodefault and reduces flakiness from shared state. No issues spotted.
140-178: Lifecycle test updates correctly respect defaults and isolate storageUsing
expectedBlobs := []string{config.DefaultBlobPlugin, "gcs", "s3"}andexpectedMetadata := []string{config.DefaultMetadataPlugin}keeps this test resilient to future renames of the default plugins. The addedSetPluginOption(..., "data-dir", t.TempDir())calls beforeGetPlugin/Startfor badger/sqlite correctly ensure that these “works with defaults” checks don’t trample the shared.dingodirectory. This all looks consistent with the new defaulting behavior.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
PLUGIN_DEVELOPMENT.md (1)
281-299: Good documentation for the new test pattern.Clear explanation of programmatic option overrides with practical examples. The guidance on using
t.TempDir()for test isolation is valuable.Minor nit: Consider "before" instead of "prior to" on line 294 for conciseness.
database/plugin/plugin_test.go (1)
11-29: Good basic test coverage for SetPluginOption.The test validates the three key behaviors: successful assignment, type mismatch error, and non-fatal handling of unknown options.
Consider adding test cases for:
- Plugin not found scenario (returns error)
- Bool, int, and uint option types
database/plugin/plugin.go (1)
71-151: Thread safety consideration for SetPluginOption.The function writes directly to
opt.Dest(e.g.,&cmdlineOptions.dataDir) without acquiring the plugin'scmdlineOptionsMutex. IfSetPluginOptionruns concurrently withNewFromCmdlineOptions(which reads underRLock), there's a potential data race.The current usage in
database.New()is safe becauseSetPluginOptioncompletes before plugin instantiation. However, for general-purpose use, consider documenting thatSetPluginOptionmust be called before any plugin instantiation, or explore acquiring the mutex if feasible.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
PLUGIN_DEVELOPMENT.md(1 hunks)database/database.go(2 hunks)database/plugin/blob/badger/plugin.go(2 hunks)database/plugin/metadata/sqlite/plugin.go(2 hunks)database/plugin/plugin.go(1 hunks)database/plugin/plugin_test.go(1 hunks)internal/integration/integration_test.go(10 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
database/database.go (2)
database/plugin/plugin.go (1)
SetPluginOption(71-151)database/plugin/register.go (2)
PluginTypeBlob(28-28)PluginTypeMetadata(27-27)
database/plugin/plugin_test.go (2)
database/plugin/plugin.go (1)
SetPluginOption(71-151)database/plugin/register.go (1)
PluginTypeMetadata(27-27)
internal/integration/integration_test.go (3)
database/plugin/register.go (1)
GetPlugin(111-120)internal/config/config.go (1)
DefaultBlobPlugin(50-50)database/plugin/plugin.go (1)
SetPluginOption(71-151)
database/plugin/plugin.go (2)
database/plugin/register.go (2)
PluginType(24-24)PluginTypeName(31-40)database/plugin/option.go (4)
PluginOptionTypeString(29-29)PluginOptionTypeBool(30-30)PluginOptionTypeInt(31-31)PluginOptionTypeUint(32-32)
🪛 LanguageTool
PLUGIN_DEVELOPMENT.md
[style] ~294-~294: ‘prior to’ might be wordy. Consider a shorter alternative.
Context: ...'s destination variable in the registry prior to plugin instantiation. If the requested ...
(EN_WORDINESS_PREMIUM_PRIOR_TO)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: go-test (1.24.x, windows-latest)
- GitHub Check: go-test (1.25.x, windows-latest)
- GitHub Check: go-test (1.24.x, ubuntu-latest)
- GitHub Check: go-test (1.24.x, macos-latest)
- GitHub Check: nilaway
- GitHub Check: Analyze (go)
- GitHub Check: go-test (1.25.x, windows-latest)
- GitHub Check: go-test (1.24.x, windows-latest)
- GitHub Check: Analyze (go)
🔇 Additional comments (5)
database/plugin/blob/badger/plugin.go (1)
46-46: LGTM! Consistent default data directory initialization.The default data directory is correctly set to ".dingo" in both
initCmdlineOptions()and theDefaultValuefor the plugin option, ensuring consistent behavior whether the plugin is initialized programmatically or via command-line options.Also applies to: 63-63
database/database.go (1)
136-144: Well-designed plugin option configuration.The
SetPluginOptioncalls correctly configure thedata-diroption before plugin instantiation. The approach of always setting the option (even when empty) is sound—empty strings trigger in-memory mode while non-empty paths enable persistence. Error handling for both calls is properly implemented.database/plugin/metadata/sqlite/plugin.go (1)
34-34: LGTM! Consistent with badger plugin defaults.The SQLite metadata plugin now defaults to ".dingo" in both
initCmdlineOptions()and theDefaultValue, maintaining consistency with the badger blob plugin.Also applies to: 51-51
internal/integration/integration_test.go (2)
157-178: Correct ordering in TestPluginLifecycleEndToEnd.Unlike
TestPluginSystemIntegration, this test correctly callsSetPluginOptionbeforeGetPlugin, ensuring the temp directory is used for plugin instantiation.
199-221: Good improvement: programmatic YAML generation.Using
yaml.Marshalinstead of a hardcoded YAML string is more robust and avoids formatting/indentation issues.
wolf31o2
force-pushed
the
fix/default-data-dir
branch
from
13d0a67 to
62aa4b3
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
database/plugin/plugin_test.go (1)
12-55: Good test coverage forSetPluginOption.The test covers the key scenarios: success cases for various types, type enforcement errors, no-op for unknown options, and error for nonexistent plugins.
Note: As the comment on lines 13-15 acknowledges, this test mutates global state. If parallelism is enabled in the future, consider using
t.Cleanupto reset options to their original values after each test to prevent cross-test interference.internal/integration/integration_test.go (1)
32-56: Consider logging or failing onSetPluginOptionerrors inTestMain.Discarding errors with
_ =on lines 42-48 and 48-53 could silently hide configuration problems that would cause confusing test failures later. Since this is test setup code, consider either:
- Logging the error (if the failure is acceptable for some plugin combinations), or
- Using
panicto fail fast if setting the data-dir is critical.- _ = plugin.SetPluginOption( + if err := plugin.SetPluginOption( plugin.PluginTypeBlob, config.DefaultBlobPlugin, "data-dir", tmpDir, - ) + ); err != nil { + // Log but don't fail - some plugins may not support data-dir + fmt.Fprintf(os.Stderr, "warning: failed to set blob data-dir: %v\n", err) + }connmanager/connection_manager.go (1)
55-63: PromRegistry addition is solid; no breaking changes detected, documentation optionalAdding
PromRegistry prometheus.RegisterertoConnectionManagerConfigcleanly exposes metric registration control. The nil check at line 85 (if cfg.PromRegistry != nil) correctly gates metric initialization, andpromauto.With(c.config.PromRegistry)at line 92 uses it properly.All existing
ConnectionManagerConfigliterals in the codebase (node.go, connection_manager_test.go) already use keyed field syntax, so positioning PromRegistry as the first field poses no risk of field mis-assignment.Optional: Consider adding a doc comment to
PromRegistrydocumenting its semantics (e.g., "nil disables metrics; non-nil enables Prometheus registration via promauto.With") for clarity to future callers, though the current implementation is self-documenting through its nil gate.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
PLUGIN_DEVELOPMENT.md(1 hunks)connmanager/connection_manager.go(2 hunks)database/database.go(3 hunks)database/plugin/blob/aws/database.go(1 hunks)database/plugin/blob/badger/database.go(2 hunks)database/plugin/blob/badger/plugin.go(2 hunks)database/plugin/blob/gcs/plugin_test.go(1 hunks)database/plugin/metadata/sqlite/database.go(2 hunks)database/plugin/metadata/sqlite/plugin.go(2 hunks)database/plugin/plugin.go(1 hunks)database/plugin/plugin_test.go(1 hunks)database/plugin/register.go(1 hunks)internal/integration/benchmark_test.go(3 hunks)internal/integration/integration_test.go(10 hunks)
✅ Files skipped from review due to trivial changes (1)
- database/plugin/blob/aws/database.go
🚧 Files skipped from review as they are similar to previous changes (4)
- database/plugin/blob/badger/plugin.go
- database/plugin/metadata/sqlite/plugin.go
- PLUGIN_DEVELOPMENT.md
- database/database.go
🧰 Additional context used
🧬 Code graph analysis (3)
database/plugin/plugin.go (2)
database/plugin/register.go (2)
PluginType(24-24)PluginTypeName(31-40)database/plugin/option.go (4)
PluginOptionTypeString(29-29)PluginOptionTypeBool(30-30)PluginOptionTypeInt(31-31)PluginOptionTypeUint(32-32)
database/plugin/plugin_test.go (2)
database/plugin/plugin.go (1)
SetPluginOption(78-235)database/plugin/register.go (2)
PluginTypeMetadata(27-27)PluginTypeBlob(28-28)
internal/integration/integration_test.go (3)
database/plugin/plugin.go (1)
SetPluginOption(78-235)database/plugin/register.go (1)
GetPlugin(115-124)internal/config/config.go (2)
DefaultBlobPlugin(50-50)DefaultMetadataPlugin(51-51)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: go-test (1.24.x, windows-latest)
- GitHub Check: go-test (1.24.x, macos-latest)
- GitHub Check: go-test (1.25.x, macos-latest)
- GitHub Check: nilaway
- GitHub Check: go-test (1.25.x, windows-latest)
- GitHub Check: docker (ubuntu-latest, amd64)
- GitHub Check: Analyze (go)
🔇 Additional comments (16)
database/plugin/blob/gcs/plugin_test.go (1)
28-32: LGTM!The struct field reordering is a minor stylistic change with no functional impact. The test logic remains correct.
database/plugin/metadata/sqlite/database.go (2)
37-44: LGTM!Struct field reordering has no functional impact.
162-164: Good security improvement: Tightening directory permissions.Changing from
fs.ModePerm(0777) to0o755prevents world-write access to the data directory. This is a sensible default for file-backed storage.database/plugin/blob/badger/database.go (2)
33-44: LGTM!The struct field reorganization groups configuration-related fields together, improving readability.
79-81: Consistent permission tightening.Using
0o755instead offs.ModePermmatches the sqlite plugin change, ensuring consistent and secure directory creation across storage backends.database/plugin/register.go (1)
52-58: LGTM!The
Registerfunction is clean and well-documented. The explicit warning about thread-safety and initialization-only usage is helpful for maintainers.internal/integration/integration_test.go (3)
59-66: SetPluginOption ordering is correct.The
SetPluginOptioncalls now properly precedeGetPlugin, ensuring the temp directory is configured before plugin instantiation. This addresses the previous review concern.
80-91: Good use of config constants.Using
config.DefaultBlobPluginandconfig.DefaultMetadataPlugininstead of hard-coded strings improves maintainability and ensures tests stay in sync with default configuration.
239-300: Well-structured YAML config integration test.The approach of marshaling a config map to YAML avoids string formatting pitfalls and makes the test configuration more readable and maintainable.
database/plugin/plugin.go (4)
66-77: Thorough documentation of concurrency constraints.The function comments clearly explain the thread-safety limitations and when this function should be called. This addresses the previous review concern about synchronization.
94-123: Proper nil checks implemented.The type-checked assignment pattern with explicit nil checks for
opt.Destand the typed destination pointer addresses the previous review concern about potential nil pointer dereferences.
182-216: Good handling of uint64/int conversion.The
PluginOptionTypeUintcase properly accepts bothuint64andintvalues, with a bounds check to reject negative integers. This provides flexibility for callers while maintaining type safety.
224-234: Reasonable fallback behavior for unknown options.Treating an unknown option as a non-fatal no-op (when the plugin exists) allows callers to set options that may not apply to all plugin implementations without error handling complexity.
internal/integration/benchmark_test.go (1)
100-108: LGTM: explicit backend names wired through cleanlyThe added
name stringfield is consistently defined on the backend descriptor struct, initialized for all backends (memory, disk, GCS, S3), and used for sub‑benchmark naming and temp‑dir prefixes. No correctness or maintainability concerns from these changes.Also applies to: 135-138, 155-158
connmanager/connection_manager.go (2)
37-41: connectionInfo field order change appears safe
connectionInfois an unexported struct used only inside this package/file, so movingisInboundafterpeerAddrdoesn’t affect external APIs or behavior. No issues from this change.
48-53: Field reordering is safe; no API compatibility riskAll
ConnectionManagerinstantiation in the codebase occurs exclusively through theNewConnectionManagerfactory function, which uses keyed fields. No unkeyed struct literals exist, and external code only interacts withConnectionManagervia pointers. Reordering fields poses no breaking change to the API.Likely an incorrect or invalid review comment.
wolf31o2
force-pushed
the
fix/default-data-dir
branch
from
62aa4b3 to
6d3a638
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
database/plugin/plugin.go (1)
94-216: Consider extracting nil-check logic into a helper to reduce repetition.Each type case repeats the same pattern: check
opt.Dest == nil, type-assert, check result nil. While the current implementation is correct and readable, a helper function could reduce boilerplate:func checkDest[T any](opt PluginOption, optionName string) (*T, error) { if opt.Dest == nil { return nil, fmt.Errorf("nil destination for option %s", optionName) } dest, ok := opt.Dest.(*T) if !ok || dest == nil { return nil, fmt.Errorf("invalid destination type for option %s", optionName) } return dest, nil }However, this is a minor improvement and the current explicit approach is also acceptable.
database/plugin/plugin_test.go (2)
12-55: Test mutates global state without cleanup; consider restoring original values.The test modifies global plugin options (e.g.,
data-dirfor sqlite and badger) but doesn't restore original values afterward. This could cause test pollution if other tests depend on default values.Consider capturing and restoring the original values:
func TestSetPluginOption_SuccessAndTypeCheck(t *testing.T) { + // Store original values for cleanup + // (would require a GetPluginOption helper or direct field access) + // Note: This test mutates global plugin state...Alternatively, run this test in isolation or use
t.Cleanup()to restore state.
34-37: Uset.TempDir()instead of hardcoded/tmp/test-badgerfor portability.Hardcoded paths can fail on Windows and may cause collisions between test runs.
// Test setting data-dir for badger plugin (blob type) - if err := plug.SetPluginOption(plug.PluginTypeBlob, "badger", "data-dir", "/tmp/test-badger"); err != nil { + if err := plug.SetPluginOption(plug.PluginTypeBlob, "badger", "data-dir", t.TempDir()); err != nil { t.Fatalf("unexpected error setting badger data-dir: %v", err) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
PLUGIN_DEVELOPMENT.md(1 hunks)connmanager/connection_manager.go(2 hunks)database/database.go(3 hunks)database/plugin/blob/aws/database.go(1 hunks)database/plugin/blob/badger/database.go(2 hunks)database/plugin/blob/badger/plugin.go(2 hunks)database/plugin/blob/gcs/plugin_test.go(1 hunks)database/plugin/metadata/sqlite/database.go(2 hunks)database/plugin/metadata/sqlite/plugin.go(2 hunks)database/plugin/plugin.go(1 hunks)database/plugin/plugin_test.go(1 hunks)database/plugin/register.go(1 hunks)internal/integration/benchmark_test.go(3 hunks)internal/integration/integration_test.go(10 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- database/plugin/metadata/sqlite/plugin.go
- PLUGIN_DEVELOPMENT.md
- database/plugin/register.go
- database/plugin/metadata/sqlite/database.go
- database/plugin/blob/badger/plugin.go
- connmanager/connection_manager.go
- database/database.go
- internal/integration/benchmark_test.go
- database/plugin/blob/aws/database.go
- database/plugin/blob/badger/database.go
- database/plugin/blob/gcs/plugin_test.go
🧰 Additional context used
🧬 Code graph analysis (2)
database/plugin/plugin.go (2)
database/plugin/register.go (2)
PluginType(24-24)PluginTypeName(31-40)database/plugin/option.go (4)
PluginOptionTypeString(29-29)PluginOptionTypeBool(30-30)PluginOptionTypeInt(31-31)PluginOptionTypeUint(32-32)
internal/integration/integration_test.go (3)
database/plugin/plugin.go (1)
SetPluginOption(78-235)database/plugin/register.go (3)
PluginTypeBlob(28-28)PluginTypeMetadata(27-27)GetPlugin(115-124)internal/config/config.go (2)
DefaultBlobPlugin(50-50)DefaultMetadataPlugin(51-51)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: docker (ubuntu-latest, amd64)
- GitHub Check: go-test (1.24.x, macos-latest)
- GitHub Check: Analyze (go)
- GitHub Check: go-test (1.24.x, windows-latest)
- GitHub Check: go-test (1.25.x, windows-latest)
- GitHub Check: nilaway
🔇 Additional comments (4)
database/plugin/plugin.go (1)
66-235: LGTM! The implementation is thorough with proper type safety and nil checks.The function correctly implements:
- Type-checked assignments for all supported option types
- Nil destination validation before dereferencing
- Safe type assertions with explicit error handling
- Clear documentation of synchronization requirements
The previous review concerns about nil pointer dereferences have been properly addressed with the two-step validation pattern (checking
opt.Dest == nilfirst, then safe type assertion).internal/integration/integration_test.go (3)
62-120: LGTM! Plugin options are correctly set before instantiation.The test now properly calls
SetPluginOptionbeforeGetPlugin, ensuring plugins use the test-specific temp directories. This addresses the previous review concern about ordering.
171-237: LGTM! Lifecycle test properly configures plugins before instantiation.The test correctly sets
data-diroptions before retrieving plugin instances, ensuring isolated test directories.
239-308: LGTM! YAML config test is well-structured.Using
yaml.Marshalto generate the config file avoids potential YAML indentation issues that could occur with raw string literals.
wolf31o2
force-pushed
the
fix/default-data-dir
branch
from
6d3a638 to
836dfbb
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
database/plugin/plugin_test.go (1)
11-55: Test coverage looks comprehensive.The test covers the key scenarios: success, type mismatch, unknown options, and plugin-not-found. The warning about global state mutation is appropriate.
Consider using
t.TempDir()instead of the hardcoded/tmp/test-badgerpath on line 35 for better cross-platform compatibility and automatic cleanup:- if err := plug.SetPluginOption(plug.PluginTypeBlob, "badger", "data-dir", "/tmp/test-badger"); err != nil { + if err := plug.SetPluginOption(plug.PluginTypeBlob, "badger", "data-dir", t.TempDir()); err != nil {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
PLUGIN_DEVELOPMENT.md(1 hunks)connmanager/connection_manager.go(2 hunks)database/database.go(3 hunks)database/plugin/blob/aws/database.go(1 hunks)database/plugin/blob/badger/database.go(2 hunks)database/plugin/blob/badger/plugin.go(2 hunks)database/plugin/blob/gcs/plugin_test.go(1 hunks)database/plugin/metadata/sqlite/database.go(2 hunks)database/plugin/metadata/sqlite/plugin.go(2 hunks)database/plugin/plugin.go(1 hunks)database/plugin/plugin_test.go(1 hunks)database/plugin/register.go(1 hunks)internal/integration/benchmark_test.go(3 hunks)internal/integration/integration_test.go(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- database/plugin/metadata/sqlite/database.go
- PLUGIN_DEVELOPMENT.md
- database/database.go
- database/plugin/plugin.go
- database/plugin/blob/badger/database.go
- connmanager/connection_manager.go
- database/plugin/metadata/sqlite/plugin.go
- database/plugin/blob/gcs/plugin_test.go
- database/plugin/blob/aws/database.go
- internal/integration/benchmark_test.go
- database/plugin/blob/badger/plugin.go
🧰 Additional context used
🧬 Code graph analysis (2)
internal/integration/integration_test.go (2)
database/plugin/register.go (3)
PluginTypeBlob(28-28)PluginTypeMetadata(27-27)GetPlugin(115-124)internal/config/config.go (2)
DefaultBlobPlugin(50-50)DefaultMetadataPlugin(51-51)
database/plugin/plugin_test.go (2)
database/plugin/plugin.go (1)
SetPluginOption(78-235)database/plugin/register.go (2)
PluginTypeMetadata(27-27)PluginTypeBlob(28-28)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: docker (ubuntu-latest, amd64)
- GitHub Check: go-test (1.25.x, windows-latest)
- GitHub Check: go-test (1.24.x, ubuntu-latest)
- GitHub Check: go-test (1.25.x, macos-latest)
- GitHub Check: go-test (1.24.x, windows-latest)
- GitHub Check: go-test (1.24.x, macos-latest)
- GitHub Check: nilaway
- GitHub Check: Analyze (go)
🔇 Additional comments (5)
database/plugin/register.go (1)
52-58: LGTM!The thread-safety documentation is accurate and helpful. The guidance to call
Registeronly duringinit()functions aligns with the typical plugin registration pattern in Go.internal/integration/integration_test.go (4)
33-73: TestMain correctly handles cleanup before os.Exit.The implementation properly addresses the issue where
deferwouldn't execute withos.Exit. Capturing the return code, performing cleanup, logging cleanup errors without failing, and then exiting is the correct pattern.
76-108: Correct ordering: SetPluginOption before GetPlugin.The plugin options are now set before plugin instantiation, ensuring the temp directories are used when
NewFromOptionsFunc()is called.
261-283: Good approach: programmatic YAML generation avoids formatting pitfalls.Using
yaml.Marshalwith a map structure is more robust than raw YAML strings, avoiding indentation and formatting issues.
208-246: Consistent pattern: data-dir set before plugin instantiation.The test correctly sets temp directories before calling
GetPlugin, ensuring isolated test environments.
Signed-off-by: Chris Gianelloni <wolf31o2@blinklabs.io>
wolf31o2
force-pushed
the
fix/default-data-dir
branch
from
836dfbb to
930411a
Compare
Summary by cubic
Set the default data directory to ".dingo" for the Badger blob and SQLite metadata plugins, with programmatic overrides via SetPluginOption (used in database.New). This ensures predictable local storage and lets tests or configs choose per-run dirs or in-memory mode.
Written for commit 930411a. Summary will update automatically on new commits.
Summary by CodeRabbit
New Features
Documentation
Configuration
Tests
✏️ Tip: You can customize this high-level summary in your review settings.