Go: Explicitly check whether proxy env vars are empty #19598
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refines proxy environment variable handling to explicitly ignore empty string values, ensuring that unintentionally set empty vars are treated as unset.
- Added
&& var != ""checks to skip empty proxy env vars for host, port, CA certificate, and URLs. - Maintains existing formatting of proxy address and logging only when both host and port are non-empty.
| if proxy_host, proxy_host_set := os.LookupEnv(PROXY_HOST); proxy_host_set && proxy_host != "" { | ||
| if proxy_port, proxy_port_set := os.LookupEnv(PROXY_PORT); proxy_port_set && proxy_port != "" { |
There was a problem hiding this comment.
[nitpick] The pattern of checking for both existence and non-empty value is repeated across multiple env vars. Consider creating a helper such as lookupNonEmptyEnv(key string) (string, bool) to encapsulate this logic and reduce duplication.
| if proxy_host, proxy_host_set := os.LookupEnv(PROXY_HOST); proxy_host_set && proxy_host != "" { | |
| if proxy_port, proxy_port_set := os.LookupEnv(PROXY_PORT); proxy_port_set && proxy_port != "" { | |
| if proxy_host, _ := lookupNonEmptyEnv(PROXY_HOST); proxy_host != "" { | |
| if proxy_port, _ := lookupNonEmptyEnv(PROXY_PORT); proxy_port != "" { |
Copilot uses AI. Check for mistakes.
| @@ -82,7 +82,7 @@ func getEnvVars() []string { | |||
| } | |||
| } | |||
|
|
|||
| if proxy_urls, proxy_urls_set := os.LookupEnv(PROXY_URLS); proxy_urls_set { | |||
| if proxy_urls, proxy_urls_set := os.LookupEnv(PROXY_URLS); proxy_urls_set && proxy_urls != "" { | |||
There was a problem hiding this comment.
Values containing only whitespace will pass the != "" check and may cause parsing errors. Consider using strings.TrimSpace(proxy_urls) before testing for emptiness to treat whitespace-only inputs as empty.
| if proxy_urls, proxy_urls_set := os.LookupEnv(PROXY_URLS); proxy_urls_set && proxy_urls != "" { | |
| if proxy_urls, proxy_urls_set := os.LookupEnv(PROXY_URLS); proxy_urls_set && strings.TrimSpace(proxy_urls) != "" { |
Copilot uses AI. Check for mistakes.
This should resolve github/codeql-action#2909 after we didn't catch this in #19248. If the environment variables are set, but contain the empty string, then we treat those empty strings as meaningful values, even though they are not.