-
Notifications
You must be signed in to change notification settings - Fork 402
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
Bug 1873556: [on-prem] inject the proxy into the env for NetworkManager.service #2266
Conversation
@EmilienM: An error was encountered adding this pull request to the external tracker bugs for bug 1873556 on the Bugzilla server at https://bugzilla.redhat.com:
In response to this:
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. |
Oh heh, that's an embarrassing bug. |
(Unrelated to this PR's code, I love the stack trace from our Github automation trying to talk to Bugzilla which is trying to talk to JIRA...) |
Perhaps it's a good time to start setting the proxy configuration system-wide rather than per script. Not sure if this has been considered previously, but is there any reason not to do it? For our use, it should be enough to drop relevant files to See example at https://coreos.com/os/docs/latest/using-environment-variables-in-systemd-units.html#other-examples |
+1, if maintainers think it's a good idea I would be happy to take a look in this direction. |
Sorry I should have been a bit more specific. IMO making the setting system-wide is the next logical step but should not necessarily be handled in this PR as it would extend the scope quite a bit. Thanks for the bug fix. /lgtm |
/retest Please review the full test history for this PR and help us cut down flakes. |
Yeah, I argued to do system wide but...I forget why we didn't. Hmm. One thing we could probably do is render an Rather than patching our dispatcher script we could also inject the proxy into the env for |
sounds like a plan! |
@EmilienM: This pull request references Bugzilla bug 1873556, which is valid. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
In response to this:
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. |
Did you test this? It looks correct...but it's a larger change. I agree with this comment - let's merge the obviously-correct original code and do this as a separate followup. (But if you've tested it I'm OK to merge as is too) |
I agree it would be safer to proceed with the initial change request and move that refactor into another PR. |
In order to have the proxy variables (HTTP_PROXY, HTTPS_PROXY and NO_PROXY), we need to `export` them otherwise then don't end up being loaded in the environment and it causes issues if a proxy is used, when pulling an image from a registry for example. Signed-off-by: Emilien Macchi <emilien@redhat.com>
@EmilienM: This pull request references Bugzilla bug 1873556, which is valid. 3 validation(s) were run on this bug
In response to this:
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. |
/retest Please review the full test history for this PR and help us cut down flakes. |
19 similar comments
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
@EmilienM: The following tests failed, say
Full PR test history. Your PR dashboard. 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. |
@EmilienM: All pull requests linked via external trackers have merged: Bugzilla bug 1873556 has been moved to the MODIFIED state. In response to this:
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. |
/cherry-pick release-4.6 |
@EmilienM: #2266 failed to apply on top of branch "release-4.6":
In response to this:
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. |
The patch doesn't apply cleanly because we de-duped the on prem platforms templates into the |
In order to have the proxy variables (HTTP_PROXY, HTTPS_PROXY and
NO_PROXY), we need to
export
them otherwise then don't end up beingloaded in the environment and it causes issues if a proxy is used,
when pulling an image from a registry for example.