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

[ci] Update script pinned_browsers for changes in Edge API product fetch #14865

Merged
merged 1 commit into from
Dec 6, 2024

Conversation

VietND96
Copy link
Member

@VietND96 VietND96 commented Dec 6, 2024

User description

Thanks for contributing to Selenium!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines.
Avoid large PRs, help reviewers by making them as simple and short as possible.

Description

Motivation and Context

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have read the contributing document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

PR Type

enhancement


Description

  • Updated the URLs and SHA256 checksums for various browser archives in common/repositories.bzl, including Firefox, Edge, and Chrome, to reflect new version releases.
  • Introduced new functions in scripts/pinned_browsers.py to handle JSON data in a case-insensitive manner, ensuring compatibility with changes in the Edge API response format.

Changes walkthrough 📝

Relevant files
Enhancement
repositories.bzl
Update browser version URLs and checksums                               

common/repositories.bzl

  • Updated URLs and SHA256 checksums for Firefox, Edge, and Chrome
    archives.
  • Incremented version numbers for browser releases.
  • +25/-25 
    pinned_browsers.py
    Add case-insensitive JSON handling for Edge API                   

    scripts/pinned_browsers.py

  • Added functions for case-insensitive JSON loading.
  • Modified Edge API data handling to be case-insensitive.
  • +28/-14 

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    Copy link
    Contributor

    qodo-merge-pro bot commented Dec 6, 2024

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Code Smell
    The case-insensitive JSON handling functions are implemented but not used consistently. The code still directly accesses 'productversion' after converting keys to lowercase.

    Error Handling
    No error handling for JSON parsing or HTTP requests. The code assumes the Edge API response will always be valid and contain the expected data structure.

    Copy link
    Contributor

    qodo-merge-pro bot commented Dec 6, 2024

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add error handling for external API requests to prevent silent failures

    Add error handling for the HTTP request to the Edge API endpoint. If the request
    fails, the script will currently fail silently. Add a try-except block to handle
    potential connection errors or invalid responses.

    scripts/pinned_browsers.py [194-195]

    -r = http.request("GET", "https://edgeupdates.microsoft.com/api/products")
    -all_data = case_insensitive_json_loads(r.data)
    +try:
    +    r = http.request("GET", "https://edgeupdates.microsoft.com/api/products")
    +    if r.status != 200:
    +        raise RuntimeError(f"Edge API request failed with status {r.status}")
    +    all_data = case_insensitive_json_loads(r.data)
    +except Exception as e:
    +    raise RuntimeError(f"Failed to fetch Edge browser data: {e}")
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion adds crucial error handling for an external API call, which could prevent silent failures and make debugging easier. This is important for reliability and maintainability.

    8
    Fix potential undefined variable issue by using consistent version tracking

    Validate that mac_version is defined before using it. Currently, mac_version is
    assigned but never used, and could be undefined if the matching conditions aren't
    met.

    scripts/pinned_browsers.py [207-212]

     if "MacOS" == release.get("platform"):
         for artifact in release["artifacts"]:
             if "pkg" == artifact["artifactname"]:
                 mac = artifact["location"]
                 mac_hash = artifact["hash"]
    -            mac_version = release["productversion"]
    +            version = release["productversion"]  # Use the common version variable
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion fixes a potential bug where mac_version is assigned but never used, while the common 'version' variable should be used instead for consistency throughout the code.

    7

    💡 Need additional feedback ? start a PR chat

    @VietND96 VietND96 merged commit a3af929 into trunk Dec 6, 2024
    8 of 9 checks passed
    @VietND96 VietND96 deleted the pinned-browsers branch December 6, 2024 08:30
    sandeepsuryaprasad pushed a commit to sandeepsuryaprasad/selenium that referenced this pull request Dec 7, 2024
    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.

    1 participant