keyshade-xyz / keyshade

Realtime secret and configuration management tool, with the best in class security and seamless integration support
https://keyshade.xyz
Mozilla Public License 2.0
196 stars 96 forks source link

feat(cli): Add functionality to operate on Environments #324

Closed vr-varad closed 2 months ago

vr-varad commented 2 months ago

User description

Description

Give a summary of the change that you have made

Fixes #300

Dependencies

axios

Future Improvements

will be adding apis

Mentions

Mention and tag the people

Screenshots of relevant screens

Add screenshots of relevant screens

Developer's checklist

If changes are made in the code:

Documentation Update


PR Type

Enhancement, Dependencies


Description


Changes walkthrough ๐Ÿ“

Relevant files
Enhancement
command.interface.ts
Define `EnvironmentCommand` interface for environment operations

apps/cli/src/commands/command.interface.ts
  • Added EnvironmentCommand interface with methods for environment
    operations.
  • +7/-0     
    environment.command.ts
    Implement `EnvironmentCommand` interface in `EnvironmentCommandImpl`

    apps/cli/src/commands/environment/environment.command.ts
  • Implemented EnvironmentCommand interface in EnvironmentCommandImpl
    class.
  • Added methods to list, get, create, update, and delete environments.
  • +35/-0   
    project.ts
    Create `EnvironmentController` class for environment API calls

    apps/cli/src/http/project.ts
  • Created EnvironmentController class with methods for API calls.
  • Added methods to list, get, create, update, and delete environments.
  • +31/-0   
    Dependencies
    package.json
    Add `axios` dependency and reorder dependencies                   

    package.json
  • Added axios dependency.
  • Reordered dependencies for consistency.
  • +6/-5     
    pnpm-lock.yaml
    Update lock file for `axios` dependency                                   

    pnpm-lock.yaml - Updated lock file to include `axios` and its dependencies.
    +25/-0   

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

    codiumai-pr-agent-free[bot] commented 2 months ago

    PR Reviewer Guide ๐Ÿ”

    โฑ๏ธ Estimated effort to review: 3 ๐Ÿ”ต๐Ÿ”ต๐Ÿ”ตโšชโšช
    ๐Ÿงช No relevant tests
    ๐Ÿ”’ No security concerns identified
    โšก Key issues to review

    **Possible Bug:** The `EnvironmentController` methods do not handle API responses correctly. The variable `response` is used but never assigned a value from an actual API call, which will lead to runtime errors. **Code Quality:** The methods in `EnvironmentController` are returning `response.data` directly without checking if the response is successful or handling potential errors. **Logging:** The use of `console.log` for outputting results in `EnvironmentCommandImpl` methods is not suitable for production environments. Consider using a more robust logging approach or handling the outputs differently.
    codiumai-pr-agent-free[bot] commented 2 months ago

    PR Code Suggestions โœจ

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add error handling to manage potential errors in the list method ___ **Consider adding error handling in the list method to manage potential errors from the
    controller.listEnvironments call.** [apps/cli/src/commands/environment/environment.command.ts [12-13]](https://github.com/keyshade-xyz/keyshade/pull/324/files#diff-58c8ab24f06cfcab7b301408a585175b5c2adcb88125d21216f185d7a213badcR12-R13) ```diff -const environments = await this.controller.listEnvironments(projectId) -console.log(environments) +try { + const environments = await this.controller.listEnvironments(projectId) + console.log(environments) +} catch (error) { + console.error(`Failed to list environments for project ${projectId}:`, error) +} ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 8 Why: Adding error handling is crucial for robustness, especially for network operations which are prone to failures.
    8
    Add error handling to manage potential errors in the get method ___ **Add error handling in the get method to manage potential errors from the
    controller.getEnvironment call.** [apps/cli/src/commands/environment/environment.command.ts [17-18]](https://github.com/keyshade-xyz/keyshade/pull/324/files#diff-58c8ab24f06cfcab7b301408a585175b5c2adcb88125d21216f185d7a213badcR17-R18) ```diff -const environment = await this.controller.getEnvironment(environmentId) -console.log(environment) +try { + const environment = await this.controller.getEnvironment(environmentId) + console.log(environment) +} catch (error) { + console.error(`Failed to get environment ${environmentId}:`, error) +} ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 8 Why: Error handling in the `get` method is essential to handle potential API call failures, improving the reliability of the application.
    8
    Add error handling to manage potential errors in the create method ___ **Add error handling in the create method to manage potential errors from the
    controller.createEnvironment call.** [apps/cli/src/commands/environment/environment.command.ts [22-23]](https://github.com/keyshade-xyz/keyshade/pull/324/files#diff-58c8ab24f06cfcab7b301408a585175b5c2adcb88125d21216f185d7a213badcR22-R23) ```diff -const newEnvironment = await this.controller.createEnvironment(projectId) -console.log(newEnvironment) +try { + const newEnvironment = await this.controller.createEnvironment(projectId) + console.log(newEnvironment) +} catch (error) { + console.error(`Failed to create environment for project ${projectId}:`, error) +} ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 8 Why: Implementing error handling in the `create` method is important to manage exceptions and provide feedback on operation failures.
    8
    Add error handling to manage potential errors in the update method ___ **Add error handling in the update method to manage potential errors from the
    controller.updateEnvironment call.** [apps/cli/src/commands/environment/environment.command.ts [27-28]](https://github.com/keyshade-xyz/keyshade/pull/324/files#diff-58c8ab24f06cfcab7b301408a585175b5c2adcb88125d21216f185d7a213badcR27-R28) ```diff -const updatedEnvironment = await this.controller.updateEnvironment(environmentId) -console.log(updatedEnvironment) +try { + const updatedEnvironment = await this.controller.updateEnvironment(environmentId) + console.log(updatedEnvironment) +} catch (error) { + console.error(`Failed to update environment ${environmentId}:`, error) +} ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 8 Why: The suggestion to add error handling in the `update` method is valid as it enhances the application's error management and user experience.
    8
    rajdip-b commented 2 months ago

    Hey @vr-varad! This PR doesn't follow the development conventions for our CLI unfortunately.

    Could you please refer to the profile command to understand how this works?

    Feel free to ping us if you feel you are stuck somewhere :) We will never want you to redo everything!

    rajdip-b commented 2 months ago

    @vr-varad You can use these docs links:

    vr-varad commented 2 months ago

    Thanks @rajdip-b working on it

    vr-varad commented 2 months ago

    @rajdip-b I have added implementation for list env can u please review it whenever you are free?

    vr-varad commented 2 months ago

    @rajdip-b I have made the changes as requested and made a good progress but I am stuck right now

    1. How should I access the base_url and api_key in the classes in command/environment?
    2. Same with the environment data?
    rajdip-b commented 2 months ago
    1. The BaseCommand automatically parses the base URL and API key. You can reference them in any of the subclasses using this.baseUrl and this.apiKey. Refer to this to see how to pass in the keys. Also, the profile use <profile_name>](https://docs.keyshade.xyz/cli/profile) comes in handy to set up a profile and use it. For example, you have created a profile that has your current api key and base url, and set it as the default one. For the subsequent commands that will invoke the API, you wont need to set the global --api-key, --base-url or --profile flags
    2. The environment data will be fetched from the API. All of the environment management needs you to invoke the API methods using the controller util functions.
    vr-varad commented 2 months ago

    @rajdip-b I have made the required changes from what I could understand. I need a check from you so I can know if I am wrong. what I got if u are creating an env u must have a profile in turn having baseurl and apikey use I use this to fetch both.

    vr-varad commented 2 months ago

    @rajdip-b

    rajdip-b commented 2 months ago

    @rajdip-b

    I'll get started with this.

    vr-varad commented 2 months ago

    @rajdip-b Thanks bro for helping out.

    vr-varad commented 2 months ago

    @rajdip-b done.

    vr-varad commented 2 months ago

    @rajdip-b resolved all the requested changes please give it a review whenever you are free and let me know if any changes are required.

    vr-varad commented 2 months ago

    @rajdip-b Sorry for not paying attention, won't happen again. Error Resolved

    rajdip-b commented 2 months ago

    This looks good. I'll test it out locally and let you know

    rajdip-b commented 2 months ago

    @vr-varad could you please finish up this PR?

    vr-varad commented 2 months ago

    Oh @rajdip-b yeah sure.

    rajdip-b commented 2 months ago

    @vr-varad can you fix the conflict? I've just pushed some new changes. After this, I can test your PR

    rajdip-b commented 2 months ago

    @vr-varad hey man, I found a lot of errors in the code. I expected you to test them out when you built the feature. I've fixed all the errors and also refactored how responses are dealt with in the api-client.

    You have other PRs related to the API client and CLI. I would like you to follow this PR example and implement the changes in there.

    Things to note while updating the other PRs:

    vr-varad commented 2 months ago

    @rajdip-b Thanks for the guidance, I will surely go through this pr and make further changes in those prs. Thanks bro.