[bug] plugin env PATH fix #2418

mohsenari · 2024-11-21T16:31:30Z

Summary

When a plugin modifies PATH and also config in devbox.json modifies PATH via env:{}, the env overwrites the plugin and so PATH modification from the plugin is lost. This is because we do maps.Copy() to merge the env from plugin to the one from config.
The copy is fine for all other env variables but for PATH we need to handle merging the two rather than overwriting.
Fixes #2138

How was it tested?

bug recreate:

devbox init
devbox add ruby (ruby plugin modifies PATH)
devbox shell (see PATH is prepended with ruby plugin)
modify devbox.json and add "env": {"PATH": "/my/config/bin:$PATH"}
re-enter devbox shell and see PATH prepend from ruby plugin is lost, only "/my/config/bin" is prepended.
fix recreate:
follow same steps, but in last step both PATH prepends from devbox.json and from plugin are present.

mikeland73 · 2024-11-21T22:56:16Z

internal/devconfig/config.go

+	rootEnv := mergePATHsFromTwoEnvs(env, c.Root.Env)
+	maps.Copy(env, rootEnv)


Can you use OSExpandEnvMap instead?

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

@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.

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

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?

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) } } }) } }

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

mikeland73 · 2024-11-21T23:00:08Z

internal/devconfig/config.go

+		pluginEnv := mergePATHsFromTwoEnvs(env, i.Env())
+		maps.Copy(env, pluginEnv)


Could you use OSExpandEnvMap instead of mergePATHsFromTwoEnvs?

mikeland73

I don't think we should treat $PATH different from other env vars. Also, a minor nit is that you are not really "merging" paths, you are expanding, but only if $PATH already exists in the new environment you are creating.

I think what you want is something like OSExpandIfPossible (I added a possible implementation).

Also, for stuff like this, adding some unit tests and/or some integration tests in example dir is really useful, since reasoning about what the final environment will look like is hard. See https://github.com/jetify-com/devbox/blob/163ae999a46e6ff4b946437b22a782557e708eeb/examples/plugins/v2-local/devbox.json#L1C1-L36C1 for an example of an integration test for plugins.

mikeland73 · 2024-11-27T19:13:18Z

internal/devconfig/config.go

+	rootEnv := mergePATHsFromTwoEnvs(env, c.Root.Env)
+	maps.Copy(env, rootEnv)


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?

mikeland73 · 2024-11-27T19:13:47Z

internal/devconfig/config.go

+	rootEnv := mergePATHsFromTwoEnvs(env, c.Root.Env)
+	maps.Copy(env, rootEnv)


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) } } }) } }

mikeland73

RC to send back to your queue

mohsenari added 2 commits November 21, 2024 02:37

fix for plugin and env both editing PATH

4686d25

Cleanup and moved logic to a function

165ffec

mohsenari requested a review from mikeland73 November 21, 2024 16:31

mohsenari added 4 commits November 21, 2024 11:31

removed commented code

bffbfc6

removed empty line

9ee0767

ci lint fix

b56a5ff

Fix ruby test error

163ae99

mikeland73 requested changes Nov 21, 2024

View reviewed changes

mohsenari requested a review from mikeland73 November 26, 2024 21:31

mikeland73 reviewed Nov 27, 2024

View reviewed changes

mikeland73 requested changes Nov 27, 2024

View reviewed changes

PR feedback added OSExpandIfPossible and unit test

8c84419

mohsenari requested a review from mikeland73 November 27, 2024 22:23

mikeland73 approved these changes Dec 2, 2024

View reviewed changes

mohsenari merged commit 28ba55e into main Dec 2, 2024
29 checks passed

mohsenari deleted the mohsen--plugin-env-path-fix branch December 2, 2024 23:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[bug] plugin env PATH fix #2418

[bug] plugin env PATH fix #2418

mohsenari commented Nov 21, 2024 •

edited

Loading

mikeland73 Nov 21, 2024

mohsenari Nov 26, 2024 •

edited

Loading

mohsenari Nov 26, 2024

mikeland73 Nov 27, 2024

mikeland73 Nov 27, 2024

mohsenari Nov 27, 2024

mikeland73 Nov 21, 2024

mikeland73 left a comment

mikeland73 Nov 27, 2024

mikeland73 Nov 27, 2024

mikeland73 left a comment

		rootEnv := mergePATHsFromTwoEnvs(env, c.Root.Env)
		maps.Copy(env, rootEnv)

		pluginEnv := mergePATHsFromTwoEnvs(env, i.Env())
		maps.Copy(env, pluginEnv)

[bug] plugin env PATH fix #2418

[bug] plugin env PATH fix #2418

Conversation

mohsenari commented Nov 21, 2024 • edited Loading

Summary

How was it tested?

Choose a reason for hiding this comment

mohsenari Nov 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikeland73 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikeland73 left a comment

Choose a reason for hiding this comment

mohsenari commented Nov 21, 2024 •

edited

Loading

mohsenari Nov 26, 2024 •

edited

Loading