-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Fix no_proxy reset in configuration.py #2459
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
This commit removes the redundant assignment to `None`, ensuring that the `no_proxy` environment variable is preserved and proxy bypass settings are applied as expected.
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: daezaa The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Welcome @daezaa! |
lgtm 👍 |
if os.getenv("no_proxy"): self.no_proxy = os.getenv("no_proxy") | ||
"""Proxy URL | ||
""" | ||
self.no_proxy = None |
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.
The change LGTM
This file is generated by openapi-generator. We also need to update the hotfix script to keep the patch: https://github.com/kubernetes-client/python/blob/master/scripts/insert_proxy_config.sh
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.
@roycaihw Thanks for the input. The script looks fine to me and no need changes but if it was executed on v33.1.0
,
python/scripts/insert_proxy_config.sh
Lines 49 to 76 in 5f6cdb2
if [ -z "$NO_PROXY_LINE" ]; then | |
# self.no_proxy = None is not present → insert full block after self.proxy = None | |
BLOCK+="${INDENT}# Load proxy from environment variables (if set)\n" | |
BLOCK+="${INDENT}if os.getenv(\"HTTPS_PROXY\"): self.proxy = os.getenv(\"HTTPS_PROXY\")\n" | |
BLOCK+="${INDENT}if os.getenv(\"https_proxy\"): self.proxy = os.getenv(\"https_proxy\")\n" | |
BLOCK+="${INDENT}if os.getenv(\"HTTP_PROXY\"): self.proxy = os.getenv(\"HTTP_PROXY\")\n" | |
BLOCK+="${INDENT}if os.getenv(\"http_proxy\"): self.proxy = os.getenv(\"http_proxy\")\n" | |
BLOCK+="${INDENT}self.no_proxy = None\n" | |
BLOCK+="${INDENT}# Load no_proxy from environment variables (if set)\n" | |
BLOCK+="${INDENT}if os.getenv(\"NO_PROXY\"): self.no_proxy = os.getenv(\"NO_PROXY\")\n" | |
BLOCK+="${INDENT}if os.getenv(\"no_proxy\"): self.no_proxy = os.getenv(\"no_proxy\")" | |
sed -i "${PROXY_LINE}a $BLOCK" "$CONFIG_FILE" | |
echo "Inserted full proxy + no_proxy block after 'self.proxy = None'." | |
else | |
# self.no_proxy = None exists → insert only env logic after that | |
BLOCK+="${INDENT}# Load proxy from environment variables (if set)\n" | |
BLOCK+="${INDENT}if os.getenv(\"HTTPS_PROXY\"): self.proxy = os.getenv(\"HTTPS_PROXY\")\n" | |
BLOCK+="${INDENT}if os.getenv(\"https_proxy\"): self.proxy = os.getenv(\"https_proxy\")\n" | |
BLOCK+="${INDENT}if os.getenv(\"HTTP_PROXY\"): self.proxy = os.getenv(\"HTTP_PROXY\")\n" | |
BLOCK+="${INDENT}if os.getenv(\"http_proxy\"): self.proxy = os.getenv(\"http_proxy\")\n" | |
BLOCK+="${INDENT}# Load no_proxy from environment variables (if set)\n" | |
BLOCK+="${INDENT}if os.getenv(\"NO_PROXY\"): self.no_proxy = os.getenv(\"NO_PROXY\")\n" | |
BLOCK+="${INDENT}if os.getenv(\"no_proxy\"): self.no_proxy = os.getenv(\"no_proxy\")" | |
sed -i "${NO_PROXY_LINE}a $BLOCK" "$CONFIG_FILE" | |
echo "Inserted environment block after 'self.no_proxy = None'." | |
fi |
the
else
block should've ran so that there would be no duplicates of self.no_proxy = None
. However, somehow, if [ -z "$NO_PROXY_LINE" ]; then
has ran even though there was self.no_proxy = None
existed.
Nevertheless, I am revising the PR with the result of executing instert_proxy_config.sh
script on v33.1.0 tag.
Please re-review
What type of PR is this?
/kind bug
What this PR does / why we need it:
Currently,
self.no_proxy
is explicitly reset toNone
after being populated from environment variables (NO_PROXY
orno_proxy
). This effectively discards the loaded values and preventsno_proxy
from being respected when configuring the client.This commit removes the redundant assignment to
None
, ensuring that theno_proxy
environment variable is preserved and proxy bypass settings are applied as expected.Which issue(s) this PR fixes:
No issue has been opened yet. Let me know if needed.
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: