fix(getter): pass settings environment variables #31613
Conversation
pull-request-size
bot
added
the
size/S
aslafy-z
force-pushed
the
feat/getter-env
branch
from
c959335 to
4762040
Compare
pull-request-size
bot
added
size/M
benoittgt
left a comment
benoittgt
left a comment
There was a problem hiding this comment.
Thanks for the clear issue. Maybe we could add tests.
Signed-off-by: Zadkiel AHARONIAN <[email protected]>
Signed-off-by: Zadkiel AHARONIAN <[email protected]>
aslafy-z
force-pushed
the
feat/getter-env
branch
from
4762040 to
8534663
Compare
| require.True(t, ok, "expected getter to be a *getterPlugin") | ||
|
|
||
| require.NotEmpty(t, gp.env, "expected env to be set on getterPlugin") | ||
| envMap := plugin.ParseEnv(gp.env) |
There was a problem hiding this comment.
ParseEnv is exposed just for this test. It is an internal package though, so not as big a deal.
There was a problem hiding this comment.
If you want me to duplicate the logic instead, I can. I thought it would not be an issue to publish it as it is an internal package.
|
I just tested on macOS, and it works as expected with the patch. I used the reproduction script from the issue: #31612 (comment) LGTM |
|
Hi maintainers, This PR fixes the downloader plugin env regression introduced by #31145 (missing HELM_* env vars), which is currently blocking some downloader plugins (example + repro in #31612). Do you have an ETA for merge, and (if accepted) is this something you would consider backporting to the next 4.0.x patch release? The v4.0.4 release notes say 4.0.5 is planned for Jan 14, 2026 (and 4.1.0 for Jan 21, 2026). If a backport is needed, I'm happy to help prepare it. @benoittgt would you agree to mention some other maintainer? Thanks! |
There was a problem hiding this comment.
Pull request overview
This PR fixes getter plugins to properly receive Helm settings environment variables and prevents a nil pointer dereference in EnvSettings.Namespace(). The changes ensure that environment variables like HELM_DEBUG, HELM_PLUGINS, and others are correctly propagated to getter plugins during execution.
Key Changes:
- Environment variables from CLI settings are now passed to getter plugins via the
plugin.Input.Envfield - Added nil check for
s.configinEnvSettings.Namespace()to prevent panic when method is called on uninitialized instances - Exported
parseEnvandformatEnvfunctions toParseEnvandFormatEnvfor external package usage
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pkg/getter/plugingetter.go | Captures environment variables from settings and passes them to getter plugins through the getterPlugin struct |
| pkg/getter/plugingetter_test.go | Adds test to verify environment variables are correctly passed to getter plugins and accessible |
| pkg/cli/environment.go | Adds nil check for config field to prevent panic in Namespace() when called via EnvVars() on uninitialized instances |
| internal/plugin/runtime.go | Exports parseEnv and formatEnv functions to ParseEnv and FormatEnv with added documentation |
| internal/plugin/runtime_test.go | Updates test calls to use exported ParseEnv and FormatEnv function names |
| internal/plugin/runtime_subprocess.go | Updates all call sites to use exported ParseEnv and FormatEnv functions |
| internal/plugin/runtime_subprocess_getter.go | Updates call sites to use exported ParseEnv and FormatEnv functions |
| internal/plugin/runtime_extismv1.go | Updates call site to use exported ParseEnv function |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if s.config != nil { | ||
| if ns, _, err := s.config.ToRawKubeConfigLoader().Namespace(); err == nil { | ||
| return ns | ||
| } |
There was a problem hiding this comment.
The nil check for s.config prevents a panic when EnvVars() is called on an uninitialized EnvSettings instance. Consider adding a test case that verifies this behavior, such as calling (&EnvSettings{}).EnvVars() or (&EnvSettings{}).Namespace() to ensure the nil check works correctly and doesn't panic.
|
/cc @banjoh @gjenkins8 @joejulian @mattfarina @robertsirc @sabre1041 @scottrigby @technosophos @TerryHowe Can you please give it a look? |
|
You can find us on slack you know. |
|
Thank you @TerryHowe @banjoh |
It should |
scottrigby
added
bug
6 participants
What this PR does / why we need it:
This PR restores Helm settings environment variables for getter plugins.
Commits:
settings.EnvVars()incollectGetterPlugins()to pass Helm environment to pluginsEnvSettings.Namespace()- Pre-existing bug triggered whenEnvVars()is called on uninitializedEnvSettings(e.g.,&cli.EnvSettings{}instead ofcli.New())Fixes #31612
Closes #31611
Special notes for your reviewer:
The nil check fix addresses a pre-existing bug in
main- callingEnvVars()on&cli.EnvSettings{}panics becauses.configis nil. Existing tests using&cli.EnvSettings{}didn't trigger this because they never entered a code path that calledEnvVars().If applicable:
docs neededlabel should be applied if so)