-
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
Open
daezaa
wants to merge
2
commits into
kubernetes-client:master
Choose a base branch
from
daezaa:master
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+3
−4
Open
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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.
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
Uh oh!
There was an error while loading. Please reload this page.
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
the
elseblock should've ran so that there would be no duplicates ofself.no_proxy = None. However, somehow,if [ -z "$NO_PROXY_LINE" ]; thenhas ran even though there wasself.no_proxy = Noneexisted.Nevertheless, I am revising the PR with the result of executing
instert_proxy_config.shscript on v33.1.0 tag.Please re-review
Uh oh!
There was an error while loading. Please reload this page.
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 script is not handling deleting the
self.no_proxy = Nonewhen it appends the block toself.proxysectionsomething like the following is needed
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.
@roccotigger
From your suggestion.. If
NO_PROXY_LINEis empty, I don't thinksed -i "${NO_PROXY_LINE}a $NOPROXY_BLOCK" "$CONFIG_FILE"is valid and you would need to putself.no_proxy = NoneI honestly don't see a problem with existing script.. If you try this script on v33.1.0, it works fine and the result of it is the pull request I made...
Please let me know if I am mistaken.
TEST DONE
Procedure:
configuration.py(Make sure no redundantself.no_proxy = None)cc : @roycaihw @yliaog
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.
I was unable to run
scripts/update-client.sh. The closest I could come was to use the v33.1.0 version.@daezaa Your analysis is correct that the script will operate correctly without changes.
How did the wrong changes make it into v34.1.0?
How to ensure they are corrected in the next released version?
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.
Well, that is my question too. :(
Tried re-running the script and the file was conserved.