Append custom env vars instead of replace in Process (#168, #169) #171
Conversation
PR #162 refactored the default configuration code. This introduced a subtle bug where `env` became `[]string{}` instead of the default of `nil`. In golang, `exec.Cmd.Env == nil` means to use the "current process's environment". By setting it to `[]string{}` as a default the Process's environment was emptied out which caused an array of strange and difficult to troubleshoot behaviour. See issues #168 and #169 This commit changes the behaviour to append model configured environment variables to the default list rather than replace them.
WalkthroughThe changes update how environment variables are handled when starting a process, ensuring that custom variables are appended to the existing environment rather than replacing it. Logging is adjusted to reflect the new environment. Additionally, a new test verifies correct environment variable handling, and a Windows-specific test configuration is simplified. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as TestProcess_EnvironmentSetCorrectly
participant Proc1 as Process (default env)
participant Proc2 as Process (env + 2 vars)
Test->>Proc1: Start process with default env
Proc1->>Proc1: Inherit default environment
Test->>Proc2: Start process with additional env vars
Proc2->>Proc2: Inherit default environment
Proc2->>Proc2: Append 2 new env vars
Test->>Test: Assert Proc2 env = Proc1 env + 2
sequenceDiagram
participant Process
participant OS
Process->>OS: Get default environment (cmd.Environ())
Process->>Process: Append config.Env to default environment
Process->>Process: Set command environment
Process->>Process: Log command and environment
Process->>OS: Start process
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
proxy/process_test.go (1)
468-490: Good test coverage for the environment variable fix with minor improvements needed.The test effectively validates the core fix by ensuring:
- Environments are not emptied (both processes have non-zero environment variables)
- Custom environment variables are properly appended (count difference verification)
However, there are a few areas for improvement:
Fix the typo on line 475:
- // ensure the additiona variables are appended to the process' environment + // ensure the additional variables are appended to the process' environmentConsider enhancing the test to verify that specific environment variables are present:
+ // Verify that the custom environment variables are actually present + envMap := make(map[string]string) + for _, env := range process2.cmd.Environ() { + parts := strings.SplitN(env, "=", 2) + if len(parts) == 2 { + envMap[parts[0]] = parts[1] + } + } + assert.Equal(t, "1", envMap["TEST_ENV1"]) + assert.Equal(t, "2", envMap["TEST_ENV2"])
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
proxy/process.go(1 hunks)proxy/process_test.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: run-tests
🔇 Additional comments (2)
proxy/process.go (2)
196-196: Excellent fix for the environment variable bug!This change correctly addresses the issue described in the PR objectives. By using
append(p.cmd.Environ(), p.config.Env...)instead of directly assigningp.config.Env, the command now preserves the inherited environment while adding custom variables. This prevents the environment from being cleared whenp.config.Envis an empty slice instead ofnil.
204-204: Enhanced logging improves debugging capabilities.Moving the debug log to after environment setup and including the environment variables in the output will help with troubleshooting environment-related issues. The timing of this log statement is now more accurate as it reflects the final state before command execution.
|
I think the fix won't be complete if we don't change Line 535 in 49035e2 |
|
That's a good catch. I'll update the code (again) |
PR #162 refactored the default configuration code. This introduced a subtle bug where
envbecame[]string{}instead of the default ofnil.In golang,
exec.Cmd.Env == nilmeans to use the "current process's environment". By setting it to[]string{}as a default the Process's environment was emptied out which caused an array of strange and difficult to troubleshoot behaviour. See issues #168 and #169This commit changes the behaviour to append model configured environment variables to the default list rather than replace them.
Summary by CodeRabbit
Bug Fixes
Tests