Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(cli): Added parent directory check #359

Conversation

Princeyadav05
Copy link
Contributor

@Princeyadav05 Princeyadav05 commented Jul 16, 2024

User description

Description

Add directory creation to configuration write functions to prevent CLI errors when parent folders don't exist.

Fixes #320

Dependencies

No new dependencies added. Using existing fs/promises and path modules.

Future Improvements

NA

Mentions

NA

Screenshots of relevant screens

N/A - CLI functionality update

Developer's checklist

  • My PR follows the style guidelines of this project
  • I have performed a self-check on my work

If changes are made in the code:

  • I have followed the coding guidelines
  • My changes in code generate no new warnings
  • My changes are breaking another fix/feature of the project
  • I have added test cases to show that my feature works
  • I have added relevant screenshots in my PR
  • There are no UI/UX issues

Documentation Update

  • This PR requires an update to the documentation at docs.keyshade.xyz
  • I have made the necessary updates to the documentation, or no documentation changes are required.

PR Type

Bug fix, Enhancement


Description

  • Added ensureDirectoryExists function to ensure parent directories are created if they do not exist.
  • Updated writeProfileConfig, writePrivateKeyConfig, and writeProjectRootConfig functions to call ensureDirectoryExists before writing files.
  • This prevents CLI errors when parent folders do not exist.

Changes walkthrough 📝

Relevant files
Enhancement
configuration.ts
Add directory creation check to configuration write functions

apps/cli/src/util/configuration.ts

  • Added ensureDirectoryExists function to create parent directories if
    they do not exist.
  • Updated writeProfileConfig, writePrivateKeyConfig, and
    writeProjectRootConfig functions to call ensureDirectoryExists.
  • +6/-0     

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    Copy link
    Contributor

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Missing Error Handling
    The function ensureDirectoryExists does not handle any errors that might occur during directory creation. Consider adding try-catch blocks around the mkdir call to handle potential exceptions and provide meaningful error messages to the user.

    Copy link
    Contributor

    codiumai-pr-agent-free bot commented Jul 16, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Error handling
    ✅ Handle errors in directory creation to improve robustness
    Suggestion Impact:The suggestion to handle potential errors during directory creation was implemented by introducing a new function `ensureDirectoryExists` which likely includes error handling, and this function was then used in place of the direct `mkdir` calls.

    code diff:

    +import { ensureDirectoryExists } from './fileUtils.ts';
     
     export const getOsType = (): 'unix' | 'windows' => {
       return process.platform === 'win32' ? 'windows' : 'unix'
    @@ -59,7 +60,7 @@
       config: ProfileConfig
     ): Promise<void> => {
       const path = getProfileConfigurationFilePath()
    -  ensureDirectoryExists(path);
    +  await ensureDirectoryExists(path);
       await writeFile(path, JSON.stringify(config, null, 2), 'utf8')
     }
     
    @@ -67,7 +68,7 @@
       config: PrivateKeyConfig
     ): Promise<void> => {
       const path = getPrivateKeyConfigurationFilePath()
    -  ensureDirectoryExists(path);
    +  await ensureDirectoryExists(path);
       await writeFile(path, JSON.stringify(config, null, 2), 'utf8')
     }
     
    @@ -83,8 +84,4 @@
       const path = `${process.env[home]}/.keyshade`
       const files = await readdir(path)
       return `- ${files.join('\n- ')}`
    -}
    -
    -async function ensureDirectoryExists(path: string): Promise<void> {
    -  await mkdir(dirname(path), { recursive: true })
    -}
    +}

    To handle potential errors that could occur during directory creation, wrap the
    mkdir call in a try-catch block.

    apps/cli/src/util/configuration.ts [89]

    -await mkdir(dirname(path), { recursive: true })
    +try {
    +    await mkdir(dirname(path), { recursive: true })
    +} catch (error) {
    +    console.error('Failed to create directory:', error);
    +}
     
    • Apply this suggestion
    Suggestion importance[1-10]: 10

    Why: Wrapping the mkdir call in a try-catch block is crucial for error handling, ensuring the application can handle potential failures gracefully.

    10
    Performance
    ✅ Add a check to see if the directory exists before creating it
    Suggestion Impact:The commit introduced a new function `ensureDirectoryExists` which encapsulates the logic to check if a directory exists before creating it. This function was then used in place of the original directory creation calls, effectively implementing the suggestion.

    code diff:

    +import { ensureDirectoryExists } from './fileUtils.ts';
     
     export const getOsType = (): 'unix' | 'windows' => {
       return process.platform === 'win32' ? 'windows' : 'unix'
    @@ -59,7 +60,7 @@
       config: ProfileConfig
     ): Promise<void> => {
       const path = getProfileConfigurationFilePath()
    -  ensureDirectoryExists(path);
    +  await ensureDirectoryExists(path);
       await writeFile(path, JSON.stringify(config, null, 2), 'utf8')
     }
     
    @@ -67,7 +68,7 @@
       config: PrivateKeyConfig
     ): Promise<void> => {
       const path = getPrivateKeyConfigurationFilePath()
    -  ensureDirectoryExists(path);
    +  await ensureDirectoryExists(path);
       await writeFile(path, JSON.stringify(config, null, 2), 'utf8')
     }
     
    @@ -83,8 +84,4 @@
       const path = `${process.env[home]}/.keyshade`
       const files = await readdir(path)
       return `- ${files.join('\n- ')}`
    -}
    -
    -async function ensureDirectoryExists(path: string): Promise<void> {
    -  await mkdir(dirname(path), { recursive: true })
    -}

    Consider checking if the directory already exists before attempting to create it.
    This can prevent unnecessary operations and improve efficiency.

    apps/cli/src/util/configuration.ts [89]

    -await mkdir(dirname(path), { recursive: true })
    +if (!existsSync(dirname(path))) {
    +    await mkdir(dirname(path), { recursive: true })
    +}
     
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: This suggestion improves performance by avoiding unnecessary directory creation operations. It is a logical enhancement to the existing code.

    9
    Maintainability
    ✅ Move ensureDirectoryExists to a common utilities file to reduce code duplication
    Suggestion Impact:The function ensureDirectoryExists was moved to a common utilities file (fileUtils.ts) and imported where needed, reducing code duplication.

    code diff:

    +import { ensureDirectoryExists } from './fileUtils.ts';
     
     export const getOsType = (): 'unix' | 'windows' => {
       return process.platform === 'win32' ? 'windows' : 'unix'
    @@ -59,7 +60,7 @@
       config: ProfileConfig
     ): Promise<void> => {
       const path = getProfileConfigurationFilePath()
    -  ensureDirectoryExists(path);
    +  await ensureDirectoryExists(path);
       await writeFile(path, JSON.stringify(config, null, 2), 'utf8')
     }
     
    @@ -67,7 +68,7 @@
       config: PrivateKeyConfig
     ): Promise<void> => {
       const path = getPrivateKeyConfigurationFilePath()
    -  ensureDirectoryExists(path);
    +  await ensureDirectoryExists(path);
       await writeFile(path, JSON.stringify(config, null, 2), 'utf8')
     }
     
    @@ -83,8 +84,4 @@
       const path = `${process.env[home]}/.keyshade`
       const files = await readdir(path)
       return `- ${files.join('\n- ')}`
    -}
    -
    -async function ensureDirectoryExists(path: string): Promise<void> {
    -  await mkdir(dirname(path), { recursive: true })
    -}

    Since ensureDirectoryExists is used in multiple places, consider moving this
    function to a common utilities file to avoid duplication and improve
    maintainability.

    apps/cli/src/util/configuration.ts [88-90]

    -async function ensureDirectoryExists(path: string): Promise<void> {
    -    await mkdir(dirname(path), { recursive: true })
    -}
    +// This function should be moved to a common utilities file
    +// and imported where needed.
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: This suggestion enhances maintainability by reducing code duplication, making it easier to manage and update the utility function.

    8
    Best practice
    Normalize or resolve the directory path to handle relative paths correctly

    To ensure that the directory path is correctly handled, use path.resolve or
    path.normalize on the path parameter to avoid issues with relative paths.

    apps/cli/src/util/configuration.ts [89]

    -await mkdir(dirname(path), { recursive: true })
    +await mkdir(dirname(path.resolve(path)), { recursive: true })
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Using path.resolve or path.normalize ensures that the directory path is handled correctly, which is a good practice to avoid issues with relative paths.

    7

    @rajdip-b rajdip-b changed the title fix: added directory check fix(cli): Added parent directory check Jul 16, 2024
    @rajdip-b rajdip-b added the foss hack Clustering all the curated issues for Foss Hack 2024 label Jul 16, 2024
    @rajdip-b rajdip-b force-pushed the fix/#320-CLI-Update-create-file-functions-to-also-create-the-parent-directory branch from fb8a88a to e2bd693 Compare July 17, 2024 15:47
    @rajdip-b rajdip-b force-pushed the fix/#320-CLI-Update-create-file-functions-to-also-create-the-parent-directory branch from e2bd693 to 547fb04 Compare July 29, 2024 13:39
    @rajdip-b rajdip-b merged commit 2e3a1af into keyshade-xyz:develop Jul 29, 2024
    4 checks passed
    rajdip-b pushed a commit that referenced this pull request Jul 29, 2024
    rajdip-b pushed a commit that referenced this pull request Jul 29, 2024
    ## [2.3.0](v2.2.0...v2.3.0) (2024-07-29)
    
    ### 🚀 Features
    
    * **api:** Add pagination metadata to Environment module ([#382](#382)) ([9baa344](9baa344))
    * **api:** Add pagination metadata to Event module ([#394](#394)) ([60010b4](60010b4))
    * **api:** Add pagination metadata to Integration module ([#391](#391)) ([0372e36](0372e36))
    * **api:** Add pagination metadata to Project module ([#393](#393)) ([bc274fd](bc274fd))
    * **api:** Add pagination metadata to Secret module ([#389](#389)) ([c4cc667](c4cc667))
    * **api:** Add pagination metadata to Variable module ([#390](#390)) ([be6aabf](be6aabf))
    * **api:** Add pagination metadata to Workspace module  ([#387](#387)) ([a08c924](a08c924))
    * **api:** Add pagination metadata to Workspace Role module ([#388](#388)) ([d8e8f49](d8e8f49))
    * **api:** Create a paginate method ([#379](#379)) ([09576f1](09576f1))
    * **api:** Create endpoint for fetching all revisions of a [secure] ([#303](#303)) ([de2b602](de2b602))
    * **api:** Create endpoint for fetching all revisions of a variable ([#304](#304)) ([9abddc1](9abddc1))
    * **cli:** Improved the DX for list profile ([#334](#334)) ([6bff496](6bff496))
    * **platform:** Add warning sonner toast for invalid otp ([#335](#335)) ([21513f5](21513f5))
    
    ### 🐛 Bug Fixes
    
    * **cli:** Added parent directory check ([#359](#359)) ([538ea7f](538ea7f))
    * **platform:** Platform types fixes ([#374](#374)) ([8e9d9ff](8e9d9ff))
    
    ### 📚 Documentation
    
    * Added docker details in setting-things-up.md ([#358](#358)) ([ed5093a](ed5093a))
    * Update postman workspace link ([d6aba27](d6aba27))
    * Updated env and cli docs ([1213d2a](1213d2a))
    
    ### 🔧 Miscellaneous Chores
    
    * Added next backend url in .env.example ([5695254](5695254))
    * **api-client:** Added pagination structure ([a70e957](a70e957))
    * **api-client:** Fixed test script ([ad70819](ad70819))
    * **api-client:** Removed try-catch from tests in environment ([a64e48c](a64e48c))
    * **api:** Add user cache for optimization ([#386](#386)) ([8d730b5](8d730b5))
    * **api:** Alter cache rehydration interval ([f5f9eec](f5f9eec))
    * **api:** Fixed naming error in variable controller ([0c5a380](0c5a380))
    * **api:** Improve handling of edge cases for paginate module ([#402](#402)) ([8591487](8591487))
    * **api:** Minor updates to user service ([249d778](249d778))
    * **api:** Skip workspace creation when user is admin ([#376](#376)) ([13f6c59](13f6c59))
    * **ci:** Add docker check   ([#383](#383)) ([3119001](3119001))
    * **ci:** Add names to CI files ([1a7e5f6](1a7e5f6))
    * **ci:** Add validate CLI pipeline ([#373](#373)) ([a91df6c](a91df6c))
    * **ci:** Adding validate pipeline ([#372](#372)) ([23cf3b3](23cf3b3))
    * **ci:** Disabled platform and api deployments ([74d601a](74d601a))
    * **ci:** Fixed deployment scripts ([12e35db](12e35db))
    * **ci:** Fixed platform script ([d783f2a](d783f2a))
    * **CI:** Include migration deployment in API deploy pipeline ([dbd5222](dbd5222))
    * **CI:** Separated deployment and docker build jobs ([090e193](090e193))
    * **CI:** Setup inter-job dependency ([1756727](1756727))
    * **ci:** Update auto-assign.yaml ([#375](#375)) ([91e0ec1](91e0ec1))
    * **cli:** Changed objects to classes ([#306](#306)) ([c83f2db](c83f2db))
    * Removed Minio config ([8feb83a](8feb83a))
    * Updated deployment scripts and added health check in platform ([fcc1c3f](fcc1c3f))
    
    ### 🔨 Code Refactoring
    
    * **api:** Updated path of some endpoints in project controller ([9502678](9502678))
    * **api:** Updated Redis provider ([33491a1](33491a1))
    rajdip-b pushed a commit that referenced this pull request Jul 29, 2024
    ## [2.3.0](v2.2.0...v2.3.0) (2024-07-29)
    
    ### 🚀 Features
    
    * **api:** Add pagination metadata to Environment module ([#382](#382)) ([9baa344](9baa344))
    * **api:** Add pagination metadata to Event module ([#394](#394)) ([60010b4](60010b4))
    * **api:** Add pagination metadata to Integration module ([#391](#391)) ([0372e36](0372e36))
    * **api:** Add pagination metadata to Project module ([#393](#393)) ([bc274fd](bc274fd))
    * **api:** Add pagination metadata to Secret module ([#389](#389)) ([c4cc667](c4cc667))
    * **api:** Add pagination metadata to Variable module ([#390](#390)) ([be6aabf](be6aabf))
    * **api:** Add pagination metadata to Workspace module  ([#387](#387)) ([a08c924](a08c924))
    * **api:** Add pagination metadata to Workspace Role module ([#388](#388)) ([d8e8f49](d8e8f49))
    * **api:** Create a paginate method ([#379](#379)) ([09576f1](09576f1))
    * **api:** Create endpoint for fetching all revisions of a [secure] ([#303](#303)) ([de2b602](de2b602))
    * **api:** Create endpoint for fetching all revisions of a variable ([#304](#304)) ([9abddc1](9abddc1))
    * **cli:** Improved the DX for list profile ([#334](#334)) ([6bff496](6bff496))
    * **platform:** Add warning sonner toast for invalid otp ([#335](#335)) ([21513f5](21513f5))
    
    ### 🐛 Bug Fixes
    
    * **cli:** Added parent directory check ([#359](#359)) ([538ea7f](538ea7f))
    * **platform:** Platform types fixes ([#374](#374)) ([8e9d9ff](8e9d9ff))
    
    ### 📚 Documentation
    
    * Added docker details in setting-things-up.md ([#358](#358)) ([ed5093a](ed5093a))
    * Update postman workspace link ([d6aba27](d6aba27))
    * Updated env and cli docs ([1213d2a](1213d2a))
    
    ### 🔧 Miscellaneous Chores
    
    * Added next backend url in .env.example ([5695254](5695254))
    * **api-client:** Added pagination structure ([a70e957](a70e957))
    * **api-client:** Fixed test script ([ad70819](ad70819))
    * **api-client:** Removed try-catch from tests in environment ([a64e48c](a64e48c))
    * **api:** Add user cache for optimization ([#386](#386)) ([8d730b5](8d730b5))
    * **api:** Alter cache rehydration interval ([f5f9eec](f5f9eec))
    * **api:** Fixed naming error in variable controller ([0c5a380](0c5a380))
    * **api:** Improve handling of edge cases for paginate module ([#402](#402)) ([8591487](8591487))
    * **api:** Minor updates to user service ([249d778](249d778))
    * **api:** Skip workspace creation when user is admin ([#376](#376)) ([13f6c59](13f6c59))
    * **ci:** Add docker check   ([#383](#383)) ([3119001](3119001))
    * **ci:** Add names to CI files ([1a7e5f6](1a7e5f6))
    * **ci:** Add validate CLI pipeline ([#373](#373)) ([a91df6c](a91df6c))
    * **ci:** Adding validate pipeline ([#372](#372)) ([23cf3b3](23cf3b3))
    * **ci:** Disabled platform and api deployments ([74d601a](74d601a))
    * **ci:** Fixed deployment scripts ([12e35db](12e35db))
    * **ci:** Fixed platform script ([d783f2a](d783f2a))
    * **CI:** Include migration deployment in API deploy pipeline ([dbd5222](dbd5222))
    * **CI:** Separated deployment and docker build jobs ([090e193](090e193))
    * **CI:** Setup inter-job dependency ([1756727](1756727))
    * **ci:** Update auto-assign.yaml ([#375](#375)) ([91e0ec1](91e0ec1))
    * **cli:** Changed objects to classes ([#306](#306)) ([c83f2db](c83f2db))
    * Removed Minio config ([8feb83a](8feb83a))
    * Updated deployment scripts and added health check in platform ([fcc1c3f](fcc1c3f))
    
    ### 🔨 Code Refactoring
    
    * **api:** Updated path of some endpoints in project controller ([9502678](9502678))
    * **api:** Updated Redis provider ([33491a1](33491a1))
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    Bug fix enhancement foss hack Clustering all the curated issues for Foss Hack 2024 Review effort [1-5]: 2
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    CLI: Update create file functions to also create the parent directory
    2 participants