Conversation
Ompragash
left a comment
There was a problem hiding this comment.
Suggestions
- Trailing whitespace on line 609
There's a trailing tab/whitespace on the blank line between the secondos.Getenvand theifcheck. This is a minor style issue but can cause linting failures:
value = os.Getenv(strings.ToUpper(key))
// <-- trailing whitespace here
if len(value) > 0 {- No unit tests
There are no existing tests forgetProxyValue, and this PR doesn't add any. Given that this function now has three-level fallback logic, unit tests would be valuable to verify:
- Returns value from lowercase env var when set
- Returns value from uppercase env var when lowercase is not set
- Returns value from
HARNESS_-prefixed var when neither standard var is set - Returns empty string when none are set
- Confirms precedence (standard vars win over
HARNESS_vars)
-
Only uppercase
HARNESS_prefix is checked — is that intentional?
The function currently checksHARNESS_HTTP_PROXY(uppercase) but notHARNESS_http_proxy(lowercase). The standard proxy lookup checks both lower and upper case, but the Harness fallback only checks uppercase. This is likely fine since Harness injects these in uppercase, but it's worth documenting that assumption explicitly. -
The
addProxyValuefunction sets build args with the original key name, notHARNESS_-prefixed
When a value is retrieved fromHARNESS_HTTP_PROXY, it gets injected into Docker build args ashttp_proxy=<value>andHTTP_PROXY=<value>(the standard names). This is correct behavior — the HARNESS prefix is only for sourcing, not for what gets passed to the build. Just confirming this is intentional and well understood. -
Comment could be updated
The existing function comment says "assumes that the upper and lower case versions of are the same" — this is slightly misleading now that there's a third source. Consider updating the doc comment to mention the Harness-prefixed fallback:
// helper function to get a proxy value from the environment.
//
// Checks for the key in the following order:
// 1. Lowercase key (e.g., http_proxy)
// 2. Uppercase key (e.g., HTTP_PROXY)
// 3. HARNESS_-prefixed uppercase key (e.g., HARNESS_HTTP_PROXY)
func getProxyValue(key string) string {- Consider logging when the HARNESS fallback is used
For debuggability, it might be useful to log a message when the value is sourced from aHARNESS_-prefixed variable, so users understand where the proxy setting came from during builds. Something like:
harnessValue := os.Getenv("HARNESS_" + strings.ToUpper(key))
if len(harnessValue) > 0 {
fmt.Printf("Using HARNESS_%s as proxy value for %s\n", strings.ToUpper(key), key)
}
return harnessValueOverall: The core logic is correct and minimal. I'd recommend adding unit tests for the getProxyValue function, updating the doc comment, and optionally adding a log line when the Harness fallback is triggered before merging.
Description for the changes added here:
Tested by making image and pushing it into personal registry, and then adding the image as a plugin step in the pipeline.

so by default in Cloud infra for a build and push step
Before:
After:

So the HARNESS_* setting are automatically inserted into the environment.