Fix XDG_RUNTIME_DIR setting in shellexec
#2019
Conversation
WalkthroughThe changes update how environment variables are merged within a function. Instead of using two separate loops to copy key-value pairs from two different sets of environment variables, the new implementation uses direct copy operations via a dedicated copy function. Additionally, a conditional check is now performed to determine if a specific runtime directory variable is missing or empty. If so, the variable is assigned a default value based on the current user's identifier. No modifications were made to any exported or public entities. ✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
CodeRabbit Configuration File (
|
esimkowitz
changed the title
XDG_RUNTIME_DIR setting in shellexec
![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)
pkg/shellexec/shellexec.go (1)
616-624: Consider updating the function documentation.The function's documentation should be updated to reflect the new behavior of setting a default value for
XDG_RUNTIME_DIRwhen it's not present or empty./* tryGetPamEnvVars tries to get the environment variables from /etc/environment, /etc/security/pam_env.conf, and ~/.pam_environment. It then returns a map of the environment variables, overriding duplicates with the following order of precedence: 1. /etc/environment 2. /etc/security/pam_env.conf 3. ~/.pam_environment + +If XDG_RUNTIME_DIR is not set or is empty, it defaults to /run/user/$UID +according to the XDG Base Directory Specification. */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/shellexec/shellexec.go(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: Build for TestDriver.ai
🔇 Additional comments (3)
pkg/shellexec/shellexec.go (3)
21-21: LGTM: Import maps package for efficient map operations.The addition of the
mapspackage import is appropriate for using theCopyfunction to merge maps efficiently.
638-639: LGTM: Use maps.Copy for efficient map merging.The change from using loops to
maps.Copyis a good improvement as it:
- Uses a built-in function designed specifically for map operations
- Makes the code more concise and readable
- Potentially offers better performance
640-642: LGTM: Set default XDG_RUNTIME_DIR according to specification.The implementation correctly follows the XDG Base Directory Specification by:
- Checking if
XDG_RUNTIME_DIRis unset or empty- Setting the default value to
/run/user/$UID- Using
os.Getuid()to get the current user's ID
XDG_RUNTIME_DIR needs to be set. If it is not set by a user-defined config, it should default to
/run/user/$UID