Skip to content
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

added configuration 'http_proxy' to allow the usage of a proxy #206

Closed

Conversation

don41382
Copy link

@don41382 don41382 commented May 1, 2017

No description provided.

@k8s-ci-robot
Copy link
Contributor

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.


Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label May 1, 2017
@codecov-io
Copy link

codecov-io commented May 1, 2017

Codecov Report

Merging #206 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #206   +/-   ##
=======================================
  Coverage   94.56%   94.56%           
=======================================
  Files           9        9           
  Lines         681      681           
=======================================
  Hits          644      644           
  Misses         37       37

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1517bca...eb374c4. Read the comment docs.

@mbohlool
Copy link
Contributor

mbohlool commented May 1, 2017

LGTM. is it possible to add a test for this?

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels May 1, 2017
@mbohlool mbohlool self-requested a review May 1, 2017 18:06
@mbohlool mbohlool self-assigned this May 1, 2017
@mbohlool
Copy link
Contributor

mbohlool commented May 9, 2017

@don41382 thanks for the PR. If you don't have time to figure out the test, let me know and I can look into adding a test if possible.

@don41382
Copy link
Author

don41382 commented May 9, 2017

@mbohlool sorry for the delayed response. I actually not sure how to test this. I am not a big python programmer. If you could show me how I would appreciate this.

@mbohlool
Copy link
Contributor

mbohlool commented May 9, 2017

You can use some mock libraries but that I am not sure what is the best practice in python too. We can merge this without test. Thanks.

@@ -89,6 +89,9 @@ def __init__(self):
# Set this to True/False to enable/disable SSL hostname verification.
self.assert_hostname = None

# http proxy setting
self.http_proxy = None
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor: Can you rename this to http_proxy_url to self-document the type?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@mbohlool
Copy link
Contributor

mbohlool commented May 9, 2017

I just added a minor comment. Will merge after you address it. please stash new changes in the existing commit. Thanks.

@mbohlool
Copy link
Contributor

mbohlool commented May 11, 2017

LGTM. Please git rebase upstream/master and stash two commits together (use git rebase -i HEAD~2).

@mbohlool
Copy link
Contributor

mbohlool commented May 12, 2017

@don41382 Sorry. things are moving around in the client because of a required restructuring of the client. You need to send this change to https://github.com/kubernetes-client/python-base now.

@mbohlool
Copy link
Contributor

as it was my fault moving things around while you were waiting for this to merge, this is an script to do the pull request automatically, just set your github username and make sure you have hub tool installed:

# make sure you have hub tool installed from here https://hub.github.com/
# run this in a temp folder

export GITHUB_USER=YOUR_GITHUB_USER

git clone https://github.com/kubernetes-client/python-base
cd python-base
hub fork
curl -o patch.diff https://patch-diff.githubusercontent.com/raw/kubernetes-incubator/client-python/pull/206.patch
sed -i'' "s.kubernetes/client/..g" patch.diff
git apply patch.diff
git commit -m "added configuration 'http_proxy' to allow the usage of a proxy"
git push $GITHUB_USER master
hub pull-request

@mbohlool
Copy link
Contributor

mbohlool commented Jun 9, 2017

closing in favor of kubernetes-client/python-base#8

@mbohlool mbohlool closed this Jun 9, 2017
yliaog pushed a commit to yliaog/client-python that referenced this pull request Jan 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants