Skip to content

[bug] plugin env PATH fix #2418

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Dec 2, 2024
Merged

[bug] plugin env PATH fix #2418

Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
4686d25
fix for plugin and env both editing PATH
mohsenari Nov 21, 2024
165ffec
Cleanup and moved logic to a function
mohsenari Nov 21, 2024
bffbfc6
removed commented code
mohsenari Nov 21, 2024
9ee0767
removed empty line
mohsenari Nov 21, 2024
b56a5ff
ci lint fix
mohsenari Nov 21, 2024
163ae99
Fix ruby test error
mohsenari Nov 21, 2024
8c84419
PR feedback added OSExpandIfPossible and unit test
mohsenari Nov 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 27 additions & 2 deletions internal/devconfig/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"net/http"
"os"
"path/filepath"
"strings"
"syscall"
"time"

Expand Down Expand Up @@ -346,9 +347,18 @@ func (c *Config) NixPkgsCommitHash() string {
func (c *Config) Env() map[string]string {
env := map[string]string{}
for _, i := range c.included {
maps.Copy(env, i.Env())
// Here, in new pluginPath we replace the keyword "$PATH"
// with what we have for PATH so far. If so far we have /plugin1/bin:$PATH
// and new plugin adds /plugin2/bin/:$PATH the result becomes
// /plugin2/bin/:/plugin1/bin/:$PATH
pluginEnv := mergePATHsFromTwoEnvs(env, i.Env())
maps.Copy(env, pluginEnv)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you use OSExpandEnvMap instead of mergePATHsFromTwoEnvs?

}
maps.Copy(env, c.Root.Env)
// Here we merge the processed PATH from all plugins to PATH in config env.
// So if config env has /config/env/:$PATH merging with the plugin PATH
// it becomes /config/env/:/plugin2/bin/:/plugin1/bin/:$PATH
rootEnv := mergePATHsFromTwoEnvs(env, c.Root.Env)
maps.Copy(env, rootEnv)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you use OSExpandEnvMap instead?

Basically take the environment provided by the plugins and use it to expand the environment of the config.

Copy link
Collaborator Author

@mohsenari mohsenari Nov 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mikeland73 after your suggestion, I tried OSExpandEnvMap and it it doesn't work.

The issue with OSExpandEnvMap is it merges the two PATHs correctly but it applies the (non-nix) system PATH to the :$PATH at the end. So with OSExpandEnvMap PATH becomes: /my/config:/my/plugin:/opt/homebrew/sbin:/usr/local/bin:...

With mergePATHsFromTwoEnvs PATH becomes: /my/config:/my/plugin:/Users/mohsenansari/code/go.jetpack.io/devbox/examples/stacks/rails/.devbox/nix/profile/default/bin:...:/opt/homebrew/sbin:/usr/local/bin:...

So OSExpandEnvMap can't work here because I don't want to expand $PATH env yet, I just want to merge them correctly without losing :$PATH suffix or $PATH: prefix to it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Essentially, OSExpandEnvMap expands PATH with non-devboxified PATH value, but I don't want that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would something like

func OSExpandIfPossible(env, existingEnv map[string]string) map[string]string {
	mapping := func(value string) string {
		// If the value is not set in existingEnv, return the value wrapped in ${...}
		if existingEnv == nil || existingEnv[value] == "" {
			return fmt.Sprintf("${%s}", value)
		}
		return existingEnv[value]
	}

	res := map[string]string{}
	for k, v := range env {
		res[k] = os.Expand(v, mapping)
	}
	return res
}

work?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And some unit tests if you decide to go this route:

package conf

import "testing"

func TestOSExpandIfPossible(t *testing.T) {
	tests := []struct {
		name        string
		env         map[string]string
		existingEnv map[string]string
		want        map[string]string
	}{
		{
			name: "basic expansion",
			env: map[string]string{
				"FOO": "$BAR",
				"BAZ": "${QUX}",
			},
			existingEnv: map[string]string{
				"BAR": "bar_value",
				"QUX": "qux_value",
			},
			want: map[string]string{
				"FOO": "bar_value",
				"BAZ": "qux_value",
			},
		},
		{
			name: "missing values remain as template",
			env: map[string]string{
				"FOO": "$BAR",
				"BAZ": "${QUX}",
			},
			existingEnv: map[string]string{
				"BAR": "bar_value",
				// QUX is missing
			},
			want: map[string]string{
				"FOO": "bar_value",
				"BAZ": "${QUX}",
			},
		},
		{
			name: "nil existing env",
			env: map[string]string{
				"FOO": "$BAR",
				"BAZ": "${QUX}",
			},
			existingEnv: nil,
			want: map[string]string{
				"FOO": "${BAR}",
				"BAZ": "${QUX}",
			},
		},
		{
			name: "empty existing env",
			env: map[string]string{
				"FOO": "$BAR",
			},
			existingEnv: map[string]string{},
			want: map[string]string{
				"FOO": "${BAR}",
			},
		},
		{
			name: "mixed literal and variable",
			env: map[string]string{
				"FOO": "prefix_${BAR}_suffix",
			},
			existingEnv: map[string]string{
				"BAR": "bar_value",
			},
			want: map[string]string{
				"FOO": "prefix_bar_value_suffix",
			},
		},
	}

	for _, tt := range tests {
		t.Run(tt.name, func(t *testing.T) {
			got := OSExpandIfPossible(tt.env, tt.existingEnv)
			if len(got) != len(tt.want) {
				t.Errorf("OSExpandIfPossible() got %v entries, want %v entries", len(got), len(tt.want))
			}
			for k, v := range tt.want {
				if got[k] != v {
					t.Errorf("OSExpandIfPossible() for key %q = %q, want %q", k, got[k], v)
				}
			}
		})
	}
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OSExpandIfPossible works. I included this suggestion along with the unit test and added a case for path to the unit test as well.

return env
}

Expand Down Expand Up @@ -406,3 +416,18 @@ func createIncludableFromPluginConfig(pluginConfig *plugin.Config) *Config {
}
return includable
}

// Instead of overwriting PATH modifications we need to merge them
// so merging of /plugin1/bin/:$PATH and /plugin2/bin/:$PATH
// and /config/env/bin/:$PATH should result in
//
// /config/env/bin/:/plugin2/bin/:/plugin1/bin/:$PATH
//
// because config env should take priority over plugins
func mergePATHsFromTwoEnvs(currentEnv, newEnv map[string]string) map[string]string {
if currentEnv["PATH"] != "" && newEnv["PATH"] != "" {
slog.Debug("A Plugin or Config wants to modify PATH. Processing the merge", "AddedPATH", newEnv["PATH"])
newEnv["PATH"] = strings.Replace(newEnv["PATH"], "$PATH", currentEnv["PATH"], 1)
}
return newEnv
}