Skip to content

Commit 6a8b5dc

Browse files
mikeland73mikeland73
authored
[plugins] Fix relative plugins, add cycle detection (#1877)
## Summary TSIA ## How was it tested? Added more tests via examples. Tested cycle manually. (including `./..` in plugin1a) <img width="640" alt="image" src="https://github.com/jetpack-io/devbox/assets/544948/8e4fcc61-c763-4ad9-a6f1-8a44e81b1520">
1 parent 226d9e8 commit 6a8b5dc

File tree

13 files changed

+247
-43
lines changed

13 files changed

+247
-43
lines changed

‎examples/plugins/v2-local/devbox.json‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,11 +10,20 @@
1010
],
1111
"scripts": {
1212
"run_test": [
13+
// This tests init hook and env included from plugin1
1314
"test -n \"$PLUGIN1_INIT_HOOK\" || exit 1",
1415
"test -n \"$PLUGIN1_ENV\" || exit 1",
16+
// This tests init hook and env included from plugin1a (via plugin1, with relative path)
17+
"test -n \"$PLUGIN1A_INIT_HOOK\" || exit 1",
18+
"test -n \"$PLUGIN1A_ENV\" || exit 1",
19+
// Test env override
1520
"if [ \"$PLUGIN1_ENV2\" != \"override\" ]; then exit 1; fi;",
21+
// test included scripts
1622
"devbox run plugin_1_script",
17-
"hello"
23+
"devbox run plugin_1A_script",
24+
// Test packages included recursively
25+
"hello",
26+
"cowsay 'Hello, world!'"
1827
]
1928
}
2029
},

‎examples/plugins/v2-local/devbox.lock‎

Lines changed: 76 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,74 @@
11
{
22
"lockfile_version": "1",
33
"packages": {
4+
"cowsay@latest": {
5+
"last_modified": "2024-02-24T23:06:34Z",
6+
"resolved": "github:NixOS/nixpkgs/9a9dae8f6319600fa9aebde37f340975cab4b8c0#cowsay",
7+
"source": "devbox-search",
8+
"version": "3.7.0",
9+
"systems": {
10+
"aarch64-darwin": {
11+
"outputs": [
12+
{
13+
"name": "out",
14+
"path": "/nix/store/jbd4ibcidgicsvfrw7485j7dvgxxaaw2-cowsay-3.7.0",
15+
"default": true
16+
},
17+
{
18+
"name": "man",
19+
"path": "/nix/store/kr3azp5dzznk5ra7a09m2j3kqrmn7n5p-cowsay-3.7.0-man",
20+
"default": true
21+
}
22+
],
23+
"store_path": "/nix/store/jbd4ibcidgicsvfrw7485j7dvgxxaaw2-cowsay-3.7.0"
24+
},
25+
"aarch64-linux": {
26+
"outputs": [
27+
{
28+
"name": "out",
29+
"path": "/nix/store/snpdm0421rfd2m0r45hmrfrsnv8i1bhs-cowsay-3.7.0",
30+
"default": true
31+
},
32+
{
33+
"name": "man",
34+
"path": "/nix/store/fnagjxw72d9mff2jjw5lz7l8nvlnwg46-cowsay-3.7.0-man",
35+
"default": true
36+
}
37+
],
38+
"store_path": "/nix/store/snpdm0421rfd2m0r45hmrfrsnv8i1bhs-cowsay-3.7.0"
39+
},
40+
"x86_64-darwin": {
41+
"outputs": [
42+
{
43+
"name": "out",
44+
"path": "/nix/store/pp6rki1vdvbafli7ghx44dghj6aw9za4-cowsay-3.7.0",
45+
"default": true
46+
},
47+
{
48+
"name": "man",
49+
"path": "/nix/store/z199j4f78l0kpqb3d94whradvbr449vi-cowsay-3.7.0-man",
50+
"default": true
51+
}
52+
],
53+
"store_path": "/nix/store/pp6rki1vdvbafli7ghx44dghj6aw9za4-cowsay-3.7.0"
54+
},
55+
"x86_64-linux": {
56+
"outputs": [
57+
{
58+
"name": "out",
59+
"path": "/nix/store/1gm1jhr61dl5bfr9mff7pk3dl44352zl-cowsay-3.7.0",
60+
"default": true
61+
},
62+
{
63+
"name": "man",
64+
"path": "/nix/store/rd4l8kpfc3w1km2z5qv8z3qxg06csy70-cowsay-3.7.0-man",
65+
"default": true
66+
}
67+
],
68+
"store_path": "/nix/store/1gm1jhr61dl5bfr9mff7pk3dl44352zl-cowsay-3.7.0"
69+
}
70+
}
71+
},
472
"hello@latest": {
573
"last_modified": "2024-02-24T23:06:34Z",
674
"resolved": "github:NixOS/nixpkgs/9a9dae8f6319600fa9aebde37f340975cab4b8c0#hello",
@@ -14,7 +82,8 @@
1482
"path": "/nix/store/0dqk50ap0jsh0wz4y6n6kqfl8ynhz84f-hello-2.12.1",
1583
"default": true
1684
}
17-
]
85+
],
86+
"store_path": "/nix/store/0dqk50ap0jsh0wz4y6n6kqfl8ynhz84f-hello-2.12.1"
1887
},
1988
"aarch64-linux": {
2089
"outputs": [
@@ -23,7 +92,8 @@
2392
"path": "/nix/store/218n5svnni1y5nm0iq0nir3jpz8yxbsb-hello-2.12.1",
2493
"default": true
2594
}
26-
]
95+
],
96+
"store_path": "/nix/store/218n5svnni1y5nm0iq0nir3jpz8yxbsb-hello-2.12.1"
2797
},
2898
"x86_64-darwin": {
2999
"outputs": [
@@ -32,7 +102,8 @@
32102
"path": "/nix/store/v1mbicfpyp9mnps4i8iccfzas53if4p6-hello-2.12.1",
33103
"default": true
34104
}
35-
]
105+
],
106+
"store_path": "/nix/store/v1mbicfpyp9mnps4i8iccfzas53if4p6-hello-2.12.1"
36107
},
37108
"x86_64-linux": {
38109
"outputs": [
@@ -41,7 +112,8 @@
41112
"path": "/nix/store/ki1s92r0524hj416rh20v0msxmi4pjvn-hello-2.12.1",
42113
"default": true
43114
}
44-
]
115+
],
116+
"store_path": "/nix/store/ki1s92r0524hj416rh20v0msxmi4pjvn-hello-2.12.1"
45117
}
46118
}
47119
}

‎examples/plugins/v2-local/plugin1/plugin.json‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,5 +18,8 @@
1818
"create_files": {
1919
"{{ .DevboxDir }}/foo.txt": "foo.txt",
2020
"{{ .Virtenv }}/process-compose.yaml": "process-compose.yaml"
21-
}
21+
},
22+
"include": [
23+
"./plugin1a"
24+
]
2225
}

‎examples/plugins/v2-local/plugin1/plugin1a/plugin.json‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
{
2+
"name": "plugin1a",
3+
"packages": ["cowsay@latest"],
4+
"env": {
5+
"PLUGIN1A_ENV": "success"
6+
},
7+
"shell": {
8+
"init_hook": [
9+
"export PLUGIN1A_INIT_HOOK=success"
10+
],
11+
"scripts": {
12+
"plugin_1A_script": [
13+
"echo success"
14+
]
15+
}
16+
},
17+
"include": [
18+
"../../plugin2"
19+
]
20+
}

‎internal/devconfig/config.go‎

Lines changed: 47 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import (
77
"maps"
88
"net/http"
99
"os"
10+
"path/filepath"
1011

1112
"github.com/pkg/errors"
1213
"github.com/samber/lo"
@@ -71,7 +72,12 @@ func readFromFile(path string) (*Config, error) {
7172
if err != nil {
7273
return nil, err
7374
}
74-
return loadBytes(b)
75+
config, err := loadBytes(b)
76+
if err != nil {
77+
return nil, err
78+
}
79+
config.Root.AbsRootPath = path
80+
return config, nil
7581
}
7682

7783
func LoadConfigFromURL(ctx context.Context, url string) (*Config, error) {
@@ -104,19 +110,40 @@ func loadBytes(b []byte) (*Config, error) {
104110
}
105111

106112
func (c *Config) LoadRecursive(lockfile *lock.File) error {
113+
return c.loadRecursive(lockfile, map[string]bool{}, "" /*cyclePath*/)
114+
}
115+
116+
// loadRecursive loads all the included plugins and their included plugins, etc.
117+
// seen should be a cloned map because loading plugins twice is allowed if they
118+
// are in different paths.
119+
func (c *Config) loadRecursive(
120+
lockfile *lock.File,
121+
seen map[string]bool,
122+
cyclePath string,
123+
) error {
107124
included := make([]*Config, 0, len(c.Root.Include))
108125

109126
for _, includeRef := range c.Root.Include {
110-
pluginConfig, err := plugin.LoadConfigFromInclude(includeRef, lockfile)
127+
pluginConfig, err := plugin.LoadConfigFromInclude(
128+
includeRef, lockfile, filepath.Dir(c.Root.AbsRootPath))
111129
if err != nil {
112130
return errors.WithStack(err)
113131
}
114132

115-
includable := &Config{
116-
Root: pluginConfig.ConfigFile,
117-
pluginData: &pluginConfig.PluginOnlyData,
133+
newCyclePath := fmt.Sprintf("%s -> %s", cyclePath, includeRef)
134+
if seen[pluginConfig.Source.Hash()] {
135+
// Note that duplicate includes are allowed if they are in different paths
136+
// e.g. 2 different plugins can include the same plugin.
137+
// We do not allow a single plugin to include duplicates.
138+
return errors.Errorf(
139+
"circular or duplicate include detected:\n%s", newCyclePath)
118140
}
119-
if err := includable.LoadRecursive(lockfile); err != nil {
141+
seen[pluginConfig.Source.Hash()] = true
142+
143+
includable := createIncludableFromPluginConfig(pluginConfig)
144+
145+
if err := includable.loadRecursive(
146+
lockfile, maps.Clone(seen), newCyclePath); err != nil {
120147
return errors.WithStack(err)
121148
}
122149

@@ -136,7 +163,9 @@ func (c *Config) LoadRecursive(lockfile *lock.File) error {
136163
Root: builtIn.ConfigFile,
137164
pluginData: &builtIn.PluginOnlyData,
138165
}
139-
if err := includable.LoadRecursive(lockfile); err != nil {
166+
newCyclePath := fmt.Sprintf("%s -> %s", cyclePath, builtIn.Source.LockfileKey())
167+
if err := includable.loadRecursive(
168+
lockfile, maps.Clone(seen), newCyclePath); err != nil {
140169
return errors.WithStack(err)
141170
}
142171
included = append(included, includable)
@@ -259,3 +288,14 @@ func (c *Config) IsEnvsecEnabled() bool {
259288
}
260289
return c.Root.IsEnvsecEnabled()
261290
}
291+
292+
func createIncludableFromPluginConfig(pluginConfig *plugin.Config) *Config {
293+
includable := &Config{
294+
Root: pluginConfig.ConfigFile,
295+
pluginData: &pluginConfig.PluginOnlyData,
296+
}
297+
if localPlugin, ok := pluginConfig.Source.(*plugin.LocalPlugin); ok {
298+
includable.Root.AbsRootPath = localPlugin.Path()
299+
}
300+
return includable
301+
}

‎internal/devconfig/config_test.go‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,12 @@ func TestDefault(t *testing.T) {
2525
if err != nil {
2626
t.Fatal("got load error:", err)
2727
}
28-
if diff := cmp.Diff(cfg, out, cmpopts.IgnoreUnexported(configfile.ConfigFile{}, configfile.PackagesMutator{}, Config{})); diff != "" {
28+
if diff := cmp.Diff(
29+
cfg,
30+
out,
31+
cmpopts.IgnoreUnexported(configfile.ConfigFile{}, configfile.PackagesMutator{}, Config{}),
32+
cmpopts.IgnoreFields(configfile.ConfigFile{}, "AbsRootPath"),
33+
); diff != "" {
2934
t.Errorf("configs not equal (-in +out):\n%s", diff)
3035
}
3136

‎internal/devconfig/configfile/file.go‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,10 @@ const (
2525

2626
// ConfigFile defines a devbox environment as JSON.
2727
type ConfigFile struct {
28+
// AbsRootPath is the absolute path to the devbox.json or plugin.json file
29+
// it will not be set for github plugins.
30+
AbsRootPath string `json:"-"`
31+
2832
Name string `json:"name,omitempty"`
2933
Description string `json:"description,omitempty"`
3034

‎internal/plugin/files.go‎

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -14,24 +14,24 @@ import (
1414
"go.jetpack.io/devbox/plugins"
1515
)
1616

17-
func getConfigIfAny(pkg Includable, projectDir string) (*Config, error) {
18-
switch pkg := pkg.(type) {
17+
func getConfigIfAny(inc Includable, projectDir string) (*Config, error) {
18+
switch includable := inc.(type) {
1919
case *devpkg.Package:
20-
return getBuiltinPluginConfigIfExists(pkg, projectDir)
20+
return getBuiltinPluginConfigIfExists(includable, projectDir)
2121
case *githubPlugin:
22-
content, err := pkg.Fetch()
22+
content, err := includable.Fetch()
2323
if err != nil {
2424
return nil, errors.WithStack(err)
2525
}
26-
return buildConfig(pkg, projectDir, string(content))
27-
case *localPlugin:
28-
content, err := os.ReadFile(pkg.Path())
26+
return buildConfig(includable, projectDir, string(content))
27+
case *LocalPlugin:
28+
content, err := os.ReadFile(includable.Path())
2929
if err != nil && !os.IsNotExist(err) {
3030
return nil, errors.WithStack(err)
3131
}
32-
return buildConfig(pkg, projectDir, string(content))
32+
return buildConfig(includable, projectDir, string(content))
3333
}
34-
return nil, errors.Errorf("unknown plugin type %T", pkg)
34+
return nil, errors.Errorf("unknown plugin type %T", inc)
3535
}
3636

3737
func getBuiltinPluginConfigIfExists(

‎internal/plugin/github.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ func (p *githubPlugin) CanonicalName() string {
2424
}
2525

2626
func (p *githubPlugin) Hash() string {
27-
h, _ := cachehash.Bytes([]byte(p.CanonicalName()))
27+
h, _ := cachehash.Bytes([]byte(p.ref.String()))
2828
return h
2929
}
3030

‎internal/plugin/includable.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,14 +13,14 @@ type Includable interface {
1313
LockfileKey() string
1414
}
1515

16-
func parseIncludable(includableRef, projectDir string) (Includable, error) {
16+
func parseIncludable(includableRef, workingDir string) (Includable, error) {
1717
ref, err := flake.ParseRef(includableRef)
1818
if err != nil {
1919
return nil, err
2020
}
2121
switch ref.Type {
2222
case flake.TypePath:
23-
return newLocalPlugin(ref, projectDir)
23+
return newLocalPlugin(ref, workingDir)
2424
case flake.TypeGitHub:
2525
return &githubPlugin{ref: ref}, nil
2626
default:

0 commit comments

Comments
 (0)