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

add timezone support #1389

Merged
merged 1 commit into from
Dec 16, 2024
Merged

add timezone support #1389

merged 1 commit into from
Dec 16, 2024

Conversation

LawyZheng
Copy link
Collaborator

@LawyZheng LawyZheng commented Dec 16, 2024

Important

Add timezone support by using local time in datetime operations and configuring timezone based on proxy location.

  • Behavior:
    • Replaces utc_datetime with local_datetime in agent.py and multiple .j2 template files to use local timezone.
    • Adds tz_info to SkyvernContext in skyvern_context.py to store timezone information.
    • Updates get_tzinfo_from_proxy() in tasks.py to map ProxyLocation to ZoneInfo.
  • Context Management:
    • Uses ensure_context() in agent.py and handler.py to ensure timezone context is available.
  • Browser Configuration:
    • Modifies build_browser_args() in browser_factory.py to set timezone_id based on proxy_location.
    • Updates create_browser_context() in browser_factory.py to set tz_info in context based on proxy_location.

This description was created by Ellipsis for ff642f4. 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 ff642f4 in 1 minute and 17 seconds

More details
  • Looked at 372 lines of code in 13 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. skyvern/forge/sdk/core/skyvern_context.py:14
  • Draft comment:
    Consider setting a default timezone to avoid potential errors when context.tz_info is None. This will ensure that datetime operations do not fail due to a missing timezone.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The code changes in the PR are consistent with the intent to add timezone support. However, there is a potential issue with the default timezone handling when the proxy location is not set. The code should ensure that a default timezone is set to avoid potential errors when context.tz_info is None.

Workflow ID: wflow_YSg4URI4iFlg59GM


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

@LawyZheng LawyZheng merged commit 9b1aeff into main Dec 16, 2024
2 checks passed
@LawyZheng LawyZheng deleted the lawy/add-tz-support branch December 16, 2024 03:22
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