Skip to content

Commit 28ba55e

Browse files
authored
[bug] plugin env PATH fix (#2418)
## 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.
1 parent ed490a2 commit 28ba55e

File tree

2 files changed

+120
-2
lines changed

2 files changed

+120
-2
lines changed

internal/devconfig/config.go

Lines changed: 20 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -346,9 +346,11 @@ func (c *Config) NixPkgsCommitHash() string {
346346
func (c *Config) Env() map[string]string {
347347
env := map[string]string{}
348348
for _, i := range c.included {
349-
maps.Copy(env, i.Env())
349+
expandedEnvFromPlugin := OSExpandIfPossible(i.Env(), env)
350+
maps.Copy(env, expandedEnvFromPlugin)
350351
}
351-
maps.Copy(env, c.Root.Env)
352+
rootConfigEnv := OSExpandIfPossible(c.Root.Env, env)
353+
maps.Copy(env, rootConfigEnv)
352354
return env
353355
}
354356

@@ -406,3 +408,19 @@ func createIncludableFromPluginConfig(pluginConfig *plugin.Config) *Config {
406408
}
407409
return includable
408410
}
411+
412+
func OSExpandIfPossible(env, existingEnv map[string]string) map[string]string {
413+
mapping := func(value string) string {
414+
// If the value is not set in existingEnv, return the value wrapped in ${...}
415+
if existingEnv == nil || existingEnv[value] == "" {
416+
return fmt.Sprintf("${%s}", value)
417+
}
418+
return existingEnv[value]
419+
}
420+
421+
res := map[string]string{}
422+
for k, v := range env {
423+
res[k] = os.Expand(v, mapping)
424+
}
425+
return res
426+
}

internal/devconfig/config_test.go

Lines changed: 100 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -327,3 +327,103 @@ func TestDefault(t *testing.T) {
327327
t.Errorf("got different JSON after load/save/load:\ninput:\n%s\noutput:\n%s", inBytes, outBytes)
328328
}
329329
}
330+
331+
func TestOSExpandIfPossible(t *testing.T) {
332+
tests := []struct {
333+
name string
334+
env map[string]string
335+
existingEnv map[string]string
336+
want map[string]string
337+
}{
338+
{
339+
name: "basic expansion",
340+
env: map[string]string{
341+
"FOO": "$BAR",
342+
"BAZ": "${QUX}",
343+
},
344+
existingEnv: map[string]string{
345+
"BAR": "bar_value",
346+
"QUX": "qux_value",
347+
},
348+
want: map[string]string{
349+
"FOO": "bar_value",
350+
"BAZ": "qux_value",
351+
},
352+
},
353+
{
354+
name: "missing values remain as template",
355+
env: map[string]string{
356+
"FOO": "$BAR",
357+
"BAZ": "${QUX}",
358+
},
359+
existingEnv: map[string]string{
360+
"BAR": "bar_value",
361+
// QUX is missing
362+
},
363+
want: map[string]string{
364+
"FOO": "bar_value",
365+
"BAZ": "${QUX}",
366+
},
367+
},
368+
{
369+
name: "nil existing env",
370+
env: map[string]string{
371+
"FOO": "$BAR",
372+
"BAZ": "${QUX}",
373+
},
374+
existingEnv: nil,
375+
want: map[string]string{
376+
"FOO": "${BAR}",
377+
"BAZ": "${QUX}",
378+
},
379+
},
380+
{
381+
name: "empty existing env",
382+
env: map[string]string{
383+
"FOO": "$BAR",
384+
},
385+
existingEnv: map[string]string{},
386+
want: map[string]string{
387+
"FOO": "${BAR}",
388+
},
389+
},
390+
{
391+
name: "mixed literal and variable",
392+
env: map[string]string{
393+
"FOO": "prefix_${BAR}_suffix",
394+
},
395+
existingEnv: map[string]string{
396+
"BAR": "bar_value",
397+
},
398+
want: map[string]string{
399+
"FOO": "prefix_bar_value_suffix",
400+
},
401+
},
402+
{
403+
name: "path special case",
404+
env: map[string]string{
405+
"FOO": "/my/config:$FOO",
406+
},
407+
existingEnv: map[string]string{
408+
"FOO": "/my/plugin:$FOO",
409+
},
410+
want: map[string]string{
411+
"FOO": "/my/config:/my/plugin:$FOO",
412+
},
413+
},
414+
}
415+
416+
for _, tt := range tests {
417+
t.Run(tt.name, func(t *testing.T) {
418+
got := OSExpandIfPossible(tt.env, tt.existingEnv)
419+
if len(got) != len(tt.want) {
420+
t.Errorf("OSExpandIfPossible() got %v entries, want %v entries", len(got), len(tt.want))
421+
}
422+
for k, v := range tt.want {
423+
if got[k] != v {
424+
t.Errorf("OSExpandIfPossible() for key %q = %q, want %q", k, got[k], v)
425+
}
426+
}
427+
})
428+
}
429+
}

0 commit comments

Comments
 (0)