-
Notifications
You must be signed in to change notification settings - Fork 6
doc & example: update: setting up proxy #39
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
base: master
Are you sure you want to change the base?
Conversation
WalkthroughThe README adds a "Using a proxy" section and converts several bash code fences to tildes. The example client now reads proxy and CA bundle environment variables, configures Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Env as Environment
participant Example as examples/client.py
participant Config as Configuration
participant Api as ApiClient
participant Service as Remote Service
User->>Example: run example
Example->>Env: read HTTPS_PROXY / HTTP_PROXY
Env-->>Example: proxy URL (optional)
Example->>Config: set proxy if present
Example->>Env: read CA_PATH / REQUESTS_CA_BUNDLE
Env-->>Example: CA path (optional)
Example->>Config: set ssl_ca_cert if present
Example->>Env: export REQUESTS_CA_BUNDLE if unset
Example->>Api: instantiate with Config
Example->>Api: set_default_header("Authorization", API_KEY)
Api->>Service: request (uses proxy / CA / header)
Service-->>Api: response
Api-->>Example: result
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Nitpick comments (1)
examples/client.py (1)
48-57
: Consider logging for better observability, Number One.The CA bundle configuration logic is sound. However, silently catching all exceptions may mask unexpected errors during development or troubleshooting. As a starship's first officer would advise, visibility into anomalies strengthens our defensive posture.
Consider adding a debug log when the attribute doesn't exist:
# set CA for both requests and the elabapi-client ca_path = os.getenv("CA_PATH") or os.getenv("REQUESTS_CA_BUNDLE") if ca_path: # let requests (and other libs) use this CA bundle unless already set os.environ.setdefault("REQUESTS_CA_BUNDLE", ca_path) try: configuration.ssl_ca_cert = ca_path - except Exception: + except AttributeError: # attribute may not exist on all generated configurations pass + except Exception as e: + # unexpected error, log for debugging + if configuration.debug: + print(f"Warning: Could not set ssl_ca_cert: {e}")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
README.md
(1 hunks)examples/client.py
(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
README.md
64-64: Code fence style
Expected: tilde; Actual: backtick
(MD048, code-fence-style)
72-72: Code fence style
Expected: tilde; Actual: backtick
(MD048, code-fence-style)
83-83: Code fence style
Expected: tilde; Actual: backtick
(MD048, code-fence-style)
🪛 Ruff (0.13.3)
examples/client.py
43-45: try
-except
-pass
detected, consider logging the exception
(S110)
43-43: Do not catch blind exception: Exception
(BLE001)
55-57: try
-except
-pass
detected, consider logging the exception
(S110)
55-55: Do not catch blind exception: Exception
(BLE001)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please adress comments from 🐰
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
examples/client.py (2)
47-56
: Engage similar refinements for CA bundle handling.The CA certificate configuration follows the same pattern as the proxy configuration. The same optional refinements apply here: consider catching
AttributeError
specifically and adding debug logging if desired.Apply this diff for more specific exception handling:
try: configuration.ssl_ca_cert = ca_path - except Exception: + except AttributeError: # attribute may not exist on all generated configurations - pass + if configuration.debug: + print("Note: This client version does not support the 'ssl_ca_cert' attribute")
29-45
: Make it so: align documentation with implementation
- Remove
NO_PROXY
from the proxy usage comments (examples/client.py:33) or implement its handling.- Optional: catch
AttributeError
instead of broadException
and add debug logging ifconfiguration.debug
is enabled.# export HTTPS_PROXY="http://127.0.0.1:8080" -# export NO_PROXY=localhost,127.0.0.1 # export REQUESTS_CA_BUNDLE=/path/to/mitmproxy-ca.pem
try: configuration.proxy = proxy_url - except Exception: + except AttributeError: # some generated clients don't expose 'proxy', so ignore safely + if getattr(configuration, "debug", False): + print("Note: This client version does not support the 'proxy' attribute")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
README.md
(3 hunks)examples/client.py
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- README.md
🧰 Additional context used
🪛 Ruff (0.13.3)
examples/client.py
43-45: try
-except
-pass
detected, consider logging the exception
(S110)
43-43: Do not catch blind exception: Exception
(BLE001)
54-56: try
-except
-pass
detected, consider logging the exception
(S110)
54-54: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (1)
examples/client.py (1)
58-62
: Excellent work, ensign!The Authorization header configuration is properly placed after the ApiClient instantiation, and the approach using
set_default_header
is sound. This ensures the API key is consistently included in all requests made through this client.
|
||
## Using a proxy | ||
|
||
The simplest way to route API traffic through a proxy is to set standard environment variables before running your script. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now the readme is missing the part about the configuration.proxy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
plus verbose sur le texte et moins verbose sur le code
Documentation on how to configure the elabapi_client using a proxy, with the example of
mitmproxy
Summary by CodeRabbit
New Features
Documentation
Notes