-
Notifications
You must be signed in to change notification settings - Fork 248
[plugins] Allow local non-compiled plugins #1224
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.
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
Changes from 9 commits
855c3f8
9fa4242
60effe5
6edd44e
639fac6
0e762a3
9c54926
bec133a
a7bf30a
7e9ba47
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
# Custom plugin example | ||
|
||
Shows how to write custom local plugin. Plugins can: | ||
|
||
* Install packages | ||
* Create templatized files (including flakes) | ||
* Declare services (using process-compose) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
{ | ||
"packages": [], | ||
"shell": { | ||
"init_hook": [ | ||
"echo 'Welcome to devbox!' > /dev/null" | ||
], | ||
"scripts": { | ||
"run_test": [ | ||
"./test.sh" | ||
] | ||
} | ||
}, | ||
"include": [ | ||
"path:my-plugin/my-plugin.json" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. regular plugins are specified as:
https://www.jetpack.io/devbox/docs/guides/plugins/#using-plugins what do you think of an alternative key like:
My concern with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I would also be OK with dropping
@Lagoja thoughts? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ah, that makes sense. I also like that we could later have "remote includes" specified as 🤔 would we ever have non-plugin includes? IIRC there was some talk of having composable If no, then There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The idea would be that including a plugin or another devbox.json is the same (so no different syntax). This would be a reason to avoid using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I see. That makes sense as a goal. How do you anticipate distinguishing the current plugin-jsons versus devbox-json style plugin-jsons? I suppose we can try to Okay, no further concerns on this thread. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @savil the schemas are almost identical. It's a matter of union-ing them (and possibly deprecating anything that doesn't make sense). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't see why regular devbox.json should have There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For maintainability I would prefer to have a single schema, and could imagine good use cases: e.g. you want your devbox.json to create a process compose or config file and don't want to write a separate plugin. That said, it's not something this PR does, I would prefer to cross that bridge when we get to it. |
||
] | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
{ | ||
"lockfile_version": "1", | ||
"packages": { | ||
"path:my-plugin/my-plugin.json": {} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
{ | ||
"name": "my-plugin", | ||
"version": "0.0.1", | ||
"readme": "Example custom plugin", | ||
"env": { | ||
"MY_FOO_VAR": "BAR" | ||
}, | ||
"create_files": { | ||
"{{ .Virtenv }}/empty-dir": "", | ||
"{{ .Virtenv }}/some-file": "some-file.txt", | ||
"{{ .Virtenv }}/process-compose.yaml": "process-compose.yaml" | ||
Comment on lines
+9
to
+11
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm wondering if we can drop the I feel a more natural way for a user to write this is:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To clarify: I fear its easy for the user to make a mistake with the {{ .Virtenv }} format. If we can help it, it would be nice to enable a genuine json which is not also a go-template. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agree that having pure json would be nicer, but there are actually 2 different directories (Virtenv and devbox.d) that plugins write to. It would feel a bit weird to default to Virtenv and then require a template for devbox.d. Are template variables that high a bar? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ohh I see. My concern about the template variables is that its a golang-ism that I fear (fairly strongly) will confuse users who are not familiar with golang (which I'd guess is a fair %, especially if devbox gains more users). If there are strictly two (or a finite set of) options, then how about making that part of the spec. Something like:
Thoughts? In our docs and examples, we can show that Alternate names for the keys could be:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are quite a few template variables and they provide a pretty large amount of customizability so I would prefer to always allow them (even if we allow shortcuts or syntax sugar). re: template variables being a golang-ism: this is true, but they are fairly common. (See helm for example). I think your suggestion of having special keys in Thoughts? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wait, what are the other template-variables? I'm feeling pretty uncomfortable about exposing our internal plugin schema directly for external usage where we'll have to support it indefinitely. Could we write down a spec. for this plugin's schema, so we can evaluate it independent of the implementation in this PR?
Can you elaborate on this: why would using TySON be harder with a schema that has whitelisted keys for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
plugins variables are defined here Created file variables are defined here
Any syntax (template variables or otherwise) we'll have to maintain "forever" (in quotes because nothing is really forever, we can always force people to update). e.g. if you add special create_file fields you are essentially saying you will maintain that forever.
For expediency, my hope was to use our current variable set and document them as experimental/may change until 1.0.0. We obviously do not want them to change, but it seems hard to guarantee. I'm OK with documenting a subset and saying everything else is private and you should not use.
I think you misunderstood me. With TySON (and a simple API for plugins) we no longer need templates or special keys. You could do: import { virtenvPath } from '@devbox/plugins';
export default {
name: "foo",
create_files: {
[virtenvPath() + "/my-file.txt"]: "my-file.txt",
[virtenvPath() + "/other.txt"]: "other.txt"
}
} Since the ts API is published and versioned, we could make somewhat breaking changes without breaking people's plugins. |
||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
version: "0.5" | ||
|
||
processes: | ||
my-plugin-service: | ||
command: echo "success" && tail -f /dev/null | ||
availability: | ||
restart: "always" |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
some data |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
#!/bin/bash | ||
|
||
if [ -z "$MY_FOO_VAR" ]; then | ||
echo "MY_FOO_VAR environment variable is not set." | ||
exit 1 | ||
else | ||
echo "MY_FOO_VAR is set to '$MY_FOO_VAR'" | ||
fi |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,18 +1,81 @@ | ||
package plugin | ||
|
||
import ( | ||
"encoding/json" | ||
"os" | ||
"path/filepath" | ||
"regexp" | ||
"strings" | ||
|
||
"go.jetpack.io/devbox/internal/boxcli/usererr" | ||
"go.jetpack.io/devbox/internal/cuecfg" | ||
"go.jetpack.io/devbox/internal/devpkg" | ||
) | ||
|
||
func (m *Manager) parseInclude(include string) (*devpkg.Package, error) { | ||
type Includable interface { | ||
CanonicalName() string | ||
Hash() string | ||
} | ||
|
||
func (m *Manager) ParseInclude(include string) (Includable, error) { | ||
includeType, name, _ := strings.Cut(include, ":") | ||
if includeType != "plugin" { | ||
return nil, usererr.New("unknown include type %q", includeType) | ||
} else if name == "" { | ||
if name == "" { | ||
return nil, usererr.New("include name is required") | ||
} else if includeType == "plugin" { | ||
return devpkg.PackageFromString(name, m.lockfile), nil | ||
} else if includeType == "path" { | ||
absPath := filepath.Join(m.ProjectDir(), name) | ||
return newLocalPlugin(absPath) | ||
} | ||
return nil, usererr.New("unknown include type %q", includeType) | ||
} | ||
|
||
type localPlugin struct { | ||
name string | ||
path string | ||
} | ||
|
||
var nameRegex = regexp.MustCompile(`^[a-zA-Z0-9_\- ]+$`) | ||
|
||
func newLocalPlugin(path string) (*localPlugin, error) { | ||
content, err := os.ReadFile(path) | ||
if err != nil { | ||
return nil, err | ||
} | ||
m := map[string]any{} | ||
if err := json.Unmarshal(content, &m); err != nil { | ||
return nil, err | ||
} | ||
name, ok := m["name"].(string) | ||
if !ok || name == "" { | ||
return nil, | ||
usererr.New("plugin %s is missing a required field 'name'", path) | ||
} | ||
return devpkg.PackageFromString(name, m.lockfile), nil | ||
if !nameRegex.MatchString(name) { | ||
return nil, usererr.New( | ||
"plugin %s has an invalid name %q. Name must match %s", | ||
path, name, nameRegex, | ||
) | ||
} | ||
return &localPlugin{ | ||
name: name, | ||
path: path, | ||
}, nil | ||
} | ||
|
||
func (l *localPlugin) CanonicalName() string { | ||
return l.name | ||
} | ||
|
||
func (l *localPlugin) IsLocal() bool { | ||
return true | ||
} | ||
|
||
func (l *localPlugin) contentPath(subpath string) string { | ||
return filepath.Join(filepath.Dir(l.path), subpath) | ||
} | ||
|
||
func (l *localPlugin) Hash() string { | ||
h, _ := cuecfg.FileHash(l.path) | ||
return h | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes! thank you for adding this
run_test