Add SOCKS proxy support #2035
Open
Add SOCKS proxy support #2035
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rhatdan
requested review from
bmahabirbu,
cgruver,
engelmi,
jhjaggars,
maxamillion and
swarajpande5
as code owners
Contributor
Reviewer's GuideThis PR adds comprehensive proxy support by introducing a dedicated proxy_support module that parses environment variables, configures urllib with HTTP/SOCKS handlers (leveraging PySocks when available), integrates setup calls into core network modules, updates documentation, adds the PySocks dependency, and covers functionality with detailed unit tests. Sequence diagram for proxy setup during module importsequenceDiagram
participant "ramalama.http_client"
participant "proxy_support"
participant "urllib.request"
participant "PySocks (optional)"
"ramalama.http_client"->>"proxy_support": setup_proxy_support()
"proxy_support"->>"proxy_support": _get_proxy_env()
alt SOCKS proxy detected
"proxy_support"->>"PySocks (optional)": import socks
"proxy_support"->>"urllib.request": install_opener with SOCKS support
else Only HTTP/HTTPS proxy
"proxy_support"->>"urllib.request": install_opener with HTTP/HTTPS proxy
end
Entity relationship diagram for proxy environment variableserDiagram
PROXY_ENV {
string http_proxy
string https_proxy
string all_proxy
string no_proxy
}
PROXY_ENV ||--o| "urllib.request.ProxyHandler" : configures
"PySocks" ||--o| "SOCKS Proxy" : enables
"NO_PROXY" ||--o| "Proxy Bypass" : controls
Class diagram for the new proxy_support moduleclassDiagram
class proxy_support {
+_get_proxy_env() dict[str, str | None]
+_is_socks_proxy(proxy_url: str) bool
+setup_proxy_support() None
+get_proxy_info() dict[str, str]
}
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
Summary of ChangesHello @rhatdan, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances RamaLama's network capabilities by implementing robust SOCKS proxy support, alongside existing HTTP/HTTPS proxy functionality. The changes centralize proxy configuration through a new module, integrate it into critical network-dependent components, and ensure that users can easily configure proxy settings using standard environment variables. This provides greater flexibility for users operating in restricted network environments, improving the application's adaptability and reach. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Instead of calling setup_proxy_support in each module import, centralize proxy initialization in your main application entrypoint to avoid side effects and repeated opener installations.
- setup_proxy_support rebuilds and reinstalls the urllib opener on every invocation; consider caching or guarding to make it truly idempotent and avoid unnecessary work.
- When PySocks isn’t installed for a SOCKS proxy, you currently log a warning and silently fall back—consider making this behavior more explicit or aborting on missing dependencies to prevent confusing network failures.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Instead of calling setup_proxy_support in each module import, centralize proxy initialization in your main application entrypoint to avoid side effects and repeated opener installations.
- setup_proxy_support rebuilds and reinstalls the urllib opener on every invocation; consider caching or guarding to make it truly idempotent and avoid unnecessary work.
- When PySocks isn’t installed for a SOCKS proxy, you currently log a warning and silently fall back—consider making this behavior more explicit or aborting on missing dependencies to prevent confusing network failures.
## Individual Comments
### Comment 1
<location> `ramalama/proxy_support.py:85-86` </location>
<code_context>
+ pass
+
+ # Build the proxy handler
+ proxy_handler = urllib.request.ProxyHandler(proxy_env)
+ opener = urllib.request.build_opener(proxy_handler)
+ urllib.request.install_opener(opener)
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** ProxyHandler receives all proxy_env keys, including 'no_proxy', which may not be expected.
Remove 'no_proxy' from proxy_env before passing it to ProxyHandler, as urllib expects 'no_proxy' to be set in the environment, not as a handler argument.
Suggested implementation:
```python
# Remove 'no_proxy' from proxy_env before passing to ProxyHandler
proxy_env_for_handler = dict(proxy_env)
proxy_env_for_handler.pop('no_proxy', None)
# Build the proxy handler
```
```python
proxy_handler = urllib.request.ProxyHandler(proxy_env_for_handler)
opener = urllib.request.build_opener(proxy_handler)
urllib.request.install_opener(opener)
```
</issue_to_address>
### Comment 2
<location> `test/unit/test_proxy_support.py:195-193` </location>
<code_context>
+ def test_setup_with_socks_proxy_no_pysocks(self, monkeypatch):
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding an assertion for warning log when PySocks is missing.
Please add an assertion to verify that the expected warning is logged when PySocks is missing, using caplog or a logger mock.
Suggested implementation:
```python
def test_setup_with_socks_proxy_no_pysocks(self, monkeypatch, caplog):
"""Test setup with SOCKS proxy when PySocks is not installed."""
for var in [
'http_proxy',
'HTTP_PROXY',
'https_proxy',
'HTTPS_PROXY',
'all_proxy',
'ALL_PROXY',
'no_proxy',
'NO_PROXY',
```
```python
monkeypatch.setenv('http_proxy', 'socks5://proxy.example.com:1080')
with patch('urllib.request.install_opener') as mock_install:
with caplog.at_level("WARNING"):
setup_proxy_support()
assert not mock_install.called
assert any("PySocks is required for SOCKS proxy support" in record.message for record in caplog.records)
```
</issue_to_address>
### Comment 3
<location> `test/unit/test_proxy_support.py:234-239` </location>
<code_context>
+ setup_proxy_support()
+ assert mock_install.called
+
+ def test_setup_with_socks_proxy_with_pysocks(self, monkeypatch):
+ """Test setup with SOCKS proxy when PySocks is installed."""
+ for var in [
+ 'http_proxy',
+ 'HTTP_PROXY',
+ 'https_proxy',
+ 'HTTPS_PROXY',
+ 'all_proxy',
+ 'ALL_PROXY',
+ 'no_proxy',
+ 'NO_PROXY',
+ ]:
+ monkeypatch.delenv(var, raising=False)
+
+ monkeypatch.setenv('all_proxy', 'socks5://proxy.example.com:1080')
+
+ # Mock socks module to simulate PySocks being installed
+ mock_socks = MagicMock()
+ with patch.dict('sys.modules', {'socks': mock_socks}):
+ with patch('urllib.request.install_opener') as mock_install:
+ setup_proxy_support()
+ assert mock_install.called
+
+ def test_setup_with_multiple_proxies(self, monkeypatch):
</code_context>
<issue_to_address>
**suggestion (testing):** Test does not verify that SOCKS proxy is actually used when PySocks is present.
Please add assertions to confirm that SOCKS proxying is actually enabled, such as verifying the correct handler is installed or that the socket module is patched appropriately.
```suggestion
# Mock socks module to simulate PySocks being installed
mock_socks = MagicMock()
with patch.dict('sys.modules', {'socks': mock_socks}):
with patch('urllib.request.install_opener') as mock_install, \
patch('urllib.request.build_opener') as mock_build_opener:
# Simulate build_opener returning a mock opener with handlers
mock_opener = MagicMock()
mock_handler = MagicMock()
mock_handler.__class__.__name__ = "SocksiPyHandler"
mock_opener.handlers = [mock_handler]
mock_build_opener.return_value = mock_opener
setup_proxy_support()
assert mock_install.called
# Assert that build_opener was called and the handler is a SOCKS handler
mock_build_opener.assert_called()
assert any(
h.__class__.__name__.lower().startswith("socks") or
h.__class__.__name__.lower().startswith("socksi")
for h in mock_opener.handlers
), "SOCKS proxy handler was not installed in opener"
# Optionally, check that the socks module was used
assert mock_socks is not None
```
</issue_to_address>
### Comment 4
<location> `test/unit/test_proxy_support.py:330-332` </location>
<code_context>
+class TestProxyIntegration:
+ """Integration tests for proxy support."""
+
+ def test_proxy_handler_installed(self, monkeypatch):
+ """Test that proxy handler is correctly installed in urllib."""
+ for var in [
+ 'http_proxy',
+ 'HTTP_PROXY',
+ 'https_proxy',
+ 'HTTPS_PROXY',
+ 'all_proxy',
+ 'ALL_PROXY',
+ 'no_proxy',
+ 'NO_PROXY',
+ ]:
+ monkeypatch.delenv(var, raising=False)
+
+ monkeypatch.setenv('http_proxy', 'http://proxy.example.com:8080')
+
+ # Reset the opener to test installation
+ urllib.request.install_opener(None)
+
+ setup_proxy_support()
+
+ # Test that the opener has been configured
+ opener = urllib.request._opener
+ assert opener is not None
+
+ def test_idempotent_setup(self, monkeypatch):
</code_context>
<issue_to_address>
**suggestion (testing):** Test does not verify that the opener uses the correct proxy settings.
Add assertions to confirm that the opener's handlers include a ProxyHandler configured with the expected proxy settings.
```suggestion
# Test that the opener has been configured
opener = urllib.request._opener
assert opener is not None
# Find the ProxyHandler in the opener's handlers
proxy_handler = None
for handler in opener.handlers:
if isinstance(handler, urllib.request.ProxyHandler):
proxy_handler = handler
break
assert proxy_handler is not None, "ProxyHandler not found in opener handlers"
# Check that the proxy settings are correct
expected_proxies = {'http': 'http://proxy.example.com:8080'}
assert proxy_handler.proxies == expected_proxies
```
</issue_to_address>
### Comment 5
<location> `test/unit/test_proxy_support.py:306-315` </location>
<code_context>
+class TestProxyIntegration:
</code_context>
<issue_to_address>
**suggestion (testing):** Missing tests for NO_PROXY behavior and proxy bypass.
Please add tests to ensure requests to NO_PROXY hosts bypass the proxy as expected.
</issue_to_address>
### Comment 6
<location> `ramalama/proxy_support.py:25` </location>
<code_context>
+from ramalama.logger import logger
+
+
+def _get_proxy_env() -> dict[str, str | None]:
+ """
+ Get proxy settings from environment variables.
</code_context>
<issue_to_address>
**issue (complexity):** Consider replacing manual proxy environment parsing and custom helpers with urllib's built-in getproxies_environment and ProxyHandler for simpler proxy setup.
```suggestion
You can collapse all of this manual env-var parsing and SOCKS handling into a call to `urllib.request.getproxies_environment()` + a single `ProxyHandler`. This removes the custom `_get_proxy_env`/`_is_socks_proxy` helpers and ad-hoc logging:
Replace your `setup_proxy_support` with:
```python
import urllib.request
from ramalama.logger import logger
def setup_proxy_support() -> None:
proxies = urllib.request.getproxies_environment()
if not proxies:
logger.debug("No proxy settings detected")
return
opener = urllib.request.build_opener(urllib.request.ProxyHandler(proxies))
urllib.request.install_opener(opener)
for scheme, url in proxies.items():
if scheme == "no_proxy":
logger.debug(f"Proxy bypass list: {url}")
else:
logger.info(f"Using proxy {scheme}={url}")
```
And simplify `get_proxy_info` to:
```python
def get_proxy_info() -> dict[str, str]:
from urllib.request import getproxies_environment
raw = getproxies_environment()
return {k: v or "not set" for k, v in raw.items()}
```
This preserves full HTTP/HTTPS/SOCKS support (so long as PySocks is installed before any socket use) while removing ~100 LOC of boilerplate.
</issue_to_address>
### Comment 7
<location> `ramalama/proxy_support.py:105` </location>
<code_context>
return {k: v if v else "not set" for k, v in proxy_env.items()}
</code_context>
<issue_to_address>
**suggestion (code-quality):** Replace if-expression with `or` ([`or-if-exp-identity`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/or-if-exp-identity))
```suggestion
return {k: v or "not set" for k, v in proxy_env.items()}
```
<br/><details><summary>Explanation</summary>Here we find ourselves setting a value if it evaluates to `True`, and otherwise
using a default.
The 'After' case is a bit easier to read and avoids the duplication of
`input_currency`.
It works because the left-hand side is evaluated first. If it evaluates to
true then `currency` will be set to this and the right-hand side will not be
evaluated. If it evaluates to false the right-hand side will be evaluated and
`currency` will be set to `DEFAULT_CURRENCY`.
</details>
</issue_to_address>
### Comment 8
<location> `test/unit/test_proxy_support.py:19-29` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid loops in tests. ([`no-loop-in-tests`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/no-loop-in-tests))
<details><summary>Explanation</summary>Avoid complex code, like loops, in test functions.
Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
* loops
* conditionals
Some ways to fix this:
* Use parametrized tests to get rid of the loop.
* Move the complex logic into helpers.
* Move the complex part into pytest fixtures.
> Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / [Don't Put Logic in Tests](https://abseil.io/resources/swe-book/html/ch12.html#donapostrophet_put_logic_in_tests)
</details>
</issue_to_address>
### Comment 9
<location> `test/unit/test_proxy_support.py:36-46` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid loops in tests. ([`no-loop-in-tests`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/no-loop-in-tests))
<details><summary>Explanation</summary>Avoid complex code, like loops, in test functions.
Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
* loops
* conditionals
Some ways to fix this:
* Use parametrized tests to get rid of the loop.
* Move the complex logic into helpers.
* Move the complex part into pytest fixtures.
> Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / [Don't Put Logic in Tests](https://abseil.io/resources/swe-book/html/ch12.html#donapostrophet_put_logic_in_tests)
</details>
</issue_to_address>
### Comment 10
<location> `test/unit/test_proxy_support.py:57-67` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid loops in tests. ([`no-loop-in-tests`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/no-loop-in-tests))
<details><summary>Explanation</summary>Avoid complex code, like loops, in test functions.
Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
* loops
* conditionals
Some ways to fix this:
* Use parametrized tests to get rid of the loop.
* Move the complex logic into helpers.
* Move the complex part into pytest fixtures.
> Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / [Don't Put Logic in Tests](https://abseil.io/resources/swe-book/html/ch12.html#donapostrophet_put_logic_in_tests)
</details>
</issue_to_address>
### Comment 11
<location> `test/unit/test_proxy_support.py:78-88` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid loops in tests. ([`no-loop-in-tests`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/no-loop-in-tests))
<details><summary>Explanation</summary>Avoid complex code, like loops, in test functions.
Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
* loops
* conditionals
Some ways to fix this:
* Use parametrized tests to get rid of the loop.
* Move the complex logic into helpers.
* Move the complex part into pytest fixtures.
> Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / [Don't Put Logic in Tests](https://abseil.io/resources/swe-book/html/ch12.html#donapostrophet_put_logic_in_tests)
</details>
</issue_to_address>
### Comment 12
<location> `test/unit/test_proxy_support.py:98-108` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid loops in tests. ([`no-loop-in-tests`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/no-loop-in-tests))
<details><summary>Explanation</summary>Avoid complex code, like loops, in test functions.
Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
* loops
* conditionals
Some ways to fix this:
* Use parametrized tests to get rid of the loop.
* Move the complex logic into helpers.
* Move the complex part into pytest fixtures.
> Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / [Don't Put Logic in Tests](https://abseil.io/resources/swe-book/html/ch12.html#donapostrophet_put_logic_in_tests)
</details>
</issue_to_address>
### Comment 13
<location> `test/unit/test_proxy_support.py:117-127` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid loops in tests. ([`no-loop-in-tests`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/no-loop-in-tests))
<details><summary>Explanation</summary>Avoid complex code, like loops, in test functions.
Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
* loops
* conditionals
Some ways to fix this:
* Use parametrized tests to get rid of the loop.
* Move the complex logic into helpers.
* Move the complex part into pytest fixtures.
> Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / [Don't Put Logic in Tests](https://abseil.io/resources/swe-book/html/ch12.html#donapostrophet_put_logic_in_tests)
</details>
</issue_to_address>
### Comment 14
<location> `test/unit/test_proxy_support.py:160-170` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid loops in tests. ([`no-loop-in-tests`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/no-loop-in-tests))
<details><summary>Explanation</summary>Avoid complex code, like loops, in test functions.
Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
* loops
* conditionals
Some ways to fix this:
* Use parametrized tests to get rid of the loop.
* Move the complex logic into helpers.
* Move the complex part into pytest fixtures.
> Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / [Don't Put Logic in Tests](https://abseil.io/resources/swe-book/html/ch12.html#donapostrophet_put_logic_in_tests)
</details>
</issue_to_address>
### Comment 15
<location> `test/unit/test_proxy_support.py:177-187` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid loops in tests. ([`no-loop-in-tests`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/no-loop-in-tests))
<details><summary>Explanation</summary>Avoid complex code, like loops, in test functions.
Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
* loops
* conditionals
Some ways to fix this:
* Use parametrized tests to get rid of the loop.
* Move the complex logic into helpers.
* Move the complex part into pytest fixtures.
> Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / [Don't Put Logic in Tests](https://abseil.io/resources/swe-book/html/ch12.html#donapostrophet_put_logic_in_tests)
</details>
</issue_to_address>
### Comment 16
<location> `test/unit/test_proxy_support.py:197-207` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid loops in tests. ([`no-loop-in-tests`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/no-loop-in-tests))
<details><summary>Explanation</summary>Avoid complex code, like loops, in test functions.
Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
* loops
* conditionals
Some ways to fix this:
* Use parametrized tests to get rid of the loop.
* Move the complex logic into helpers.
* Move the complex part into pytest fixtures.
> Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / [Don't Put Logic in Tests](https://abseil.io/resources/swe-book/html/ch12.html#donapostrophet_put_logic_in_tests)
</details>
</issue_to_address>
### Comment 17
<location> `test/unit/test_proxy_support.py:220-230` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid loops in tests. ([`no-loop-in-tests`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/no-loop-in-tests))
<details><summary>Explanation</summary>Avoid complex code, like loops, in test functions.
Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
* loops
* conditionals
Some ways to fix this:
* Use parametrized tests to get rid of the loop.
* Move the complex logic into helpers.
* Move the complex part into pytest fixtures.
> Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / [Don't Put Logic in Tests](https://abseil.io/resources/swe-book/html/ch12.html#donapostrophet_put_logic_in_tests)
</details>
</issue_to_address>
### Comment 18
<location> `test/unit/test_proxy_support.py:243-253` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid loops in tests. ([`no-loop-in-tests`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/no-loop-in-tests))
<details><summary>Explanation</summary>Avoid complex code, like loops, in test functions.
Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
* loops
* conditionals
Some ways to fix this:
* Use parametrized tests to get rid of the loop.
* Move the complex logic into helpers.
* Move the complex part into pytest fixtures.
> Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / [Don't Put Logic in Tests](https://abseil.io/resources/swe-book/html/ch12.html#donapostrophet_put_logic_in_tests)
</details>
</issue_to_address>
### Comment 19
<location> `test/unit/test_proxy_support.py:269-279` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid loops in tests. ([`no-loop-in-tests`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/no-loop-in-tests))
<details><summary>Explanation</summary>Avoid complex code, like loops, in test functions.
Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
* loops
* conditionals
Some ways to fix this:
* Use parametrized tests to get rid of the loop.
* Move the complex logic into helpers.
* Move the complex part into pytest fixtures.
> Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / [Don't Put Logic in Tests](https://abseil.io/resources/swe-book/html/ch12.html#donapostrophet_put_logic_in_tests)
</details>
</issue_to_address>
### Comment 20
<location> `test/unit/test_proxy_support.py:286-296` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid loops in tests. ([`no-loop-in-tests`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/no-loop-in-tests))
<details><summary>Explanation</summary>Avoid complex code, like loops, in test functions.
Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
* loops
* conditionals
Some ways to fix this:
* Use parametrized tests to get rid of the loop.
* Move the complex logic into helpers.
* Move the complex part into pytest fixtures.
> Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / [Don't Put Logic in Tests](https://abseil.io/resources/swe-book/html/ch12.html#donapostrophet_put_logic_in_tests)
</details>
</issue_to_address>
### Comment 21
<location> `test/unit/test_proxy_support.py:311-321` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid loops in tests. ([`no-loop-in-tests`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/no-loop-in-tests))
<details><summary>Explanation</summary>Avoid complex code, like loops, in test functions.
Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
* loops
* conditionals
Some ways to fix this:
* Use parametrized tests to get rid of the loop.
* Move the complex logic into helpers.
* Move the complex part into pytest fixtures.
> Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / [Don't Put Logic in Tests](https://abseil.io/resources/swe-book/html/ch12.html#donapostrophet_put_logic_in_tests)
</details>
</issue_to_address>
### Comment 22
<location> `test/unit/test_proxy_support.py:336-346` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid loops in tests. ([`no-loop-in-tests`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/no-loop-in-tests))
<details><summary>Explanation</summary>Avoid complex code, like loops, in test functions.
Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
* loops
* conditionals
Some ways to fix this:
* Use parametrized tests to get rid of the loop.
* Move the complex logic into helpers.
* Move the complex part into pytest fixtures.
> Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / [Don't Put Logic in Tests](https://abseil.io/resources/swe-book/html/ch12.html#donapostrophet_put_logic_in_tests)
</details>
</issue_to_address>
### Comment 23
<location> `ramalama/proxy_support.py:36-38` </location>
<code_context>
def _get_proxy_env() -> dict[str, str | None]:
"""
Get proxy settings from environment variables.
Returns a dictionary with lowercase keys for all proxy-related env vars.
Checks both lowercase and uppercase variants of each variable.
"""
proxy_vars = ['http_proxy', 'https_proxy', 'all_proxy', 'no_proxy']
proxy_env = {}
for var in proxy_vars:
# Check lowercase first, then uppercase
value = os.environ.get(var) or os.environ.get(var.upper())
if value:
proxy_env[var] = value
logger.debug(f"Found proxy setting: {var}={value}")
return proxy_env
</code_context>
<issue_to_address>
**suggestion (code-quality):** Use named expression to simplify assignment and conditional ([`use-named-expression`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/use-named-expression/))
```suggestion
if value := os.environ.get(var) or os.environ.get(var.upper()):
```
</issue_to_address>
### Comment 24
<location> `ramalama/proxy_support.py:81-83` </location>
<code_context>
def setup_proxy_support() -> None:
"""
Configure urllib to use proxy settings from environment variables.
This function should be called once at application startup to configure
proxy support for all urllib operations.
Supports HTTP, HTTPS, and SOCKS proxies through standard environment variables.
SOCKS proxy support requires the PySocks library to be installed.
"""
proxy_env = _get_proxy_env()
if not proxy_env:
logger.debug("No proxy environment variables found")
return
# Check if any SOCKS proxies are configured
has_socks = any(_is_socks_proxy(url) for url in proxy_env.values() if url)
if has_socks:
try:
import socket # noqa: F401
import socks # noqa: F401
# The import of socks monkey-patches socket, so we need to rebuild the urllib opener
logger.debug("SOCKS proxy support enabled via PySocks")
except ImportError:
logger.warning(
"SOCKS proxy configured but PySocks library not installed. " "Install it with: pip install PySocks"
)
# Fall back to trying without SOCKS support
pass
# Build the proxy handler
proxy_handler = urllib.request.ProxyHandler(proxy_env)
opener = urllib.request.build_opener(proxy_handler)
urllib.request.install_opener(opener)
# Log configured proxies
for key, value in proxy_env.items():
if value and key != 'no_proxy':
logger.info(f"Using proxy: {key}={value}")
elif key == 'no_proxy':
logger.debug(f"Proxy bypass list: {value}")
</code_context>
<issue_to_address>
**suggestion (code-quality):** Remove redundant pass statement ([`remove-redundant-pass`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/remove-redundant-pass/))
```suggestion
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+85
to
+84
| proxy_handler = urllib.request.ProxyHandler(proxy_env) | ||
| opener = urllib.request.build_opener(proxy_handler) |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): ProxyHandler receives all proxy_env keys, including 'no_proxy', which may not be expected.
Remove 'no_proxy' from proxy_env before passing it to ProxyHandler, as urllib expects 'no_proxy' to be set in the environment, not as a handler argument.
Suggested implementation:
# Remove 'no_proxy' from proxy_env before passing to ProxyHandler
proxy_env_for_handler = dict(proxy_env)
proxy_env_for_handler.pop('no_proxy', None)
# Build the proxy handler proxy_handler = urllib.request.ProxyHandler(proxy_env_for_handler)
opener = urllib.request.build_opener(proxy_handler)
urllib.request.install_opener(opener)|
|
||
| with patch('urllib.request.install_opener') as mock_install: | ||
| setup_proxy_support() | ||
| assert mock_install.called |
Contributor
There was a problem hiding this comment.
suggestion (testing): Consider adding an assertion for warning log when PySocks is missing.
Please add an assertion to verify that the expected warning is logged when PySocks is missing, using caplog or a logger mock.
Suggested implementation:
def test_setup_with_socks_proxy_no_pysocks(self, monkeypatch, caplog):
"""Test setup with SOCKS proxy when PySocks is not installed."""
for var in [
'http_proxy',
'HTTP_PROXY',
'https_proxy',
'HTTPS_PROXY',
'all_proxy',
'ALL_PROXY',
'no_proxy',
'NO_PROXY', monkeypatch.setenv('http_proxy', 'socks5://proxy.example.com:1080')
with patch('urllib.request.install_opener') as mock_install:
with caplog.at_level("WARNING"):
setup_proxy_support()
assert not mock_install.called
assert any("PySocks is required for SOCKS proxy support" in record.message for record in caplog.records)
Comment on lines
+306
to
+336
| class TestProxyIntegration: | ||
| """Integration tests for proxy support.""" | ||
|
|
||
| def test_proxy_handler_installed(self, monkeypatch): | ||
| """Test that proxy handler is correctly installed in urllib.""" | ||
| for var in [ | ||
| 'http_proxy', | ||
| 'HTTP_PROXY', | ||
| 'https_proxy', | ||
| 'HTTPS_PROXY', |
Contributor
There was a problem hiding this comment.
suggestion (testing): Missing tests for NO_PROXY behavior and proxy bypass.
Please add tests to ensure requests to NO_PROXY hosts bypass the proxy as expected.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces SOCKS proxy support to the RamaLama application, enhancing its network capabilities. The changes include adding PySocks as a dependency, creating a proxy support module, integrating proxy support into HTTP, chat, and MCP clients, and updating documentation and unit tests. I have identified a few areas where improvements can be made, particularly in error handling and code clarity, and have provided specific suggestions.
Comment on lines
+77
to
+78
| except ImportError: | ||
| logger.warning( | ||
| "SOCKS proxy configured but PySocks library not installed. " "Install it with: pip install PySocks" | ||
| ) |
Contributor
There was a problem hiding this comment.
The warning message could be improved by suggesting the user install PySocks with pip install PySocks[socks5] to ensure that the SOCKS dependencies are installed, as the base install may not include them.
logger.warning(
"SOCKS proxy configured but PySocks library not installed. " "Install it with: pip install PySocks[socks5]"
)
Comment on lines
+213
to
+216
| with patch('urllib.request.install_opener') as mock_install: | ||
| # Should still work but without SOCKS support | ||
| setup_proxy_support() | ||
| assert mock_install.called |
Contributor
There was a problem hiding this comment.
The test description mentions that the test should still work, but it would be helpful to assert that a warning is logged when PySocks is not installed. This will ensure that the fallback mechanism is working as expected.
Suggested change
| with patch('urllib.request.install_opener') as mock_install: | |
| # Should still work but without SOCKS support | |
| setup_proxy_support() | |
| assert mock_install.called | |
| with patch('urllib.request.install_opener') as mock_install: | |
| with patch('ramalama.logger.logger.warning') as mock_warning: | |
| # Should still work but without SOCKS support | |
| setup_proxy_support() | |
| assert mock_install.called | |
| mock_warning.assert_called_once() |
Fixes containers#1740 This commit adds comprehensive SOCKS proxy support to RamaLama: - Added PySocks as a dependency in pyproject.toml - Created ramalama/proxy_support.py module to handle proxy configuration - Integrated proxy support in http_client.py, chat.py, and mcp_client.py - Added comprehensive unit tests in test/unit/test_proxy_support.py - Updated man pages (ramalama.1.md and ramalama-pull.1.md) to document proxy support Proxy support includes: - HTTP_PROXY, HTTPS_PROXY, and ALL_PROXY environment variables - NO_PROXY for bypassing specific hosts - SOCKS4, SOCKS5, and SOCKS5h proxy protocols via PySocks - Automatic detection and configuration at module import time - Graceful fallback when PySocks is not available for SOCKS proxies Generated with the help of Cursor Signed-off-by: Daniel J Walsh <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #1740
This commit adds comprehensive SOCKS proxy support to RamaLama:
Proxy support includes:
Generated with the help of Cursor
Summary by Sourcery
Introduce comprehensive proxy support to RamaLama, enabling HTTP, HTTPS, and SOCKS proxies via standard environment variables with automatic configuration and graceful fallback handling.
New Features:
Build:
Documentation:
Tests: