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

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

Merged
merged 39 commits into from
Jul 31, 2024

Conversation

vr-varad
Copy link
Contributor

@vr-varad vr-varad commented Jul 6, 2024

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

  • 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, Dependencies


Description

  • Defined EnvironmentCommand interface for environment operations.
  • Implemented EnvironmentCommand interface in EnvironmentCommandImpl class with methods to list, get, create, update, and delete environments.
  • Created EnvironmentController class for making API calls related to environments.
  • Added axios dependency to handle HTTP requests.
  • Updated pnpm-lock.yaml to include axios and its dependencies.

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

    Copy link
    Contributor

    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.

    Copy link
    Contributor

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

    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]

    -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]

    -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]

    -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]

    -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 rajdip-b changed the title cli: Add functionality to operate on Environments feat(cli): Add functionality to operate on Environments Jul 6, 2024
    @rajdip-b
    Copy link
    Member

    rajdip-b commented Jul 6, 2024

    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!

    @vr-varad
    Copy link
    Contributor Author

    vr-varad commented Jul 7, 2024

    Thanks @rajdip-b working on it

    @vr-varad
    Copy link
    Contributor Author

    vr-varad commented Jul 9, 2024

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

    Copy link
    Member

    @rajdip-b rajdip-b left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    The structure looks good. You can continue your work. Here are the things you might want to fix:

    • There can be many errors when you are calling the endpoints from the environment controller. It will be best to show the error that the API has send, rather than writing your own error message.
    • You forgot to tie up the EnvironmentCommand in index.ts

    apps/cli/src/commands/environment.command.ts Outdated Show resolved Hide resolved
    apps/cli/src/types/command/environment.types.d.ts Outdated Show resolved Hide resolved
    @vr-varad
    Copy link
    Contributor Author

    @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
    Copy link
    Member

    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
    Copy link
    Contributor Author

    @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.

    Copy link
    Member

    @rajdip-b rajdip-b left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    Here are my reviews:

    • stick to using camelCase names.
    • Use the controller from api-client package and delete the one you made.

    apps/cli/src/commands/environment/create.env.ts Outdated Show resolved Hide resolved
    package.json Outdated Show resolved Hide resolved
    @vr-varad
    Copy link
    Contributor Author

    @rajdip-b

    @rajdip-b
    Copy link
    Member

    @rajdip-b

    I'll get started with this.

    @rajdip-b
    Copy link
    Member

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

    @vr-varad
    Copy link
    Contributor Author

    Oh @rajdip-b yeah sure.

    apps/cli/src/commands/environment/create.environment.ts Outdated Show resolved Hide resolved
    apps/cli/src/commands/environment/create.environment.ts Outdated Show resolved Hide resolved
    apps/cli/src/commands/environment/create.environment.ts Outdated Show resolved Hide resolved
    apps/cli/src/commands/environment/delete.environment.ts Outdated Show resolved Hide resolved
    apps/cli/src/commands/environment/get.environment.ts Outdated Show resolved Hide resolved
    apps/cli/src/commands/environment/list.environment.ts Outdated Show resolved Hide resolved
    apps/cli/tsconfig.json Outdated Show resolved Hide resolved
    @vr-varad vr-varad requested a review from rajdip-b July 30, 2024 05:46
    Copy link
    Member

    @rajdip-b rajdip-b left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    • Use Logger.error for log messages that are errors
    • image This is how you should be using the spinners and intro, outro combination. In most of the places, you are starting the spinner after you have called the api.

    Copy link
    Member

    @rajdip-b rajdip-b left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    You would need to initialize and start the spinner BEFORE you make any calls to the API. That way the spinner will actually spin and is good DX. spin.start('') is what you will use in place of intro.

    @vr-varad vr-varad requested a review from rajdip-b July 31, 2024 05:31
    @rajdip-b
    Copy link
    Member

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

    @rajdip-b
    Copy link
    Member

    @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:

    • I've removed intro, outro and spinner from CLI and user plain Logger for now. We will come to DX in a separate PR.
    • I've updated how the response works in api client
    • Replaced try-catch based error handling in CLI to if-else based as per the new flow in command action methods.

    @rajdip-b rajdip-b merged commit a26f7c1 into keyshade-xyz:develop Jul 31, 2024
    5 checks passed
    @vr-varad
    Copy link
    Contributor Author

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

    rajdip-b added a commit that referenced this pull request Sep 5, 2024
    Co-authored-by: vr-varad <varadgupta21#gmail.com>
    Co-authored-by: Rajdip Bhattacharya <agentR47@gmail.com>
    rajdip-b pushed a commit that referenced this pull request Sep 5, 2024
    ## [2.4.0](v2.3.0...v2.4.0) (2024-09-05)
    
    ### 🚀 Features
    
    * **api-client:** Create controller for Event module ([#399](#399)) ([122df35](122df35))
    * **api-client:** Create controller for Integration module ([#397](#397)) ([697d38b](697d38b))
    * **api-client:** Create controller for Project module ([#370](#370)) ([fa25866](fa25866))
    * **api-client:** Create controller for Secret module ([#396](#396)) ([7e929c0](7e929c0))
    * **api-client:** Create controller for Variable module ([#395](#395)) ([3e114d9](3e114d9))
    * **api:** Add global search in workspace ([c49962b](c49962b))
    * **api:** Add max page size ([#377](#377)) ([ed18eb0](ed18eb0))
    * **cli:** Add functionality to operate on Environments ([#324](#324)) ([4c6f3f8](4c6f3f8))
    * **cli:** Quit on decryption failure ([#381](#381)) ([1349d15](1349d15))
    
    ### 🐛 Bug Fixes
    
    * **api-client:** Fixed broken export ([096df2c](096df2c))
    * **api:** Add NotFound exception on passing an invalid roleId while inviting user in workspace ([#408](#408)) ([ab441db](ab441db))
    * **cli:** Fixed missing module ([f7a091f](f7a091f))
    * **platform:**  Build failure in platform ([#385](#385)) ([90dcb2c](90dcb2c))
    
    ### 🔧 Miscellaneous Chores
    
    * Add api client build script and updated CI ([da0e27a](da0e27a))
    * **api:** Reorganized import using path alias ([d5befd1](d5befd1))
    * **ci:** Update CLI CI name ([8f4c456](8f4c456))
    * **cli:** Add Zod validation to parseInput function ([#362](#362)) ([34e6c39](34e6c39))
    * Fixed api client tests and rearranged controllers ([1307604](1307604))
    * Housekeeping ([c5f1330](c5f1330))
    * **platform:** Added strict null check ([072254f](072254f))
    * **web:** Added strict null check ([7e12b47](7e12b47))
    
    ### 🔨 Code Refactoring
    
    * **api:** Update logic for forking projects ([#398](#398)) ([4cf3838](4cf3838))
    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 Environments
    2 participants