[plugins] Allow local non-compiled plugins #1224

mikeland73 · 2023-06-28T02:24:59Z

Summary

This change allows users to add their own plugins using devbox.json include field (see example). It now supports all plugin fields (including services).

Fixed: Issue where editing plugin would not be reflected.

How was it tested?

devbox run run_test -c examples/plugins/local
MY_FOO_VAR is set to 'BAR'
devbox services up

Lagoja · 2023-06-29T22:12:19Z

Pretty awesome! Env variables + services seem to be working, and updating when I make changes

I noticed that if I add additional files to add_files in the example, they don't update on subsequent devbox shell runs. Is there something special I need to do for changes to that section of the plugin to have effect?

mikeland73 · 2023-06-30T18:38:39Z

@Lagoja right now you need to delete your .devbox/local.lock file to force it to recreate. I'll fix this.

savil

This is awesome! Any thoughts on the questions below?

savil · 2023-06-30T17:16:40Z

examples/plugins/local/devbox.json

+    ],
+    "scripts": {
+      "run_test": [
+        "./test.sh"


yes! thank you for adding this run_test

savil · 2023-06-30T18:18:27Z

examples/plugins/local/my-plugin/my-plugin.json

+    "{{ .Virtenv }}/empty-dir": "",
+    "{{ .Virtenv }}/some-file": "some-file.txt",
+    "{{ .Virtenv }}/process-compose.yaml": "process-compose.yaml"


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", }

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.

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?

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.

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?

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?

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.

savil · 2023-06-30T18:25:04Z

examples/plugins/local/devbox.json

+    }
+  },
+  "include": [
+    "path:my-plugin/my-plugin.json"


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

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?

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

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.

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.

@savil the schemas are almost identical. It's a matter of union-ing them (and possibly deprecating anything that doesn't make sense).

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

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.

savil · 2023-06-30T18:33:58Z

internal/plugin/includes.go

+	name, ok := m["name"].(string)
+	if !ok || name == "" {
+		return nil,
+			usererr.New("plugin %s is missing a required field 'name'", path)
+	}


I'd suggest setting some constraints on allowable names. Maybe:

specify that they must belong to [a-z][A-Z]-

length limit of 32 or 64 characters
?

I'd worry that having no constraints would get "creatively" used

savil

After thinking it over, while we could have a stricter and more statically analyzable schema for the config, we'll be using golang templates for the create_files anyway. And it seems unavoidable (without a full rewrite of the plugin manager).

I'm going to approve to unblock, but perhaps lets discuss in meeting tomorrow prior to landing...

LucilleH · 2023-07-06T20:25:13Z

Sorry for being late to the party - I like this, and the only thing I'm thinking about is around create_files and how tyson can make this interface much nicer

[plugins] Prototype that allows local non-compiled plugins

855c3f8

mikeland73 requested review from loreto and Lagoja June 28, 2023 02:25

mikeland73 added 3 commits June 27, 2023 19:25

Revert change

9fa4242

Merge branch 'main' into landau/local-plugins

60effe5

Add support for creating files and services

6edd44e

mikeland73 requested a review from savil June 29, 2023 19:46

mikeland73 changed the title ~~[plugins] Prototype that allows local non-compiled plugins~~ [plugins] Allow local non-compiled plugins Jun 29, 2023

Make isLocal private

639fac6

savil reviewed Jun 30, 2023

View reviewed changes

mikeland73 added 3 commits June 30, 2023 13:52

Reflect plugin changes

0e762a3

validate plugin name

9c54926

Merge branch 'main' into landau/local-plugins

bec133a

savil approved these changes Jul 6, 2023

View reviewed changes

mikeland73 added 2 commits July 5, 2023 21:42

Merge branch 'main' into landau/local-plugins

a7bf30a

Remove unused var

7e9ba47

mikeland73 merged commit 6c833de into main Jul 6, 2023

mikeland73 deleted the landau/local-plugins branch July 6, 2023 18:04

[plugins] Allow local non-compiled plugins #1224

[plugins] Allow local non-compiled plugins #1224

Uh oh!

Conversation

mikeland73 commented Jun 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

How was it tested?

Uh oh!

Lagoja commented Jun 29, 2023

Uh oh!

mikeland73 commented Jun 30, 2023

Uh oh!

savil left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

savil left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LucilleH commented Jul 6, 2023

Uh oh!

Uh oh!

mikeland73 commented Jun 28, 2023 •

edited

Loading

savil left a comment •

edited

Loading