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

get observer cruise API #1447

Merged
merged 1 commit into from
Dec 30, 2024
Merged

get observer cruise API #1447

merged 1 commit into from
Dec 30, 2024

Conversation

wintonzheng
Copy link
Contributor

@wintonzheng wintonzheng commented Dec 29, 2024

Important

Add new API endpoint in agent_protocol.py to retrieve observer cruise details, with service function in observer_service.py.

  • New API Endpoint:
    • Adds get_observer_cruise route in agent_protocol.py to retrieve observer cruise details by observer_cruise_id.
    • Route requires organization dependency for authorization.
  • Service Function:
    • Implements get_observer_cruise in observer_service.py to fetch cruise details from the database using observer_cruise_id and organization_id.

This description was created by Ellipsis for 17131f6. It will automatically update as commits are pushed.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Reviewed everything up to 7754599 in 1 minute and 26 seconds

More details
  • Looked at 30 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. skyvern/forge/sdk/services/observer_service.py:924
  • Draft comment:
    Consider handling the case where the observer cruise is not found and return an appropriate response or raise an exception.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    The comment suggests adding error handling, but this appears to be a simple pass-through function where error handling is done by the callers. The function follows a common pattern where the caller decides how to handle missing data. Adding error handling here would reduce flexibility for callers who may want to handle the error case differently.
    The comment could be valid if there's a strong reason to enforce consistent error handling at this layer rather than leaving it to callers. Some APIs do prefer to handle errors consistently at the lowest level.
    Looking at the usage in the codebase (e.g. run_observer_cruise), the current pattern of handling errors at the caller level is working well and provides flexibility. There's no clear evidence that centralized error handling would be better.
    Delete the comment. The current pattern of handling errors at the caller level is appropriate and well-established in the codebase.

Workflow ID: wflow_qIfvjOIJT0UGrM82


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@wintonzheng wintonzheng force-pushed the shu/get_observer_cruise_api branch from 7754599 to 17131f6 Compare December 29, 2024 18:37
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Incremental review on 17131f6 in 15 seconds

More details
  • Looked at 33 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. skyvern/forge/sdk/routes/agent_protocol.py:1102
  • Draft comment:
    Consider adding a tags parameter to the @base_router.get decorator for consistency with other endpoints.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The new API endpoint in agent_protocol.py is designed to retrieve observer cruise details using the observer_cruise_id. The corresponding service function in observer_service.py fetches the cruise details from the database. The implementation seems correct, but there are some improvements and checks needed.
2. skyvern/forge/sdk/services/observer_service.py:924
  • Draft comment:
    Add error handling for database access issues in get_observer_cruise.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The get_observer_cruise function in observer_service.py is straightforward, but it lacks error handling for database access issues.

Workflow ID: wflow_C07s9nPtV8abHn0e


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@wintonzheng wintonzheng merged commit d5dacd9 into main Dec 30, 2024
2 checks passed
@wintonzheng wintonzheng deleted the shu/get_observer_cruise_api branch December 30, 2024 20:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant