fix: Use HTTP_PROXY and HTTPS_PROXY environment variables for proxy
#2397
Conversation
Use the same proxy configuration as the `http` and `https` protocols.
|
""" WalkthroughThe proxy configuration logic was simplified across documentation, implementation, and tests. Proxy enablement now relies solely on the presence of Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (5)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
|
Report bugs in Issues The following are automatically added:
Available user actions:
PR will be approved when the following conditions are met:
Approvers and Reviewers
Supported /retest check runs
Supported labels
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
README.md(1 hunks)ocp_resources/resource.py(2 hunks)tests/test_resources.py(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
README.md (1)
Learnt from: EdDev
PR: RedHatQE/openshift-python-wrapper#2291
File: ocp_resources/resource.py:126-126
Timestamp: 2025-01-28T07:25:19.573Z
Learning: The environment variable `OPENSHIFT_PYTHON_WRAPPER_CLIENT_USE_PROXY` must be set and not empty to override the `use_proxy` parameter in the `get_client` function.
🧬 Code Graph Analysis (1)
ocp_resources/resource.py (1)
tests/test_resources.py (1)
client(32-33)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: tox
- GitHub Check: python-module-install
- GitHub Check: conventional-title
🔇 Additional comments (8)
ocp_resources/resource.py (3)
104-113: Simplified proxy configuration approachThe code now automatically detects and uses proxy settings from environment variables (
HTTPS_PROXYorHTTP_PROXY) if they're available, instead of requiring an explicit proxy enablement flag. This is a cleaner approach as it follows standard environment variable conventions used by other tools.
114-132: Improved client initialization logicThe client initialization code was refactored to be more consistent and maintainable. Creating the client from either a config dictionary or a config file now follows a clear pattern where the configuration is first created, then used to initialize the client.
134-136: Consolidated error handlingThe error handling was simplified by consolidating the client creation and wrapping in a DynamicClient within a single try block, which makes the code cleaner and easier to maintain.
tests/test_resources.py (4)
4-4: More specific import improvedChanged from importing the entire kubernetes module to importing only the specific exception needed, which is a good practice for reducing import overhead and improving code clarity.
22-28: Improved test fixture organizationSeparated the container configuration retrieval from client creation into two distinct fixtures, improving the test structure by following single responsibility principle. The error message inclusion in the skip reason is also a good addition for better debugging information.
31-33: Simplified client fixtureClient fixture now uses the extracted container configuration from the previous fixture, maintaining a clean separation of concerns and making the test more maintainable.
118-123: Updated test to match new proxy behaviorAdded a new test that verifies the proxy handling behavior when environment variables are set. The test correctly expects a
ConfigExceptionrather than aValueErrorsince the proxy configuration is now auto-detected from environment variables.This test aligns with the new implementation in
get_clientwhich automatically sets proxy from environment variables.README.md (1)
102-109: Documentation updated to match new implementationThe proxy configuration documentation was updated to reflect the simplified approach where users only need to set the
HTTPS_PROXYorHTTP_PROXYenvironment variables without any additional flag.
|
/retest tox |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/test_resources.py (3)
119-124: Consider testing HTTP_PROXY as well.The test confirms that HTTPS_PROXY is correctly used, but according to the implementation in
resource.py, HTTP_PROXY is also supported as a fallback. Consider adding another test case to verify HTTP_PROXY works when HTTPS_PROXY isn't set.def test_client_with_proxy(monkeypatch, k3scontainer_config): proxy = "http://env-proxy.com" monkeypatch.setenv(name="HTTPS_PROXY", value=proxy) client = get_client(config_dict=k3scontainer_config) assert client.configuration.proxy == proxy + + +def test_client_with_http_proxy(monkeypatch, k3scontainer_config): + proxy = "http://env-http-proxy.com" + monkeypatch.setenv(name="HTTP_PROXY", value=proxy) + + client = get_client(config_dict=k3scontainer_config) + assert client.configuration.proxy == proxy
119-124: Verify proxy precedence behavior.The implementation in
resource.pyprioritizes HTTPS_PROXY over HTTP_PROXY. Consider adding a test to verify this precedence when both environment variables are set.+def test_proxy_precedence(monkeypatch, k3scontainer_config): + https_proxy = "https://env-https-proxy.com" + http_proxy = "http://env-http-proxy.com" + monkeypatch.setenv(name="HTTPS_PROXY", value=https_proxy) + monkeypatch.setenv(name="HTTP_PROXY", value=http_proxy) + + client = get_client(config_dict=k3scontainer_config) + # Verify HTTPS_PROXY takes precedence over HTTP_PROXY + assert client.configuration.proxy == https_proxy
119-124:❓ Verification inconclusive
Test that existing proxy configuration is preserved.
According to the implementation in
resource.py, the proxy is only set from environment variables if_client.configuration.proxyis not already set. Consider adding a test to verify this behavior.
🏁 Script executed:
#!/bin/bash # Check if there's a way to set the proxy in the k3scontainer_config # First check if k3scontainer_config is used to initialize the client configuration rg -A 3 "new_client_from_config_dict" --glob "*.py" # Then check how kubernetes client configuration handles proxy settings rg -A 5 "Configuration.*proxy" --glob "*.py"Length of output: 388
I’d like to inspect how the
k3scontainer_configfixture and the proxy‐setting logic inget_clientare implemented. Running a few focused searches:#!/bin/bash # Show how the fixture builds config_dict rg -n -C3 "def k3scontainer_config" --glob "tests/test_resources.py" # Locate get_client and its signature rg -n -C3 "def get_client" --glob "ocp_resources/resource.py" # Show full context around new_client_from_config_dict invocation rg -n -C10 "new_client_from_config_dict" --glob "ocp_resources/resource.py" # Look for any proxy‐related logic in resource.py rg -n -C5 "HTTPS_PROXY" --glob "ocp_resources/resource.py" rg -n -C5 ".proxy" --glob "ocp_resources/resource.py"
To confirm whether
get_clientpreserves an existingconfiguration.proxyand that there isn’t already a test covering it, let’s inspect the implementation and the fixture:#!/bin/bash # Show get_client() definition rg -n -C5 "def get_client" --glob "ocp_resources/resource.py" # Show new_client_from_config_dict call and surrounding proxy logic rg -n -C10 "new_client_from_config_dict" --glob "ocp_resources/resource.py" # Look for any explicit proxy assignments in that file rg -n -C5 "proxy" --glob "ocp_resources/resource.py" # Inspect the test fixture to see if it ever injects proxy into config_dict rg -n -C5 "def k3scontainer_config" --glob "tests/test_resources.py"
Ensure preconfigured proxy isn’t overridden
Based on the logic in ocp_resources/resource.py,
get_clientshould only apply theHTTPS_PROXYenvironment variable whenclient.configuration.proxyis unset. Please add a test that confirms an existing proxy value inconfig_dictisn’t replaced by the environment, for example:def test_client_with_proxy(monkeypatch, k3scontainer_config): proxy = "http://env-proxy.com" monkeypatch.setenv(name="HTTPS_PROXY", value=proxy) client = get_client(config_dict=k3scontainer_config) assert client.configuration.proxy == proxy +def test_client_preserves_existing_proxy(monkeypatch, k3scontainer_config): + existing = "http://custom-proxy.com" + # inject a preconfigured proxy into the config dict + k3scontainer_config["proxy"] = existing + # also set the environment proxy + monkeypatch.setenv(name="HTTPS_PROXY", value="http://env-proxy.com") + + client = get_client(config_dict=k3scontainer_config) + # ensure the preconfigured proxy remains unchanged + assert client.configuration.proxy == existing
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
README.md(1 hunks)tests/test_resources.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- README.md
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/test_resources.py (1)
ocp_resources/resource.py (1)
get_client(79-150)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: tox
- GitHub Check: conventional-title
- GitHub Check: python-module-install
🔇 Additional comments (3)
tests/test_resources.py (3)
1-2: Good practice usingfrom __future__ import annotations.This import enables postponed evaluation of annotations, avoiding potential circular import issues and allowing for more flexible type annotations. This is especially useful for self-referencing types or forward references.
23-30: Good refactoring of the K3S container fixture.Splitting the original client fixture into separate
k3scontainer_configandclientfixtures improves modularity and test isolation. The improved error message in the DockerException catch block provides more context when tests are skipped.
32-34: Good separation of concerns in fixtures.Using the
k3scontainer_configfixture as a dependency for theclientfixture creates a clear separation of concerns and improves test maintainability. Changing the scope to "class" is appropriate for the test structure.
|
/verified |
|
/approve |
Use the same proxy configuration as the
httpandhttpsprotocols.Summary by CodeRabbit
Documentation
HTTPS_PROXYorHTTP_PROXYenvironment variable.Bug Fixes
Tests