Skip to content

Conversation

@montsamu
Copy link

@montsamu montsamu commented Jul 25, 2024

User description

There are network topologies and configurations where a proxy for the remote connection makes sense, and you do not want to set a system-wide proxy using environment variables. This small PR allows this with the smallest possible change and effort, instead of requiring users to subclass RemoteConnection, e.g.

"""A simple extension of Selenium RemoteConnection that allows a parameterized proxy instead of
solely setting a proxy via global ENV vars"""
from selenium.webdriver.remote.remote_connection import RemoteConnection

class ProxiedRemoteConnection(RemoteConnection): # pylint:disable=too-few-public-methods
    """Extends RemoteConnection to allow for a `proxy_url` parameter which is used in an
    override implementation of _get_proxy_url"""
    def __init__(self, remote_server_addr: str, keep_alive: bool = False, ignore_proxy: bool = False, proxy_url = None):
        """Extends RemoteConnection to allow for a `proxy_url` init parameter"""
        self.__proxy_url = proxy_url
        super().__init__(remote_server_addr, keep_alive, ignore_proxy)
    def _get_proxy_url(self):
        """Overrides RemoteConnection to return the init `proxy_url` if supplied"""
        if self.__proxy_url:
            return self.__proxy_url
        return super()._get_proxy_url()

Description

A non-breaking additional kwarg parameter to RemoteConnection which allows pass-in of a proxy_url to use.

Motivation and Context

There are network topologies and configurations where a proxy for the remote connection makes sense, and you do not want to set a system-wide proxy using environment variables. This small PR allows this with the smallest possible change and effort, instead of requiring users to subclass RemoteConnection

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

  • Introduced a new proxy_url parameter to the RemoteConnection class's __init__ method, allowing users to specify a proxy URL directly.
  • Updated the proxy URL assignment logic to prioritize the provided proxy_url over the environment variable or default behavior.
  • This change enables more flexible network configurations without requiring system-wide proxy settings.

Changes walkthrough 📝

Relevant files
Enhancement
remote_connection.py
Add support for `proxy_url` parameter in RemoteConnection

py/selenium/webdriver/remote/remote_connection.py

  • Added proxy_url parameter to __init__ method.
  • Modified proxy URL assignment logic to use the new proxy_url parameter
    if provided.
  • +2/-2     

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    There are network topologies and configurations where a proxy for the remote connection makes sense, and you do not want to set a system-wide proxy using environment variables.
    @CLAassistant
    Copy link

    CLAassistant commented Jul 25, 2024

    CLA assistant check
    All committers have signed the CLA.

    @qodo-merge-pro qodo-merge-pro bot added the P-enhancement PR with a new feature label Jul 25, 2024
    @qodo-merge-pro
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Code Clarity
    The initialization of _proxy_url in line 274 is complex and could be simplified for better readability. Consider breaking it down into multiple lines or adding comments to explain the logic.

    @qodo-merge-pro
    Copy link
    Contributor

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Maintainability
    Refactor the complex inline conditional assignment of self._proxy_url for better readability

    Refactor the conditional assignment of self._proxy_url to enhance readability and
    maintainability. Splitting complex inline conditions into multiple lines or separate
    statements can improve code clarity.

    py/selenium/webdriver/remote/remote_connection.py [274]

    -self._proxy_url = proxy_url if proxy_url else self._get_proxy_url() if not ignore_proxy else None
    +if proxy_url:
    +    self._proxy_url = proxy_url
    +elif not ignore_proxy:
    +    self._proxy_url = self._get_proxy_url()
    +else:
    +    self._proxy_url = None
     
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: Refactoring the conditional assignment of self._proxy_url into multiple lines significantly enhances readability and maintainability, making the code easier to understand and less error-prone.

    9
    Best practice
    Initialize proxy_url with a default empty string to enhance type consistency and error handling

    Consider initializing proxy_url with a default value that is more descriptive or
    indicative of its optional nature, such as an empty string, rather than None. This
    can help in type consistency and avoid potential NoneType errors when the variable
    is used elsewhere without checks.

    py/selenium/webdriver/remote/remote_connection.py [251]

    -def __init__(self, remote_server_addr: str, keep_alive: bool = False, ignore_proxy: bool = False, proxy_url: str = None):
    +def __init__(self, remote_server_addr: str, keep_alive: bool = False, ignore_proxy: bool = False, proxy_url: str = ''):
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Initializing proxy_url with an empty string instead of None can improve type consistency and prevent potential NoneType errors. However, it may require additional checks elsewhere in the code to handle the empty string case.

    7

    @diemol diemol requested a review from AutomatedTester July 29, 2024 11:46
    @VietND96
    Copy link
    Member

    I don't think it's still valid after ClientConfig was added from release 4.26.0.
    Please refer to the usage of ClientConfig to add the proxy

    @VietND96 VietND96 closed this Nov 12, 2024
    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.

    3 participants