Refactor environment variable expansion in PrepareCommands and update tests

[yxxhero]

What this PR does / why we need it:
fix helm plugin cmd args env issue

Special notes for your reviewer:

If applicable:

• this PR contains user facing changes (the docs needed label should be applied if so)
• this PR contains unit tests
• this PR has been tested for backwards compatibility

[Copilot]

Pull Request Overview

This PR fixes a bug in environment variable expansion where the PrepareCommands function was ignoring custom environment variables passed via the env parameter.

• Replaced os.ExpandEnv() with os.Expand() using a custom mapping function to properly expand variables from the env parameter
• Updated tests to use parseEnv(os.Environ()) instead of empty maps to better reflect real-world usage
• Added test coverage for expanding both system and custom environment variables

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
internal/plugin/subprocess_commands.go	Fixed environment variable expansion to use the env parameter instead of only system environment variables
internal/plugin/subprocess_commands_test.go	Updated tests to use parseEnv(os.Environ()) and added test case for mixed system and custom environment variable expansion

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[yxxhero]

@gjenkins8

[yxxhero]

@scottrigby could you take an review? thanks so much.

[gjenkins8]

minor: Can we just move this logic to be included below?:

env["TESTX"] = "testx"

(as the usage of parseEnv is just causing this to round-trip through the process env vars)

[yxxhero]

got it.

[gjenkins8]

similar to above:

Suggested change

	env := parseEnv(os.Environ())
	env := map[string]string {
	"TEST": "test",
	}

[gjenkins8]

(and remove the usage of t.Setenv("TEST", "test"))

[sabre1041]

LGTM

[robertsirc]

LGTM

[robertsirc]

This is LGTM but @gjenkins8 had some comments giving him a chance to check on this before merging out of respect.

R

[scottrigby]

Nice work fixing the env map variable expansion! However, I'd suggest keeping test coverage for both OS-level and env map variables rather than replacing one with the other.

The current test changes remove t.Setenv() calls entirely in favor of only testing the env map. Ideally, TestPrepareCommandsExpand() should verify that expansion works correctly for:

1. OS-level environment variables (via t.Setenv)
2. Variables passed in the env parameter
3. Precedence when the same variable exists in both

I'm thinking we should keep one t.Setenv() call and add env map variables. Eg:

func TestPrepareCommandsExpand(t *testing.T) {
    t.Setenv("OS_VAR", "os_value")  // Test OS-level
    
    env := map[string]string{
        "MAP_VAR": "map_value",     // Test env map
        "OS_VAR":  "override_value", // Test precedence (may also be good to test here)
    }
    
    cmdArgs := []string{"-c", "echo \"${OS_VAR} ${MAP_VAR}\""}
    // ... rest of test
}

This would ensure the expansion feature is thoroughly tested for scenarios where plugins encounter variables from both sources.

Basically, removing t.Setenv reduces coverage, we should add it back in addition to what you've added here. Checking for precedence is a nice to have, but if you don't mind ensuring this test covers that too in this same PR I think our bases will be covered.

TestPrepareCommandsNoExpand() should also ensure envs don't expand from either of the two sources (wouldn't need to test precedence).

[gjenkins8]

Nice work fixing the env map variable expansion! However, I'd suggest keeping test coverage for both OS-level and env map variables rather than replacing one with the other.

The current test changes remove t.Setenv() calls entirely in favor of only testing the env map. Ideally, TestPrepareCommandsExpand() should verify that expansion works correctly for:

1. OS-level environment variables (via t.Setenv)
2. Variables passed in the env parameter
3. Precedence when the same variable exists in both

I'm thinking we should keep one t.Setenv() call and add env map variables. Eg:

func TestPrepareCommandsExpand(t *testing.T) {
    t.Setenv("OS_VAR", "os_value")  // Test OS-level
    
    env := map[string]string{
        "MAP_VAR": "map_value",     // Test env map
        "OS_VAR":  "override_value", // Test precedence (may also be good to test here)
    }
    
    cmdArgs := []string{"-c", "echo \"${OS_VAR} ${MAP_VAR}\""}
    // ... rest of test
}

This would ensure the expansion feature is thoroughly tested for scenarios where plugins encounter variables from both sources.

Basically, removing t.Setenv reduces coverage, we should add it back in addition to what you've added here. Checking for precedence is a nice to have, but if you don't mind ensuring this test covers that too in this same PR I think our bases will be covered.

TestPrepareCommandsNoExpand() should also ensure envs don't expand from either of the two sources (wouldn't need to test precedence).

I don't think this is necessary, PrepareCommands doesn't access the process environment. It expects the full environment to be passed (which is much better from a dependency management/testing perspective IMHO)

[scottrigby]

@gjenkins8 you're right. @yxxhero never mind, my suggestion for coverage should be added elsewhere - eg, at the CLI level (ultimately testing the functionality of plugin.SubprocessPluginRuntime.runCLI() not in TestPrepareCommandsExpand()). Your test updates here are good for this function as-is 👍