Skip to content

[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

Merged
merged 10 commits into from
Jul 6, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion devbox.lock
Original file line number Diff line number Diff line change
Expand Up @@ -17,4 +17,4 @@
"version": "1.52.2"
}
}
}
}
7 changes: 7 additions & 0 deletions examples/plugins/local/README.md
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)
16 changes: 16 additions & 0 deletions examples/plugins/local/devbox.json
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"
Copy link
Collaborator

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

]
}
},
"include": [
"path:my-plugin/my-plugin.json"
Copy link
Collaborator

Choose a reason for hiding this comment

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

regular plugins are specified as:

> {
  "includes": [
    "plugin:nginx"
  ]
}

https://www.jetpack.io/devbox/docs/guides/plugins/#using-plugins

what do you think of an alternative key like:

  • plugin-path:
  • local-plugin:
    or..even just plugin: and if it ends in a .json then we know its a local plugin.

My concern with path is that its a bit ambiguous in terms of "path to what?", and we could someday have includes that are non-plugins...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

path is the standard nix way to refer to local urls for inputs, which is why I used it. My thought was to reuse nix path prefixes:

github:
https:
path:

I would also be OK with dropping path and just allowing relative or absolute paths. So

my/plugin/p.json
https://myplugin.com/p.json
github:user/repo

@Lagoja thoughts?

Copy link
Collaborator

Choose a reason for hiding this comment

The 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 https://acme.com/code/super-tool/tree/main/hammer_plugin.json

🤔 would we ever have non-plugin includes? IIRC there was some talk of having composable devbox.json or other concepts that are "included".

If no, then path sounds great.
if yes, then plugin-path:, plugin-https:, and plugin-github: ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

includes is intended to support devbox.json composability generally and not just plugins. But we need to merge plugin json and devbox.json.

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 plugin as part of the protocol.

Copy link
Collaborator

Choose a reason for hiding this comment

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

includes is intended to support devbox.json composability generally and not just plugins. But we need to merge plugin json and devbox.json.

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 plugin as part of the protocol.

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 json.Unmarshal with current-plugin-schema and if that fails, try unmarshalling with devbox-json-style-plugin-schema.

Okay, no further concerns on this thread.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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).

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't see why regular devbox.json should have create_files...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

]
}
6 changes: 6 additions & 0 deletions examples/plugins/local/devbox.lock
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
{
"lockfile_version": "1",
"packages": {
"path:my-plugin/my-plugin.json": {}
}
}
13 changes: 13 additions & 0 deletions examples/plugins/local/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
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm wondering if we can drop the {{ .Virtenv }}? Why do we need it?

I feel a more natural way for a user to write this is:

"create_files": {
   "empty-dir": "",
   "some-file": "some-file.txt",
 }

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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?

Copy link
Collaborator

Choose a reason for hiding this comment

The 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:

"create_files": {
    "virtenv": {
     "empty-dir": "",
     "some-file": "some-file.txt",
   },
   "devbox.d": {
      "process-config": "process-conf.yaml"
   }
 },

Thoughts? In our docs and examples, we can show that virtenv and devbox.d are the only two acceptable keys.

Alternate names for the keys could be:

  • internal and external, so that plugin authors know which files are internal to their plugin, versus externally editable by users.
  • plugin-owned and user-owned.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 create_files is good. My main counter is I would still allow using template variables and custom paths to allow more customizability. And a (better?) possible replacement is https://github.com/jetpack-io/tyson and if we want to do that, we might regret adding special fields to create_files.

Thoughts?

Copy link
Collaborator

Choose a reason for hiding this comment

The 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?

And a (better?) possible replacement is https://github.com/jetpack-io/tyson and if we want to do that, we might regret adding special fields to create_files.

Can you elaborate on this: why would using TySON be harder with a schema that has whitelisted keys for create_files? How is it different from the whitelisted keys at the top-level like version and env?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wait, what are the other template-variables?

plugins variables are defined here

Created file variables are defined here

I'm feeling pretty uncomfortable about exposing our internal plugin schema directly for external usage where we'll have to support it indefinitely.

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.

Could we write down a spec.

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.

Can you elaborate on this: why would using TySON be harder with a schema that has whitelisted keys for create_files?

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.

}
}
7 changes: 7 additions & 0 deletions examples/plugins/local/my-plugin/process-compose.yaml
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"
1 change: 1 addition & 0 deletions examples/plugins/local/my-plugin/some-file.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
some data
8 changes: 8 additions & 0 deletions examples/plugins/local/test.sh
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
17 changes: 15 additions & 2 deletions internal/impl/devbox.go
Original file line number Diff line number Diff line change
Expand Up @@ -127,12 +127,15 @@ func (d *Devbox) Config() *devconfig.Config {
}

func (d *Devbox) ConfigHash() (string, error) {
hashes := lo.Map(d.PackagesAsInputs(), func(i *devpkg.Package, _ int) string { return i.Hash() })
pkgHashes := lo.Map(d.PackagesAsInputs(), func(i *devpkg.Package, _ int) string { return i.Hash() })
includeHashes := lo.Map(d.Includes(), func(i plugin.Includable, _ int) string { return i.Hash() })
h, err := d.cfg.Hash()
if err != nil {
return "", err
}
return cuecfg.Hash(h + strings.Join(hashes, ""))
return cuecfg.Hash(
h + strings.Join(pkgHashes, "") + strings.Join(includeHashes, ""),
)
}

func (d *Devbox) NixPkgsCommitHash() string {
Expand Down Expand Up @@ -919,6 +922,16 @@ func (d *Devbox) PackagesAsInputs() []*devpkg.Package {
return devpkg.PackageFromStrings(d.Packages(), d.lockfile)
}

func (d *Devbox) Includes() []plugin.Includable {
includes := []plugin.Includable{}
for _, includePath := range d.cfg.Include {
if include, err := d.pluginManager.ParseInclude(includePath); err == nil {
includes = append(includes, include)
}
}
return includes
}

func (d *Devbox) HasDeprecatedPackages() bool {
for _, pkg := range d.PackagesAsInputs() {
if pkg.IsLegacy() {
Expand Down
17 changes: 14 additions & 3 deletions internal/plugin/files.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,20 +4,28 @@
package plugin

import (
"os"
"regexp"
"strings"

"github.com/pkg/errors"
"go.jetpack.io/devbox/internal/devpkg"
"go.jetpack.io/devbox/plugins"
)

func getConfigIfAny(pkg *devpkg.Package, projectDir string) (*config, error) {
func getConfigIfAny(pkg Includable, projectDir string) (*config, error) {
configFiles, err := plugins.BuiltIn.ReadDir(".")
if err != nil {
return nil, errors.WithStack(err)
}

if local, ok := pkg.(*localPlugin); ok {
content, err := os.ReadFile(local.path)
if err != nil && !os.IsNotExist(err) {
return nil, errors.WithStack(err)
}
return buildConfig(pkg, projectDir, string(content))
}

for _, file := range configFiles {
if file.IsDir() || strings.HasSuffix(file.Name(), ".go") {
continue
Expand All @@ -44,6 +52,9 @@ func getConfigIfAny(pkg *devpkg.Package, projectDir string) (*config, error) {
return nil, nil
}

func getFileContent(contentPath string) ([]byte, error) {
func getFileContent(pkg Includable, contentPath string) ([]byte, error) {
if local, ok := pkg.(*localPlugin); ok {
return os.ReadFile(local.contentPath(contentPath))
}
return plugins.BuiltIn.ReadFile(contentPath)
}
73 changes: 68 additions & 5 deletions internal/plugin/includes.go
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
}
31 changes: 19 additions & 12 deletions internal/plugin/plugin.go
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ func (c *config) Services() (services.Services, error) {
}

func (m *Manager) Include(included string) error {
name, err := m.parseInclude(included)
name, err := m.ParseInclude(included)
if err != nil {
return err
}
Expand All @@ -80,7 +80,7 @@ func (m *Manager) Create(pkg *devpkg.Package) error {
return m.create(pkg, m.lockfile.Packages[pkg.Raw])
}

func (m *Manager) create(pkg *devpkg.Package, locked *lock.Package) error {
func (m *Manager) create(pkg Includable, locked *lock.Package) error {
virtenvPath := filepath.Join(m.ProjectDir(), VirtenvPath)
cfg, err := getConfigIfAny(pkg, m.ProjectDir())
if err != nil {
Expand Down Expand Up @@ -129,12 +129,12 @@ func (m *Manager) create(pkg *devpkg.Package, locked *lock.Package) error {
}

func (m *Manager) createFile(
pkg *devpkg.Package,
pkg Includable,
filePath, contentPath, virtenvPath string,
) error {
name := pkg.CanonicalName()
debug.Log("Creating file %q from contentPath: %q", filePath, contentPath)
content, err := getFileContent(contentPath)
content, err := getFileContent(pkg, contentPath)
if err != nil {
return errors.WithStack(err)
}
Expand All @@ -148,21 +148,25 @@ func (m *Manager) createFile(
return err
}

attributePath, err := pkg.PackageAttributePath()
if err != nil {
return err
var urlForInput, attributePath string

if pkg, ok := pkg.(*devpkg.Package); ok {
attributePath, err = pkg.PackageAttributePath()
if err != nil {
return err
}
urlForInput = pkg.URLForFlakeInput()
}

var buf bytes.Buffer
if err = tmpl.Execute(&buf, map[string]any{
"DevboxConfigDir": m.ProjectDir(),
"DevboxDir": filepath.Join(m.ProjectDir(), devboxDirName, name),
"DevboxDirRoot": filepath.Join(m.ProjectDir(), devboxDirName),
"DevboxProfileDefault": filepath.Join(m.ProjectDir(), nix.ProfilePath),
"PackageAttributePath": attributePath,
"Packages": m.Packages(),
"System": system,
"URLForInput": pkg.URLForFlakeInput(),
"URLForInput": urlForInput,
"Virtenv": filepath.Join(virtenvPath, name),
}); err != nil {
return errors.WithStack(err)
Expand Down Expand Up @@ -193,9 +197,12 @@ func (m *Manager) Env(
includes []string,
computedEnv map[string]string,
) (map[string]string, error) {
allPkgs := append([]*devpkg.Package(nil), pkgs...)
allPkgs := []Includable{}
for _, pkg := range pkgs {
allPkgs = append(allPkgs, pkg)
}
for _, included := range includes {
input, err := m.parseInclude(included)
input, err := m.ParseInclude(included)
if err != nil {
return nil, err
}
Expand All @@ -218,7 +225,7 @@ func (m *Manager) Env(
return conf.OSExpandEnvMap(env, computedEnv, m.ProjectDir()), nil
}

func buildConfig(pkg *devpkg.Package, projectDir, content string) (*config, error) {
func buildConfig(pkg Includable, projectDir, content string) (*config, error) {
cfg := &config{}
name := pkg.CanonicalName()
t, err := template.New(name + "-template").Parse(content)
Expand Down
7 changes: 5 additions & 2 deletions internal/plugin/services.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,12 @@ func (m *Manager) GetServices(
) (services.Services, error) {
allSvcs := services.Services{}

allPkgs := append([]*devpkg.Package(nil), pkgs...)
allPkgs := []Includable{}
for _, pkg := range pkgs {
allPkgs = append(allPkgs, pkg)
}
for _, include := range includes {
name, err := m.parseInclude(include)
name, err := m.ParseInclude(include)
if err != nil {
return nil, err
}
Expand Down