index storage path in env #1274
Conversation
WalkthroughA new optional field Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant Clap
participant Options
participant App
User->>CLI: Run command with --index-storage-path
CLI->>Clap: Parse CLI arguments
Clap->>Options: Assign canonicalized path from flag/env var
Options->>Options: Check if index_storage_path is set
Options->>Options: Create directory if necessary
Options->>App: Supply configuration for indexing
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/cli.rs (1)
228-236: New option follows established pattern but consider adding a helper method.The implementation of
index_storage_pathoption follows the codebase's established patterns for path configuration, using the appropriate clap attributes and consistent naming conventions. The TODO comment clearly marks this as a temporary solution until smart cache is implemented.Consider adding a helper method similar to
staging_dir()that ensures the index directory exists:+ /// Path to index directory, ensures that it exists or returns the PathBuf + pub fn index_dir(&self) -> Option<&PathBuf> { + if let Some(path) = &self.index_storage_path { + fs::create_dir_all(path) + .expect("Should be able to create index directory if it doesn't exist"); + Some(path) + } else { + None + } + }This would provide a consistent API for accessing and ensuring the existence of the index directory, similar to how
staging_dir()works for the staging directory.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/cli.rs(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
src/cli.rs (1)
src/option.rs (1)
canonicalize_path(111-114)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Build Default x86_64-pc-windows-msvc
- GitHub Check: Build Default x86_64-unknown-linux-gnu
- GitHub Check: Build Default aarch64-unknown-linux-gnu
- GitHub Check: Quest Smoke and Load Tests for Distributed deployments
- GitHub Check: Build Kafka x86_64-unknown-linux-gnu
- GitHub Check: Quest Smoke and Load Tests for Standalone deployments
- GitHub Check: Build Kafka aarch64-apple-darwin
- GitHub Check: coverage
nikhilsinhaparseable
force-pushed
the
env-index-path
branch
from
474e72a to
477cc11
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/cli.rs (1)
430-439: The index_dir implementation looks good but could be more robust.The method correctly creates the directory if it doesn't exist and returns the path reference, following the pattern used by other similar methods like
staging_dir().Consider including the path in the error message to aid debugging:
- .expect("Should be able to create index directory if it doesn't exist"); + .unwrap_or_else(|err| panic!("Failed to create index directory at {:?}: {}", path, err));Also, you might want to check if the path exists but is not a directory:
pub fn index_dir(&self) -> Option<&PathBuf> { if let Some(path) = &self.index_storage_path { + if path.exists() && !path.is_dir() { + panic!("Index path exists but is not a directory: {:?}", path); + } fs::create_dir_all(path) .expect("Should be able to create index directory if it doesn't exist"); Some(path) } else { None } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/cli.rs(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: coverage
- GitHub Check: Build Kafka x86_64-unknown-linux-gnu
- GitHub Check: Build Default x86_64-pc-windows-msvc
- GitHub Check: Build Default aarch64-apple-darwin
- GitHub Check: Quest Smoke and Load Tests for Standalone deployments
- GitHub Check: Build Default x86_64-apple-darwin
- GitHub Check: Quest Smoke and Load Tests for Distributed deployments
- GitHub Check: Build Default aarch64-unknown-linux-gnu
- GitHub Check: Build Default x86_64-unknown-linux-gnu
- GitHub Check: Build Kafka aarch64-apple-darwin
🔇 Additional comments (1)
src/cli.rs (1)
228-235: LGTM: The implementation of the new index_storage_path option follows the codebase patterns.The new CLI option for index storage path is correctly implemented with appropriate environment variable mapping (P_INDEX_DIR) and value validation through the canonicalize_path parser.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/cli.rs (1)
431-440: Implementation of index_dir method looks good.The method correctly handles both cases - creating the directory when the path is specified, and returning None when it's not. The error handling is consistent with other similar methods like
staging_dir().For better error messaging consistency, consider matching the error message format with the one in
staging_dir():- .expect("Should be able to create index directory if it doesn't exist"); + .expect("Should be able to create dir if doesn't exist");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/cli.rs(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Build Default aarch64-apple-darwin
- GitHub Check: coverage
- GitHub Check: Build Default x86_64-pc-windows-msvc
- GitHub Check: Build Kafka x86_64-unknown-linux-gnu
- GitHub Check: Build Default x86_64-unknown-linux-gnu
- GitHub Check: Build Kafka aarch64-apple-darwin
- GitHub Check: Build Default aarch64-unknown-linux-gnu
- GitHub Check: Build Default x86_64-apple-darwin
- GitHub Check: Quest Smoke and Load Tests for Distributed deployments
- GitHub Check: Quest Smoke and Load Tests for Standalone deployments
🔇 Additional comments (1)
src/cli.rs (1)
228-236: New field added correctly for index storage path.This new configuration option allows specifying a custom directory path for indexing. The implementation follows the existing patterns in the codebase with proper annotation for command-line argument, environment variable, and help text.
Note that there's a TODO comment indicating this is meant to be a temporary solution until smart cache is implemented. Consider adding more context about the planned implementation timeline or adding this information to the project documentation.
1 participant
Summary by CodeRabbit