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): Removed unnecessary console log in secrets #515

Merged
merged 1 commit into from
Oct 30, 2024

Conversation

anudeeps352
Copy link
Contributor

@anudeeps352 anudeeps352 commented Oct 28, 2024

User description

Description

Removed an unnecessary console log

Fixes #302

Dependencies

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

enhancement


Description

  • Removed an unnecessary console log statement from the ListSecret command in the CLI application.
  • This change enhances the code by cleaning up redundant output, improving readability and performance.

PRDescriptionHeader.CHANGES_WALKTHROUGH

Relevant files
Enhancement
list.secret.ts
Remove unnecessary console log from ListSecret command     

apps/cli/src/commands/secret/list.secret.ts

  • Removed an unnecessary console log statement.
  • Improved code readability by eliminating redundant output.
  • +0/-1     

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    Copy link
    Contributor

    codiumai-pr-agent-free bot commented Oct 28, 2024

    PR Reviewer Guide 🔍

    (Review updated until commit 96e99d6)

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis 🔶

    302 - Partially compliant

    Fully compliant requirements:

    • List all secrets under a project

    Not compliant requirements:

    • Fetch all revisions of a secret
    • Create a secret
    • Update a secret
    • Rollback a secret
    • Delete a secret
    • Create an implementation of command.interface.ts and name it SecretCommand
    • Stash all the functions in src/commands/secret
    • Use the secretController under ControllerInstance to make the API calls
    ⏱️ Estimated effort to review: 1 🔵⚪⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Code Cleanup
    Verify if removing the console.log statement affects any debugging or logging functionality

    Copy link
    Contributor

    codiumai-pr-agent-free bot commented Oct 28, 2024

    PR Code Suggestions ✨

    Latest suggestions up to 96e99d6
    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Enhancement
    Provide user feedback when no items are found in a list operation

    Consider handling the case where secrets.length is 0 to provide feedback to the user
    when no secrets are found.

    apps/cli/src/commands/secret/list.secret.ts [44-46]

     if (secrets.length > 0) {
       data.forEach((secret: any) => {
         Logger.info(`- ${secret.name} (${secret.value})`)
       })
    +} else {
    +  Logger.info('No secrets found.')
     }
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion improves user experience by providing feedback when no secrets are found, which is a valuable enhancement for understanding the command's output.

    8
    Best practice
    Improve type safety by using a specific interface instead of 'any' for loop variables

    Consider using a more specific type for the secret parameter in the forEach loop
    instead of any. This will provide better type safety and code clarity.

    apps/cli/src/commands/secret/list.secret.ts [45-46]

    -data.forEach((secret: any) => {
    +interface Secret {
    +  name: string;
    +  value: string;
    +}
    +
    +data.forEach((secret: Secret) => {
       Logger.info(`- ${secret.name} (${secret.value})`)
     })
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion to use a specific interface instead of 'any' enhances type safety and code clarity, which is beneficial for maintainability and reducing potential runtime errors.

    7

    💡 Need additional feedback ? start a PR chat


    Previous suggestions

    Suggestions up to commit e77a309
    CategorySuggestion                                                                                                                                    Score
    Security
    Remove unnecessary console logging of potentially sensitive data

    Remove the console.log statement as it's unnecessary and may expose sensitive
    information. The Logger is already being used to display the necessary information.

    apps/cli/src/commands/secret/list.secret.ts [43-54]

     if (success) {
    -  console.log(data)
       const secrets = data
       if (secrets.length > 0) {
         data.items.forEach((secret: any) => {
           Logger.info(`- ${secret.name} (${secret.value})`)
         })
       } else {
         Logger.info('No secrets found')
       }
     } else {
       Logger.error(`Failed fetching secrets: ${error.message}`)
     }
    Suggestion importance[1-10]: 9

    Why: Removing the console.log statement is crucial as it may expose sensitive information. The Logger is already being used to display necessary information, making the console.log redundant and potentially harmful.

    9
    Possible issue
    Validate the rotateAfter option to ensure it only accepts allowed values

    Add input validation for the rotateAfter option to ensure it only accepts valid
    values ('24', '168', '720', '8760', or 'never').

    apps/cli/src/commands/secret/create.secret.ts [214-256]

     private async parseInput(options: CommandActionData['options']): Promise<{
       name: string
       note?: string
       rotateAfter?: '24' | '168' | '720' | '8760' | 'never'
       entries: Array<{ value: string; environmentSlug: string }>
     }> {
       let { name, note, rotateAfter } = options
       const { entries } = options
     
       if (!name) {
         name = await text({
           message: 'Enter the name of secret',
           placeholder: 'My Secret'
         })
       }
     
       if (!entries) {
         throw new Error('Entries is required')
       }
     
       if (!note) {
         note = name
       }
     
    +  if (rotateAfter && !['24', '168', '720', '8760', 'never'].includes(rotateAfter)) {
    +    throw new Error('Invalid rotateAfter value. Must be 24, 168, 720, 8760, or never.')
    +  }
    +
       const parsedEntries = entries.map((entry) => {
         const entryObj: { value: string; environmentSlug: string } = {
           value: '',
           environmentSlug: ''
         }
         entry.split(' ').forEach((pair) => {
           const [key, value] = pair.split('=')
           entryObj[key] = value
         })
         return entryObj
       })
     
       return {
         name,
         note,
         rotateAfter,
         entries: parsedEntries
       }
     }
    Suggestion importance[1-10]: 8

    Why: Adding validation for the rotateAfter option is important to prevent invalid values from being processed, which could lead to unexpected behavior or errors. This enhances the robustness of the input handling.

    8
    Validate required options before making the API call for secret rollback

    Add validation for the required environmentSlug and version options to ensure they
    are provided before making the API call.

    apps/cli/src/commands/secret/rollback.secret.ts [122-144]

     async action({ args, options }: CommandActionData): Promise<void> {
       const [secretSlug] = args
       const { environmentSlug, version } = await this.parseInput(options)
    +
    +  if (!environmentSlug || !version) {
    +    Logger.error('Both environmentSlug and version are required for rollback')
    +    return
    +  }
    +
       const { data, error, success } =
         await ControllerInstance.getInstance().secretController.rollbackSecret(
           {
             environmentSlug,
             version,
             secretSlug
           },
           this.headers
         )
     
       if (success) {
         Logger.info(`Secret ${data.name} (${data.slug}) updated successfully!`)
         Logger.info(`Created at ${data.createdAt}`)
         Logger.info(`Updated at ${data.updatedAt}`)
         Logger.info(`Note: ${data.note}`)
         Logger.info(`rotateAfter: ${data.rotateAfter}`)
       } else {
         Logger.error(`Failed to update secret: ${error.message}`)
       }
     }
    Suggestion importance[1-10]: 8

    Why: Validating the required options before making the API call is essential to prevent unnecessary API requests and to provide immediate feedback to the user, enhancing the user experience and reliability of the command.

    8
    Enhancement
    Parse and validate the entries option before updating the secret

    Parse and validate the entries option before passing it to the controller to ensure
    proper formatting and prevent potential errors.

    apps/cli/src/commands/secret/update.secret.ts [131-152]

     async action({ args, options }: CommandActionData): Promise<void> {
       const [secretSlug] = args
    +  const { entries, ...otherOptions } = options
    +
    +  const parsedEntries = entries ? this.parseEntries(entries) : undefined
     
       const { data, error, success } =
         await ControllerInstance.getInstance().secretController.updateSecret(
           {
             secretSlug,
    -        ...options
    +        ...otherOptions,
    +        entries: parsedEntries
           },
           this.headers
         )
     
       if (success) {
         Logger.info(`Secret ${data.name} (${data.slug}) updated successfully!`)
         Logger.info(`Created at ${data.createdAt}`)
         Logger.info(`Updated at ${data.updatedAt}`)
         Logger.info(`Note: ${data.note}`)
         Logger.info(`rotateAfter: ${data.rotateAfter}`)
       } else {
         Logger.error(`Failed to update secret: ${error.message}`)
       }
     }
     
    +private parseEntries(entries: string[]): Array<{ value: string; environmentSlug: string }> {
    +  return entries.map((entry) => {
    +    const [value, environmentSlug] = entry.split('=')
    +    if (!value || !environmentSlug) {
    +      throw new Error(`Invalid entry format: ${entry}`)
    +    }
    +    return { value, environmentSlug }
    +  })
    +}
    +
    Suggestion importance[1-10]: 7

    Why: Parsing and validating the entries option ensures that the data is correctly formatted before being sent to the controller, reducing the risk of errors and improving data integrity.

    7

    @anudeeps352 anudeeps352 changed the title fix(cli) :Operate on secret cli - removed unnecessary console log fix(cli): Operate on secret cli - removed unnecessary console log Oct 28, 2024
    @rajdip-b rajdip-b changed the title fix(cli): Operate on secret cli - removed unnecessary console log fix(cli): Removed unnecessary console log in secrets Oct 29, 2024
    @rajdip-b
    Copy link
    Member

    I think somehow you have comitted all the files again.

    @anudeeps352
    Copy link
    Contributor Author

    Yeah I was also thinking the same. I dunno how it happened,I only changed the console log

    @anudeeps352
    Copy link
    Contributor Author

    Hey if its easy enough then ig you can directly modify the file itself and just close this pr right ? I dont think such a small issue requires a pr

    Copy link
    Contributor

    Persistent review updated to latest commit 96e99d6

    @anudeeps352
    Copy link
    Contributor Author

    Ig now its fine right ?

    @rajdip-b rajdip-b merged commit 9403cc4 into keyshade-xyz:develop Oct 30, 2024
    7 checks passed
    rajdip-b pushed a commit that referenced this pull request Nov 5, 2024
    ## [2.7.0](v2.6.0...v2.7.0) (2024-11-05)
    
    ### 🚀 Features
    
    * **cli:** Add functionality to operate on Variables ([#514](#514)) ([32d93e6](32d93e6))
    * **platform:** Create ui link for resend otp ([#489](#489)) ([46eb5c5](46eb5c5))
    
    ### 🐛 Bug Fixes
    
    * **api,api-client:** Add environmentSlug in multiple places across the [secure] module ([#509](#509)) ([ee58f07](ee58f07))
    * **cli:** Removed unnecessary console log in [secure]s ([#515](#515)) ([9403cc4](9403cc4))
    
    ### 🔧 Miscellaneous Chores
    
    * Fixed lint issues ([835397a](835397a))
    * Minor housekeeping ([922bf31](922bf31))
    * Update eslint ([c583718](c583718))
    * Update eslint ([7c0c596](7c0c596))
    * Update pnpx commands to pnpm dlx ([#511](#511)) ([534a231](534a231))
    @rajdip-b
    Copy link
    Member

    rajdip-b commented Nov 5, 2024

    🎉 This PR is included in version 2.7.0 🎉

    The release is available on GitHub release

    Your semantic-release bot 📦🚀

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    CLI: Add functionality to operate on Secrets
    2 participants