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): Api health probe #645

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

csehatt741
Copy link
Contributor

@csehatt741 csehatt741 commented Jan 20, 2025

User description

Description

For commands invoking API endpoints, the CLI first checks if the backend is reachable before executing the command itself by invoking the /api/health endpoint in order to give an accurate error message to the user about the issue.

Fixes #601

Screenshots of relevant screens

image

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, Bug fix


Description

  • Added API health check in BaseCommand to validate backend connectivity.

  • Implemented canMakeHttpRequests method in all command classes to enable HTTP checks.

  • Introduced AppController for handling /api/health endpoint requests.

  • Updated ControllerInstance to initialize and manage AppController.


Changes walkthrough 📝

Relevant files
Enhancement
44 files
base.command.ts
Added API health check in BaseCommand.                                     
+16/-4   
create.environment.ts
Added canMakeHttpRequests override for environment creation.
+4/-0     
delete.environment.ts
Added canMakeHttpRequests override for environment deletion.
+4/-0     
get.environment.ts
Added canMakeHttpRequests override for fetching environment details.
+4/-0     
list.environment.ts
Added canMakeHttpRequests override for listing environments.
+4/-0     
update.environment.ts
Added canMakeHttpRequests override for updating environment details.
+4/-0     
create.project.ts
Added canMakeHttpRequests override for project creation. 
+4/-0     
delete.project.ts
Added canMakeHttpRequests override for project deletion. 
+4/-0     
fork.project.ts
Added canMakeHttpRequests override for project forking.   
+4/-0     
get.project.ts
Added canMakeHttpRequests override for fetching project details.
+4/-0     
list-forks.project.ts
Added canMakeHttpRequests override for listing project forks.
+4/-0     
list.project.ts
Added canMakeHttpRequests override for listing projects. 
+4/-0     
sync.project.ts
Added canMakeHttpRequests override for syncing projects. 
+4/-0     
unlink.project.ts
Added canMakeHttpRequests override for unlinking projects.
+4/-0     
update.project.ts
Added canMakeHttpRequests override for updating project details.
+4/-0     
create.secret.ts
Added canMakeHttpRequests override for secret creation.   
+4/-0     
delete.secret.ts
Added canMakeHttpRequests override for secret deletion.   
+4/-0     
get.secret.ts
Added canMakeHttpRequests override for fetching secret details.
+4/-0     
list.secret.ts
Added canMakeHttpRequests override for listing secrets.   
+4/-0     
revisions.secret.ts
Added canMakeHttpRequests override for fetching secret revisions.
+4/-0     
rollback.secret.ts
Added canMakeHttpRequests override for rolling back secrets.
+4/-0     
update.secret.ts
Added canMakeHttpRequests override for updating secret details.
+4/-0     
create.variable.ts
Added canMakeHttpRequests override for variable creation.
+4/-0     
delete.variable.ts
Added canMakeHttpRequests override for variable deletion.
+4/-0     
list.variable.ts
Added canMakeHttpRequests override for listing variables.
+4/-0     
revisions.variable.ts
Added canMakeHttpRequests override for fetching variable revisions.
+4/-0     
rollback.variable.ts
Added canMakeHttpRequests override for rolling back variables.
+4/-0     
update.variable.ts
Added canMakeHttpRequests override for updating variable details.
+4/-0     
create.workspace.ts
Added canMakeHttpRequests override for workspace creation.
+4/-0     
delete.workspace.ts
Added canMakeHttpRequests override for workspace deletion.
+4/-0     
export.workspace.ts
Added canMakeHttpRequests override for exporting workspaces.
+4/-0     
get.workspace.ts
Added canMakeHttpRequests override for fetching workspace details.
+4/-0     
list.workspace.ts
Added canMakeHttpRequests override for listing workspaces.
+4/-0     
get-all-members.membership.ts
Added canMakeHttpRequests override for fetching workspace members.
+4/-0     
create.role.ts
Added canMakeHttpRequests override for workspace role creation.
+4/-0     
delete.role.ts
Added canMakeHttpRequests override for workspace role deletion.
+4/-0     
get.role.ts
Added canMakeHttpRequests override for fetching workspace role
details.
+4/-0     
list.role.ts
Added canMakeHttpRequests override for listing workspace roles.
+4/-0     
update.role.ts
Added canMakeHttpRequests override for updating workspace roles.
+4/-0     
search.workspace.ts
Added canMakeHttpRequests override for searching workspaces.
+4/-0     
update.workspace.ts
Added canMakeHttpRequests override for updating workspace details.
+4/-0     
controller-instance.ts
Added AppController initialization in ControllerInstance.
+11/-0   
app.ts
Implemented AppController for API health checks.                 
+14/-0   
index.ts
Exported AppController from API client package.                   
+2/-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.
  • Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis ✅

    601 - Fully compliant

    Compliant requirements:

    • Add health check probe to CLI before making HTTP requests
    • Implement health check in BaseCommand's prepare function using /api/health endpoint
    • Show error message if server is not reachable
    • Only check health for commands that make HTTP requests
    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Error Handling

    The catch block for health check API call only throws a generic connection error. Should also handle and display specific error cases like invalid API key, unauthorized access etc.

    try {
      await ControllerInstance.getInstance().appController.health(
        this.headers
      )
    } catch {
      throw new Error(
        `Could not connect to the server: ${this.baseUrl}.`
      )
    }

    Copy link
    Contributor

    codiumai-pr-agent-free bot commented Jan 20, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    ✅ Add timeout to health check

    Add error handling for network timeouts and connection issues by implementing a
    timeout for the health check request. This prevents the CLI from hanging
    indefinitely on network problems.

    apps/cli/src/commands/base.command.ts [51-59]

     try {
    -  await ControllerInstance.getInstance().appController.health(
    -    this.headers
    -  )
    -} catch {
    +  const timeoutPromise = new Promise((_, reject) => 
    +    setTimeout(() => reject(new Error('Request timed out')), 5000)
    +  );
    +  await Promise.race([
    +    ControllerInstance.getInstance().appController.health(this.headers),
    +    timeoutPromise
    +  ]);
    +} catch (error) {
       throw new Error(
    -    `Could not connect to the server: ${this.baseUrl}.`
    -  )
    +    `Could not connect to the server: ${this.baseUrl}. ${error instanceof Error ? error.message : ''}`
    +  );
     }

    [Suggestion has been applied]

    Suggestion importance[1-10]: 8

    Why: The suggestion adds important timeout handling for network requests, preventing the CLI from hanging indefinitely on connection issues. This is a critical improvement for user experience and error handling robustness.

    8

    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 missed adding canMakeHttpRequests to these files:

    • search.workspace.ts
    • update.workspace.ts

    @csehatt741
    Copy link
    Contributor Author

    • search.workspace.ts

    They are also included in this PR

    @csehatt741 csehatt741 requested a review from rajdip-b January 20, 2025 14:30
    @rajdip-b
    Copy link
    Member

    Oh, almost forgot, could you also include this in CHANGELOG.md under apps/cli please?

    @csehatt741
    Copy link
    Contributor Author

    apps/cli changelog updated

    @csehatt741 csehatt741 requested a review from rajdip-b January 21, 2025 07:04
    Copy link

    codecov bot commented Jan 21, 2025

    Codecov Report

    All modified and coverable lines are covered by tests ✅

    Project coverage is 96.20%. Comparing base (ce50743) to head (4fbd208).
    Report is 318 commits behind head on develop.

    Additional details and impacted files
    @@             Coverage Diff             @@
    ##           develop     #645      +/-   ##
    ===========================================
    + Coverage    91.71%   96.20%   +4.48%     
    ===========================================
      Files          111       14      -97     
      Lines         2510      316    -2194     
      Branches       469        6     -463     
    ===========================================
    - Hits          2302      304    -1998     
    + Misses         208       12     -196     
    Flag Coverage Δ
    api-client 96.20% <ø> (?)
    api-e2e-tests ?

    Flags with carried forward coverage won't be shown. Click here to find out more.

    ☔ View full report in Codecov by Sentry.
    📢 Have feedback on the report? Share it here.

    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 API health probe
    2 participants