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

Simplify GitSource proxy settings #682

Closed
imjasonh opened this issue Mar 18, 2021 · 5 comments · Fixed by #683
Closed

Simplify GitSource proxy settings #682

imjasonh opened this issue Mar 18, 2021 · 5 comments · Fixed by #683
Assignees
Labels
kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. priority/awaiting-more-evidence Lowest priority. Possibly useful, but not yet enough support to actually get it done.

Comments

@imjasonh
Copy link
Contributor

The GitSource type has a few fields related to proxying requests:

type GitSource struct {
...
	HTTPProxy string  `json:"httpProxy,omitempty"`
	HTTPSProxy string `json:"httpsProxy,omitempty"`
	// NoProxy can be used to specify domains for which no proxying should be performed. Optional.
	NoProxy string.   `json:"noProxy,omitempty"`
...
}

(ref)

Would it make sense to communicate this information in one field, Proxy string json:"proxy"`?

At the time people are defining the source, they would know whether it's an HTTP or HTTPS repo, and I don't know why they'd want to specify "don't proxy this domain" -- is that the default?

These were added long long ago, alongside a lot of other stuff, so I don't know if they were added with a particular purpose in mind. @sbose78 might know.

With a single Proxy field, we could generate HTTPS_PROXY=${proxy} HTTP_PROXY=${proxy} git clone ${url} if it's set, and just git clone ${url} otherwise, unless there's some semantics to Git proxying I'm not understanding (entirely possible!)

(This isn't a high priority but it might make sense to try to do this before v1beta1 and definitely before v1)

@imjasonh
Copy link
Contributor Author

imjasonh commented Mar 18, 2021

/kind cleanup
/priority awaiting-more-evidence

@openshift-ci-robot openshift-ci-robot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. priority/awaiting-more-evidence Lowest priority. Possibly useful, but not yet enough support to actually get it done. labels Mar 18, 2021
@imjasonh
Copy link
Contributor Author

/kind api-change

@openshift-ci-robot openshift-ci-robot added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Mar 18, 2021
@imjasonh
Copy link
Contributor Author

Upon further review, these fields don't seem to be hooked up to anything, and should be safe to remove. If anybody was setting them before, they weren't doing anything. I'll send a PR to remove them.

/assign

@sbose78
Copy link
Member

sbose78 commented Mar 18, 2021

Yeah they were added long ago with a hope that they would be utilised soon enough. It's been close to a year, we may remove these. And bring them in with an enhancement proposal if needed.

@sbose78 sbose78 closed this as completed Mar 18, 2021
@sbose78 sbose78 reopened this Mar 18, 2021
@sbose78
Copy link
Member

sbose78 commented Mar 18, 2021

I'm good with using a single field later on. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. priority/awaiting-more-evidence Lowest priority. Possibly useful, but not yet enough support to actually get it done.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants