WIP fix: better env loading [IDE-1314] #407
Conversation
Previosuly we were not using the PATH from the user's shell (or Bash), now we will use both. Plus, refactored to split the functions for shell environment loading and config file loading. Plus, allowlisted the Homebrew version of Bash.
If JAVA_HOME and/or GOROOT are set in the project env files then we will preppend `${value}/bin` to the PATH.
Previously it was being left unset.
🎉 Snyk checks have passed. No issues have been found so far.✅ security/snyk check is complete. No issues have been found. (View Details) ✅ license/snyk check is complete. No issues have been found. (View Details) ✅ code/snyk check is complete. No issues have been found. (View Details) |
🎉 Snyk checks have passed. No issues have been found so far.✅ security/snyk check is complete. No issues have been found. (View Details) ✅ license/snyk check is complete. No issues have been found. (View Details) ✅ code/snyk check is complete. No issues have been found. (View Details) |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
pkg/envvars/environment.go
Outdated
| bashOutput := getEnvFromShell("bash") | ||
|
|
||
| // Prepend the Bash PATH now, as it is low priority | ||
| // We use the Bash shell env as well, since the more info scraping the better to help OSS scans |
There was a problem hiding this comment.
Maybe change the comment to read "We first read the Bash shell env to use as a fallback...." or similar?
pkg/envvars/environment.go
Outdated
|
|
||
| // this is applied at the end always, as it does not overwrite existing variables | ||
| // We use the Bash shell env as well, since the more info scraping the better to help OSS scans | ||
| defer func() { _ = gotenv.Apply(strings.NewReader(bashOutput)) }() //nolint:errcheck // we can't do anything with the error |
There was a problem hiding this comment.
Why are we doing this?
There was a problem hiding this comment.
The more potential binary paths we can add to the PATH the better, as its more fallback for OSS scans to look for things like java and mvn.
Or were you wanting me to update the comment?
There was a problem hiding this comment.
I changed the comment, is it better?
Plus, refactor for clarity.
pkg/envvars/environment.go
Outdated
| if val, ok := bashEnv[PathEnvVarName]; ok { | ||
| UpdatePath(val, false) | ||
| } |
There was a problem hiding this comment.
nitpick: this could be moved into the deferred function
pkg/envvars/environment.go
Outdated
| sdkVarNames := []string{"JAVA_HOME", "GOROOT"} | ||
| sdkValues := make(map[string]string) | ||
| for _, sdkVar := range sdkVarNames { | ||
| sdkValues[sdkVar] = os.Getenv(sdkVar) | ||
| // Unset the env var so we can capture if the config file sets it. | ||
| _ = os.Unsetenv(sdkVar) | ||
| } |
There was a problem hiding this comment.
There's a few more, but we can cover that later. E.g. PYTHONPATH, GRADLE_HOME, MVN_HOME etc.
There was a problem hiding this comment.
I don't like that we unset them. This can affect other threads/goroutines in process space.
The preference is that the envvars funcs should just return maps of read environments instead of actually setting the environment variables in the process. Made the maps utils exported.
Useful for calling environments
|
I'm going to draft this and put it on hold as it is getting out of hand and we are too close to release to be dealing with it. Instead I have opened #411 for just the bare minimum of changes we need for now. |
rrama
changed the title
3 participants
Previously we were not using the PATH from the user's shell (or Bash), now we will use both.
Plus, refactored to split the functions for shell environment loading and config file loading.
Plus, allowlisted the Homebrew version of Bash.
If
JAVA_HOMEand/orGOROOTare set in the project env files then we will prepend${value}/binto the PATH.Note, to be merged along with snyk/snyk-ls#969