-
Notifications
You must be signed in to change notification settings - Fork 523
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
Proxy support #309
Proxy support #309
Conversation
|
||
class YumMixin(BasePackageMixin): | ||
|
||
def Startup(self): | ||
"""Eliminates the need to have a tty to run sudo commands.""" | ||
if FLAGS.http_proxy or FLAGS.https_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.
or FLAGS.ftp_proxy
FYI, review comments are intended as suggestions - we don't want the review process to turn into a barrier to contributions. Feel free to push back on requests and/or ask us to take care of issues. |
OK, i think now it looks good |
LGTM, thanks for making the changes. @cmccoy , any comments from your side? |
test this please |
OK, i was added documentation now. In subject of test, i have that environment in my work then proxy works good, do you think about unit tests? |
LGTM. There are going to be some merge conflicts due to #321. Let us know if we can help. |
Working on the merge now - seeing some lint errors from flake8, I'll take care of them. |
I've merged this manually to the dev branch, but this didn't work right since it erroneously applied changes to the deleted |
The dev branch is now up to date. I had combined your patches into the single change d0ebc16 with no modifications, then followed up with the two fixup changes - in case you run into merge conflicts, you could try cherry picking those separately on top of your original changes. Also, can you please verify that things work right for you? I had slightly changed the logic as part of the move to take better advantage of inheritance, it should be equivalent but I could only do limited testing. For reference:
|
Added possibility to configure proxy on VM when setup must stay behind the proxy server.