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.

Sign up for GitHub

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

✨ Add proxy option #136

Merged
merged 4 commits into from
Jan 11, 2024
Merged

✨ Add proxy option #136

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
00b4b19
Add proxy option
aufi Jan 8, 2024
a89bdc4
Add no-proxy
aufi Jan 8, 2024
15b1570
Add env vars defaults for http&https proxy
aufi Jan 8, 2024
3bf01f2
Use proxy env vars case insensitive
aufi Jan 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 8 additions & 6 deletions cmd/analyze.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,8 @@ type analyzeCommand struct {
rules []string
jaegerEndpoint string
enableDefaultRulesets bool
proxy string
httpProxy string
httpsProxy string
noProxy string

// tempDirs list of temporary dirs created, used for cleanup
Expand Down Expand Up @@ -162,8 +163,9 @@ func NewAnalyzeCmd(log logr.Logger) *cobra.Command {
analyzeCommand.Flags().BoolVar(&analyzeCmd.overwrite, "overwrite", false, "overwrite output directory")
analyzeCommand.Flags().StringVar(&analyzeCmd.jaegerEndpoint, "jaeger-endpoint", "", "jaeger endpoint to collect traces")
analyzeCommand.Flags().BoolVar(&analyzeCmd.enableDefaultRulesets, "enable-default-rulesets", true, "run default rulesets with analysis")
analyzeCommand.Flags().StringVar(&analyzeCmd.proxy, "proxy", "", "HTTP&HTTPS proxy string URL")
analyzeCommand.Flags().StringVar(&analyzeCmd.noProxy, "no-proxy", "", "proxy excluded URLs (relevant only with `proxy`)")
analyzeCommand.Flags().StringVar(&analyzeCmd.httpProxy, "http-proxy", os.Getenv("http_proxy"), "HTTP proxy string URL")
Copy link
Contributor

Choose a reason for hiding this comment

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

one small concern, would be that the default value printed out here, would be changed based on env var.

Also I think you will need to handle both http_proxy and HTTP_PROXY.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for comment! Printing actual default values in help didn't look bad to me.
What I've been looking on proxy env variables, lower case http_proxy is much more common than HTTP_PROXY. However, please update me if mentioned issues needs to be addressed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Pretty sure it does; Docker, I think, only uses uppercase, and I think Openshift does as well.

Copy link
Member Author

@aufi aufi Jan 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

👍 will update!

analyzeCommand.Flags().StringVar(&analyzeCmd.httpsProxy, "https-proxy", os.Getenv("https_proxy"), "HTTPS proxy string URL")
analyzeCommand.Flags().StringVar(&analyzeCmd.noProxy, "no-proxy", os.Getenv("no_proxy"), "proxy excluded URLs (relevant only with proxy)")

return analyzeCommand
}
Expand Down Expand Up @@ -464,10 +466,10 @@ func (a *analyzeCommand) getConfigVolumes() (map[string]string, error) {
}

// Set proxy to providers
if a.proxy != "" {
if a.httpProxy != "" || a.httpsProxy != "" {
proxy := provider.Proxy{
HTTPProxy: a.proxy,
HTTPSProxy: a.proxy,
HTTPProxy: a.httpProxy,
HTTPSProxy: a.httpsProxy,
NoProxy: a.noProxy,
}
for i := range provConfig {
Expand Down