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 observer cruise schema #1551

Merged
merged 1 commit into from
Jan 14, 2025
Merged

Conversation

wintonzheng
Copy link
Contributor

@wintonzheng wintonzheng commented Jan 14, 2025

Important

Fix ObserverCruise schema by changing url type, adding new fields, and updating URL validation.

  • Schema Changes:
    • In observers.py, change url type from HttpUrl to str in ObserverCruise.
    • Add totp_verification_url, totp_identifier, proxy_location, and webhook_callback_url to ObserverCruise.
    • Add ProxyLocation import from schemas/tasks.py.
    • Add validate_urls method to validate url, webhook_callback_url, and totp_verification_url.
  • Service Changes:
    • Remove warning log for missing plan in run_observer_cruise_helper in observer_service.py.

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

<!-- ELLIPSIS_HIDDEN -->

> [!IMPORTANT]
> Update `ObserverCruise` schema in `observers.py` to change `url` type, add new fields, and enhance URL validation.
>
>   - **Schema Changes**:
>     - In `ObserverCruise`, change `url` type from `HttpUrl` to `str`.
>     - Add fields `totp_verification_url`, `totp_identifier`, `proxy_location`, and `webhook_callback_url` to `ObserverCruise`.
>   - **Validation**:
>     - Add `validate_urls` method to `ObserverCruise` to validate `url`, `webhook_callback_url`, and `totp_verification_url` using `validate_url`.
>
> <sup>This description was created by </sup>[<img alt="Ellipsis" src="https://img.shields.io/badge/Ellipsis-blue?color=175173">](https://www.ellipsis.dev?ref=Skyvern-AI%2Fskyvern-cloud&utm_source=github&utm_medium=referral)<sup> for 2f7114620e0de4b1f8e4dcd42cf8dcc3d4fbcbc4. It will automatically update as commits are pushed.</sup>

<!-- ELLIPSIS_HIDDEN -->
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 4789d0b in 44 seconds

More details
  • Looked at 58 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. skyvern/forge/sdk/schemas/observers.py:34
  • Draft comment:
    Changing the type of url from HttpUrl to str reduces the strictness of URL validation. Ensure that validate_url provides adequate validation to compensate for this change.
  • Reason this comment was not posted:
    Comment did not seem useful.
2. skyvern/forge/sdk/services/observer_service.py:394
  • Draft comment:
    The removal of the check for 'plan' in the observer response could lead to issues if 'plan' is expected to be present. Consider adding a check or handling for when 'plan' is missing.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    The removed check was just logging a warning when plan was missing, but didn't affect execution flow. The code continues running whether plan exists or not. The plan variable is only used as an optional field in the output object. There's no evidence that plan is required for correct operation.
    I could be missing some implicit requirement that plan must exist. The warning may have been important for debugging.
    The code clearly shows plan is used only optionally. If plan was required, the code would fail later when trying to use it, but it doesn't. The warning wasn't actionable.
    The check for plan was unnecessary since plan is only used as an optional field. Removing it is safe.

Workflow ID: wflow_skbynyv0BB4Y1nsW


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

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 4789d0b in 1 minute and 32 seconds

More details
  • Looked at 58 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. skyvern/forge/sdk/schemas/observers.py:34
  • Draft comment:
    Consider using HttpUrl instead of str for the url field to ensure URL validation.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    The code has deliberately moved away from using HttpUrl to a custom validation approach. The validate_url() function is used consistently across multiple URL fields in the file. This seems to be an intentional design choice to have more control over URL validation. The comment is suggesting to revert a deliberate change that has been implemented systematically.
    Maybe HttpUrl provides additional validation or features that the custom validator doesn't have? Maybe there's a good reason to use Pydantic's built-in validator?
    The consistent use of custom validation across multiple classes and fields suggests this was a deliberate architectural decision. If HttpUrl had advantages, they would likely have kept it.
    Delete the comment as it's suggesting to revert an intentional, systematic change to URL validation that has been consistently implemented throughout the file.
2. skyvern/forge/sdk/services/observer_service.py:391
  • Draft comment:
    Reconsider removing the check for 'plan' in the observer response. Ensure 'plan' is validated before proceeding to avoid potential issues.
  • Reason this comment was not posted:
    Comment did not seem useful.

Workflow ID: wflow_Nasqmd7dVM5Qhdrx


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

@wintonzheng wintonzheng merged commit e4ad2ff into main Jan 14, 2025
6 checks passed
@wintonzheng wintonzheng deleted the shu/fix_observer_cruise_schema branch January 14, 2025 21:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant