Fix environment variable handling by preserving placeholders for missing values #34
Conversation
WalkthroughThe changes in this pull request modify the Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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
🧹 Outside diff range and nitpick comments (2)
src/utils/ibc-processor.ts (2)
45-45: Consider additional type safety improvements.While the current fix works correctly, we could further improve type safety and error handling.
Consider this enhanced version:
- return process.env[p1] !== undefined ? String(process.env[p1]) : match; // Replace with env var value (even if empty), or keep the placeholder if not found + const envValue = process.env[p1]; + return envValue !== undefined && envValue !== null + ? String(envValue) + : match; // Replace with env var value (even if empty), or keep the placeholder if not foundThis version:
- Adds null check for complete type safety
- Improves readability by splitting the logic
- Maintains the same functionality
42-48: Document the environment variable interpolation behavior.The changes in environment variable handling should be documented to make the behavior clear to users.
Add a comment block explaining the interpolation behavior:
const fileContents = fs.readFileSync(configPath, 'utf8') + // Environment variable interpolation: + // - ${VAR} is replaced with the value of VAR if defined (including empty string) + // - ${VAR} is preserved as-is if VAR is undefined + // This allows for optional configuration through environment variables const interpolatedFileContents = fileContents.replace(/\$\{([A-Z0-9_]+)}/g, (match, p1) => { return process.env[p1] !== undefined ? String(process.env[p1]) : match; });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/utils/ibc-processor.ts(1 hunks)
🔇 Additional comments (2)
src/utils/ibc-processor.ts (2)
45-45: LGTM! Correct fix for environment variable handling.
The change properly distinguishes between undefined and empty environment variables, ensuring that empty strings are preserved rather than falling back to the placeholder. This fixes the original issue while maintaining backward compatibility for undefined variables.
42-48: Verify YAML parsing after environment variable interpolation.
The current implementation might not catch all edge cases where environment variable interpolation could lead to invalid YAML.
Let's verify the error handling:
Summary by CodeRabbit
Bug Fixes
Refactor