✨ Feature: Preserve Existing .env File and Update Values if Present
#5
Conversation
WalkthroughThe changes introduced in this pull request enhance the functionality of the Changes
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: 6
🧹 Outside diff range and nitpick comments (2)
src/index.ts (1)
Line range hint
38-59: Fetch SSM parameters concurrently to improve performance.The
fetchAndUpdateEnvVariablesfunction currently fetches SSM parameters sequentially within afor...ofloop. This can lead to increased execution time, especially when dealing with numerous parameters.Consider modifying the function to fetch parameters concurrently using
Promise.all:async function fetchAndUpdateEnvVariables( paramMap: Record<string, string>, existingEnvVariables: Record<string, string>, ): Promise<Record<string, string>> { console.log('Fetching parameters...'); - for (const [envVar, ssmName] of Object.entries(paramMap)) { - try { - const value = await fetchSSMParameter(ssmName); - if (value) { - existingEnvVariables[envVar] = value; - console.log(`${envVar}=${value}`); - } else { - console.error(`Warning: No value found for ${ssmName}`); - } - } catch (error) { - console.error(`Error fetching parameter ${ssmName}: ${error}`); - throw new Error(`ParameterNotFound: ${ssmName}`); - } - } + await Promise.all( + Object.entries(paramMap).map(async ([envVar, ssmName]) => { + try { + const value = await fetchSSMParameter(ssmName); + if (value !== undefined) { + existingEnvVariables[envVar] = value; + console.log(`${envVar}=${value}`); + } else { + console.error(`Warning: No value found for ${ssmName}`); + } + } catch (error) { + console.error(`Error fetching parameter ${ssmName}:`, error); + throw new Error(`ParameterNotFound: ${ssmName}`); + } + }) + ); return existingEnvVariables; }This change will initiate all fetch requests simultaneously, reducing the total execution time.
tests/index.test.ts (1)
129-144: Ensure Variables with Empty Values Are Not Added to.envFileThe test checks that a warning is logged when an SSM parameter has no value. It would strengthen the test to also verify that the variable with the empty value is not written to the
.envfile, ensuring that the application correctly handles this scenario.Consider adding an assertion to verify the
.envfile does not include the parameter with an empty value:// Assert expect(consoleSpy).toHaveBeenCalledWith(expect.stringContaining('Warning: No value found for')); + const envFileContent = fs.readFileSync(mockEnvFilePath, 'utf-8'); + expect(envFileContent).not.toContain('NEXT_PUBLIC_CREDENTIAL_PASSWORD='); fs.unlinkSync(mockEnvFilePath); fs.unlinkSync(mockMapPath);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- src/index.ts (2 hunks)
- tests/index.test.ts (2 hunks)
- tests/sample/param_map.json (1 hunks)
🧰 Additional context used
🔇 Additional comments (3)
tests/sample/param_map.json (1)
1-3: Significant changes to authentication-related parametersThe modifications to this file represent a substantial shift in how authentication-related parameters are handled:
Removed keys:
- "NEXT_PUBLIC_CREDENTIAL_EMAIL"
- "NEXT_PUBLIC_CREDENTIAL_PASSWORD"
- "NEXT_PUBLIC_JWT_SECRET"
Added key:
- "TOKEN_SECRET": "/Test/Token"
These changes have several implications:
- The authentication mechanism appears to have been simplified, moving from separate credential and JWT secret keys to a single token secret.
- The removal of the "NEXT_PUBLIC_" prefix suggests this information is no longer intended to be public-facing.
- The new value "/Test/Token" is less specific than the previous values, which could be a security concern if not properly managed.
Please ensure that:
- All parts of the codebase that previously used the removed keys have been updated accordingly.
- The new "TOKEN_SECRET" is being used securely and isn't exposing sensitive information.
- If "/Test/Token" is a placeholder, it should be replaced with the actual path before deployment.
To verify the usage of these keys in the codebase, you can run the following script:
This will help identify any places where the old keys are still being used or where the new key needs to be implemented.
src/index.ts (1)
68-69: Verify AWS SSM client configuration and error handling.In the
fetchSSMParameterfunction, if the SSM client is not properly configured with AWS credentials or region, the request will fail. Additionally, consider handling specific AWS errors more gracefully.Run the following script to confirm that the AWS SSM client can access the parameters:
If the command fails, ensure that:
- AWS credentials are correctly set in your environment.
- The AWS region is specified and correct.
- The IAM role or user has permissions to access the SSM parameters.
tests/index.test.ts (1)
21-24: Appropriate Mock for Empty SSM Parameter ValueThe added mock implementation correctly simulates an SSM parameter that exists but has an empty value. This is essential for testing how the application handles parameters without values.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
src/index.ts (1)
39-66: LGTM: Improved error handling infetchAndUpdateEnvVariables.The changes to
fetchAndUpdateEnvVariablessignificantly improve error handling and address the previous review comment about continuing to fetch remaining parameters after an error occurs. The function now collects errors and throws a consolidated error at the end if any parameters failed to fetch, which is a great improvement.However, consider using
Promise.allinstead of afor...ofloop for potential performance benefits, especially when dealing with a large number of parameters:async function fetchAndUpdateEnvVariables( paramMap: Record<string, string>, existingEnvVariables: Record<string, string>, ): Promise<Record<string, string>> { console.log('Fetching parameters...'); const errors: string[] = []; await Promise.all( Object.entries(paramMap).map(async ([envVar, ssmName]) => { try { const value = await fetchSSMParameter(ssmName); if (value) { existingEnvVariables[envVar] = value; console.log(`${envVar}=${value}`); } else { console.error(`Warning: No value found for: '${ssmName}'`); } } catch (error) { console.error(`Error fetching parameter: '${ssmName}'`); errors.push(`ParameterNotFound: ${ssmName}`); } }) ); if (errors.length > 0) { throw new Error(`Some parameters could not be fetched:\n${errors.join('\n')}`); } return existingEnvVariables; }This change would allow for concurrent fetching of parameters, potentially improving performance.
tests/index.test.ts (3)
77-100: LGTM: New test case for appending SSM parametersThis new test case is a valuable addition as it verifies that existing environment variables are preserved while new SSM parameters are appended. The test structure follows the Arrange-Act-Assert pattern, which is a good practice.
However, there's an unintended whitespace issue in the
existingEnvContent. To avoid potential parsing issues, consider removing the leading whitespace:- const existingEnvContent = 'EXISTING_VAR=existingValue'; + const existingEnvContent = 'EXISTING_VAR=existingValue';
102-123: LGTM: New test case for overwriting SSM parametersThis new test case is a great addition as it verifies that existing environment variables are correctly overwritten with new SSM parameter values. The test structure follows the Arrange-Act-Assert pattern, which is a good practice.
Similar to the previous test, there's an unintended whitespace issue in the
existingEnvContent. To avoid potential parsing issues, consider removing the leading whitespace:- const existingEnvContent = '[email protected]'; + const existingEnvContent = '[email protected]';
125-141: LGTM: New test case for logging warning on empty SSM parameter valueThis new test case is an excellent addition as it verifies the behavior when an SSM parameter has no value. The use of a console spy to check for the warning message is a good approach.
Consider adding an assertion to verify that the environment variable is not set when the SSM parameter has no value. This would ensure that empty values are not inadvertently added to the .env file:
expect(updatedEnvFileContent).not.toContain('NEXT_PUBLIC_CREDENTIAL_PASSWORD=');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (4)
- package.json (2 hunks)
- src/cli/cliRunner.ts (1 hunks)
- src/index.ts (1 hunks)
- tests/index.test.ts (2 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/cli/cliRunner.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🧰 Additional context used
🔇 Additional comments (10)
src/index.ts (7)
3-3: LGTM: Good addition ofdotenvpackage.The import of
dotenvis a great addition. It addresses the previous suggestion to use a robust parser for.envfiles, which will help handle various edge cases and improve the overall reliability of the environment variable handling.
7-15: LGTM: Excellent refactoring of therunfunction.The changes to the
runfunction are well-structured and align perfectly with the PR objectives. The use of helper functions (loadParamMap,loadExistingEnvVariables,fetchAndUpdateEnvVariables, andwriteEnvFile) improves modularity and readability. This approach effectively preserves existing environment variables while updating or adding new ones from the SSM parameters.
17-25: LGTM: Robust implementation ofloadParamMap.The
loadParamMapfunction is well-implemented with proper error handling for JSON parsing. This addresses the previous review comment about handling potential JSON parsing errors. The function now logs the error and throws a more informative error message, which will help with debugging and improve the overall robustness of the application.
27-37: LGTM: Excellent implementation ofloadExistingEnvVariables.The
loadExistingEnvVariablesfunction is well-implemented and addresses the previous review comment about using a robust parser for.envfiles. By utilizingdotenv.parse, the function now correctly handles various edge cases that may occur in.envfiles. The function also properly handles the scenario when the file doesn't exist, returning an empty object in that case.
68-76: LGTM: Well-implementedfetchSSMParameterfunction.The
fetchSSMParameterfunction is a great addition that encapsulates the logic for fetching a parameter from SSM. It correctly uses the AWS SDK and improves code modularity by separating this functionality. The use ofWithDecryption: trueensures that encrypted parameters are properly handled.
1-83: Overall, excellent improvements to Envilder's.envfile handling.The changes in this PR significantly enhance Envilder's ability to handle
.envfiles. The new implementation successfully preserves existing environment variables while updating or adding new ones from SSM parameters. Key improvements include:
- Use of
dotenvfor robust parsing of.envfiles.- Modular design with separate functions for different responsibilities.
- Improved error handling, especially in
loadParamMapandfetchAndUpdateEnvVariables.- Better code organization and readability.
These changes align well with the PR objectives and address most of the previous review comments. The code is now more robust and maintainable.
To further improve the implementation, consider:
- Using
Promise.allinfetchAndUpdateEnvVariablesfor potential performance benefits.- Enhancing
writeEnvFileto properly handle special characters in environment variable values.Great work on this feature enhancement!
78-83:⚠️ Potential issueImprove
writeEnvFileto handle special characters.While the
writeEnvFilefunction is a good addition, it doesn't handle escaping of special characters in environment variable values. This could lead to issues when parsing the.envfile later, especially for values containing newlines, quotes, or equal signs.Consider using
dotenv.stringifyor manually escaping special characters:import * as dotenv from 'dotenv'; function writeEnvFile(envFilePath: string, envVariables: Record<string, string>): void { const envContent = Object.entries(envVariables) .map(([key, value]) => `${key}=${dotenv.stringify(value)}`) .join('\n'); fs.writeFileSync(envFilePath, envContent); }Alternatively, if you prefer manual escaping:
function writeEnvFile(envFilePath: string, envVariables: Record<string, string>): void { const envContent = Object.entries(envVariables) .map(([key, value]) => { const escapedValue = value.replace(/(\n|\r|\n\r)/g, '\\n').replace(/"/g, '\\"'); return `${key}="${escapedValue}"`; }) .join('\n'); fs.writeFileSync(envFilePath, envContent); }This will ensure that the generated
.envfile can be correctly parsed later, even with complex values.tests/index.test.ts (3)
21-23: LGTM: Enhanced mock implementation for SSMThe addition of a new condition for the '/path/to/ssm/password_no_value' parameter is a good improvement. It allows testing the scenario where an SSM parameter exists but has no value, which is an important edge case to consider.
Line range hint
1-141: Overall assessment: Significant improvements to test coverageThe changes to this file have greatly enhanced the test suite for the Envilder CLI. New test cases cover important scenarios such as appending new SSM parameters, overwriting existing variables, and handling SSM parameters with no value. These additions will help ensure the robustness and reliability of the CLI.
A few minor issues were identified:
- Inconsistency in SSM parameter naming in one test case.
- Unintended whitespace in some test data.
- A suggestion for an additional assertion in the empty value test case.
Addressing these minor points will further improve the quality of the test suite. Great work on expanding the test coverage!
65-65:⚠️ Potential issueMaintain consistency in SSM parameter naming
The change from a specific SSM parameter path to a generic string 'non-existent parameter' is inconsistent with the best practices for SSM parameter naming and contradicts the realistic paths used in other tests.
As mentioned in a previous review, it's recommended to use realistic SSM parameter paths in tests. Please consider reverting this change and using a path like '/path/to/ssm/non_existent_parameter' instead.
Also applies to: 73-75
Description
This PR introduces a new feature to Envilder, allowing it to append or update values in an existing
.envfile instead of overwriting the entire file. If an.envfile already exists, Envilder will keep the previous environment variables intact and only update the ones specified in the new parameter map. If the variable exists, it will be updated with the latest value from AWS SSM; if it doesn’t exist, it will be appended to the.envfile.Approach
The change modifies the functionality of the
runmethod to:.envfile if present..envfile.This approach ensures that existing configurations are preserved, while still allowing for updates to specific values when needed.
Open Questions and Pre-Merge TODOs
.envfiles or variables with no value) are handled properly..envfiles to ensure performance remains optimal.Learning
The implementation required a better understanding of how to read and update environment files without erasing or overwriting existing variables. The decision to use a combination of reading, parsing, and re-writing only necessary variables ensures that the feature is robust and doesn't inadvertently remove existing configurations.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores
"dotenv": "^16.4.5"to the project.