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 1925534: Add proxy to oc #751
Bug 1925534: Add proxy to oc #751
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: zerodayz 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 |
1fb35ff
to
477b7a8
Compare
@zerodayz: This pull request references Bugzilla bug 1925534, which is invalid:
Comment 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. |
/bugzilla refresh |
@zerodayz: This pull request references Bugzilla bug 1925534, which is valid. The bug has been moved to the POST state. 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. |
0a48771
to
d31ea19
Compare
/retest |
4 similar comments
/retest |
/retest |
/retest |
/retest |
pkg/cli/login/login.go
Outdated
@@ -83,6 +83,9 @@ func NewCmdLogin(f kcmdutil.Factory, streams genericclioptions.IOStreams) *cobra | |||
cmds.Flags().StringVarP(&o.Username, "username", "u", o.Username, "Username for server") | |||
cmds.Flags().StringVarP(&o.Password, "password", "p", o.Password, "Password for server") | |||
|
|||
// Proxy is the proxy to be used for the cluster requests | |||
cmds.Flags().StringVar(&o.Proxy, "proxy", o.Proxy, "Proxy for the cluster") |
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'm very cautious about adding new flags, so I'd prefer we don't fix that bit. https://github.com/kubernetes/client-go/blob/release-1.21/tools/clientcmd/api/types.go#L85-L94 explicitly states that http_proxy
or https_proxy
environment variables are being used if it's empty.
For config modifications we always have oc config set
command.
Although, it does make sense to copy proxyURL once it's set when invoking oc project
. @zerodayz can you change your PR such that it only does the last bit mentioned? We did a similar improvement in #692
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.
Hi @soltysh is this what you have meant? Please check. Thank you.
1c35d3b
to
5eed4f0
Compare
5eed4f0
to
e9d4dc1
Compare
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.
/lgtm
/approve
/retest
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: soltysh, zerodayz The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/retest Please review the full test history for this PR and help us cut down flakes. |
2 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. |
/test e2e-aws-upgrade |
/retest Please review the full test history for this PR and help us cut down flakes. |
11 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. |
@zerodayz: All pull requests linked via external trackers have merged: Bugzilla bug 1925534 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. |
We can use existing Proxy field.
Without patch:
With patch:
With Login option
--proxy
addedoc
will use that during login and will save the proxy as proxy-url for any subsequent requests against that cluster.