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.

Sign up for GitHub

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

Jump to bottom

fix(cli): Check for .keyshade dir if profile isn't found #636

Merged
merged 2 commits into from
Jan 18, 2025
Merged

fix(cli): Check for .keyshade dir if profile isn't found #636

merged 2 commits into from
Jan 18, 2025

Conversation

muntaxir4
Copy link
Contributor

@muntaxir4 muntaxir4 commented Jan 18, 2025 •
edited by codiumai-pr-agent-free bot
Loading

User description

Fixes #635

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

Description

  • Fixes issue with profile creation in CLI.

  • Ensures .keyshade directory exists before creating profile files.

  • Adds directory existence check to fetchProfileConfig function.

Changes walkthrough 📝

Relevant files
Bug fix
configuration.ts
Ensure `.keyshade` directory exists before file creation 

apps/cli/src/util/configuration.ts

  • Added ensureDirectoryExists call to check .keyshade directory
    existence.
  • Ensures profile configuration file is created only after directory
    exists.
    		• +1/-0     
    Need help?
  • Type /help how to ... in the comments thread for any question about Qodo Merge usage.
  • Check out the documentation for more information.
    •

    @codiumai-pr-agent-free CodiumAI PR-Agent (Free)
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis 🔶

    635 - Partially compliant

    Compliant requirements:

    • Fix CLI profile creation failure by ensuring the .keyshade directory exists before creating profile files

    Non-compliant requirements:

    • No changes made to handle private-keys.json file creation

    Requires further human verification:

    • Verify that profile creation works end-to-end with the new changes
    • Test on different operating systems to ensure directory/file creation works correctly
    ⏱️ Estimated effort to review: 1 🔵⚪⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Incomplete Fix

    The fix only handles profiles.json but doesn't address private-keys.json file creation which was also mentioned in the bug report

    if (!existsSync(path)) {
  await ensureDirectoryExists(path)
  await writeFile(path, '{}', 'utf8')

    @codiumai-pr-agent-free CodiumAI PR-Agent (Free)
    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Directory creation utility should receive directory path instead of file path to properly create parent directories

    The ensureDirectoryExists function should be called with the directory path, not the
    file path. Extract the directory path from the full file path before ensuring its
    existence.

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

    -await ensureDirectoryExists(path)
+await ensureDirectoryExists(dirname(path))
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: This is a critical bug fix. Using a file path instead of a directory path with ensureDirectoryExists would cause incorrect directory creation and potential runtime errors.

    		9
    General
    Add error handling for file system operations to prevent silent failures

    Add error handling around the file operations to gracefully handle potential
    filesystem permission issues or disk space problems.

    apps/cli/src/util/configuration.ts [31-34]

     if (!existsSync(path)) {
-  await ensureDirectoryExists(path)
-  await writeFile(path, '{}', 'utf8')
+  try {
+    await ensureDirectoryExists(dirname(path))
+    await writeFile(path, '{}', 'utf8')
+  } catch (error) {
+    throw new Error(`Failed to create profile configuration: ${error.message}`)
+  }
 }
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Adding proper error handling for filesystem operations is crucial for reliability, as it prevents silent failures and provides clear error messages for debugging filesystem permission or space issues.

    		8

    @muntaxir4 muntaxir4 requested a review from rajdip-b January 18, 2025 17:13
    @rajdip-b rajdip-b changed the title fix(cli): check for .keyshade dir if profile isn't found fix(cli): Check for .keyshade dir if profile isn't found Jan 18, 2025
    @rajdip-b rajdip-b merged commit a69665d into keyshade-xyz:develop Jan 18, 2025
    6 checks passed
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Reviewers

    @rajdip-b rajdip-b rajdip-b approved these changes

    Assignees
    No one assigned
    Labels
    Review effort [1-5]: 1
    Projects
    None yet
    Milestone
    No milestone
    Development

    Successfully merging this pull request may close these issues.

    CLI: Profile creation fails
    2 participants
    @muntaxir4 @rajdip-b