[plugins] Implement plugins v2 #1850

mikeland73 · 2024-02-27T05:38:34Z

Summary

Apologies in advance for the size of this PR. Was not really sure how to split it up, since it's a fairly large step change.

This PR makes 2 major changes:

Change how plugin fields are processed. Instead of being processed independently in several parts of the code (Env, Scripts, etc) they are now combined inside of devconfig in a simple, predictable way.
Plugins are now allowed to have all fields that devbox.json have. In fact the plugin struct now embeds a ConfigFile.

All fields from included plugins are added to the environment. The merge algorithm is as follows:

includes are evaluated in order first. Base config is evaluated last.
Slices are concatenated (init hooks, etc)
maps are merged with later evaluation replacing previous one on conflict.
Packages are deduped by name and concatenated. (i.e. multiple versions not allowed)

Known issues/limitations:

Github plugins will be slow. When using remote imports, every devbox command does 1 or more HTTP requests to load the imports.
central lock file. All dependencies are stored there.
While recursive includes are allowed, relative paths are not handled correctly. That means if project A includes plugin 1 and plugin 1 includes a relative local plugin, it will not work. Including github plugins recursively works as expected (but will be slow).
Cycle detection is not implemented. A user can cause infinite include.
Plugins are json only. They don't support hujson. (This is inline with current plugins, but we should build support to match devbox.json)

Additional requires testing:

Test create_files works as expected (for both local and github)
Test process compose works as expected
Test global push pull (config changes may have affected that)
Ensure that we are able to add a package that is already added by plugin.

Follow up cleanup:

Remove tyson support (this will simplify config code)
Plugin manager can be removed or greatly reduced.
Fix some of the limitations (relative paths, cycle detection)
Cache included remote plugins for performance (and to avoid github limits).

How was it tested?

New example tests
Manual testing
Existing tests

savil · 2024-02-29T18:49:24Z

From reading the summary, a couple of callouts:

Design of github plugins

"Github plugins will be slow. When using remote imports, every devbox command does 1 or more HTTP requests to load the imports."
"Cache included remote plugins for performance (and to avoid github limits).".

Two concerns:

Reproducibility: if we are pulling from Github every time, then every change to that Github repo will be instantly reflected. Instead, could we lock the current version in time in the lockfile (perhaps resolve to a git commit)? Then devbox update can fetch the very latest version.
Denial of service: due to the github limits being hit, especially for folks using Devbox in CICD. As a mitigation, we can add support for GITHUB_TOKEN that has higher limits. In CICD, users should be recommended to set that as well.

Plugins evaluation order

"includes are evaluated in order first. Base config is evaluated last."

This makes sense. How are we handling plugins that are applied on packages added in the base-config? For example, a Rust plugin that must be applied to the version of Rust included in the base config.

savil

Reviewed some. Pausing to do other work. Will resume.

Good:

Love that we are moving plugin code out of functions like computeEnv

Scenarios to consider:

if a plugin changes, then devbox shell will get the latest content. How will direnv know to refresh its environment?

savil · 2024-02-29T19:23:08Z

internal/devconfig/config.go

+		return false
+	}
+	return !cfg.Root.Equals(&DefaultConfig().Root)
+}


nit: Can we have this be IsDefault and caller can use the not operator?

Otherwise, in the future, if we'll want to write a function that needs IsDefault and will have to call it as !devconfig.IsNotDefault and we get a double negative

will do in cleanup.

savil · 2024-02-29T19:24:28Z

internal/devconfig/config.go

 	b, err := os.ReadFile(path)
 	if err != nil {
 		return nil, err
 	}
-	baseConfig, err := loadBytes(b)
+	return loadBytes(b)


can replace entire function's implemenation with return readFromFile(path)

savil · 2024-02-29T19:49:03Z

internal/devconfig/config.go

+	}, nil
+}
+
+func (c *Config) LoadRecursive(lockfile *lock.File) error {


reviewed till here.

gcurtis

Also still reviewing. I'll finish it up this afternoon.

gcurtis · 2024-03-02T06:08:06Z

internal/devconfig/config.go

 }

-func (c *Config) PackageMutator() *packagesMutator {
+func LoadConfigFromURL(url string) (*Config, error) {


Pass in context so HTTP request can be interrupted.

savil

Pausing for a bit. Left some more comments but not any fundamental design problems so far!

Question:

is there a way to disable a builtin plugin by having an includable plugin supersede it? For example, the builtin python plugin may be doing something a user feels is undesirable and would like to either disable it completely, or override it with their own python plugin

savil · 2024-03-04T16:52:20Z

internal/devconfig/config.go

+func (c *Config) LoadRecursive(lockfile *lock.File) error {
+	included := make([]*Config, 0, len(c.Root.Include))
+
+	for _, importRef := range c.Root.Include {


nit: will be nice to stick to consistent terminology by calling it includeRef instead of importRef

savil · 2024-03-04T18:45:19Z

internal/devconfig/config.go

+	}
+
+	builtIns, err := plugin.GetBuiltinsForPackages(
+		c.Root.PackagesMutator.Collection,


It's a bit odd to have PackagesMutator be the source of reading which packages are in the devbox.json file. Is there a nicer API we can use?

Something like configFileStruct.Packages() so the code here would be c.Root.Packages()

I think implementing Packages() in both the config and the configFile would be a bit confusing.

I actually think Collection can be private. This would make it an implementation detail. Thoughts?

Never mind, this is used outside of the configfile package.

I'm adding SingleFilePackages and will make Collection unexported.

savil · 2024-03-04T18:48:24Z

internal/devconfig/config.go

+		pluginData := builtIn.PluginOnlyData
+		includable.pluginData = &pluginData


I don't follow: why are we re-setting the includable.pluginData here after the LoadRecursive call?

This looks weird. This may be a mistake.

savil · 2024-03-04T18:49:55Z

internal/devconfig/config.go

 	return &c.Root.PackagesMutator
 }

-func (c *Config) Packages() []Package {
-	return c.Root.PackagesMutator.collection
+func (c *Config) PluginConfigs() []*plugin.Config {


Could we call this IncludedPluginConfigs()?

Because: I'd have expected PluginConfigs() to be IncludedPluginConfigs() + BuiltInPluginConfigs()

savil · 2024-03-04T18:53:11Z

internal/devconfig/config.go

+	if c.pluginData != nil {
+		configs = append(configs, &plugin.Config{
+			ConfigFile:     c.Root,
+			PluginOnlyData: *c.pluginData,
+		})
+	}


🤔 I don't follow. Why do we add this additional plugin.Config?

This is the step that actually adds the current plugin in the recursive stack. In pseudocode:

configs = get-all-child-configs; if cur-is-plugin: configs = append(configs, config-for-cur)

It uses c.pluginData != nil to ensure it doesn't add the top level devbox.json (which is not a plugin).

It runs after the for loop in order to respect the order configs are evaluated in.

savil · 2024-03-04T19:34:39Z

internal/devconfig/configfile/file.go

 func (c *ConfigFile) Bytes() []byte {
 	b := c.ast.root.Pack()
 	return bytes.ReplaceAll(b, []byte("\t"), []byte("  "))
 }

 func (c *ConfigFile) Hash() (string, error) {
+	if c.ast == nil {
+		return cachehash.JSON(c)
+	}


in what scenario would c.ast be nil?

Instead, should we be building the ast in this if-block?

plugin configs don't use hujson (but will in the future). Scope of this change is already too big to include that moving part.

savil · 2024-03-04T19:47:25Z

internal/devpkg/package.go

+		pkg.patchGlibc = sync.OnceValue(func() bool {
+			return cfgPkg.PatchGlibc && nix.SystemIsLinux()


is changing to a function really needed?

I'm not convinced because:nix.SystemIsLinux relies on nix.System which is pre-computed once when running the cobra command, so likely won't be an actual system call here

This is needed because unfortunately devbox CLI calls devbox.Open when populating autocomplete for scripts. (https://github.com/jetpack-io/devbox/blob/main/internal/boxcli/run.go#L54-L54). Note that this code happens before ensureNixInstalled is called. Arguably, we don't want to require nix for stuff like listing scripts which doesn't need nix.

So devbox.Open cannot call anything requiring nix, otherwise all commends will fail. Without this change, just parsing a devbox.json requires nix.

We should probably replace command.ValidArgs with command.ValidArgsFunction but even then, I think it's useful to keep devbox.Open working without nix.

savil · 2024-03-04T19:58:45Z

internal/plugin/includes.go

@@ -4,11 +4,22 @@ import (
 	"strings"

 	"go.jetpack.io/devbox/internal/devpkg"
+	"go.jetpack.io/devbox/internal/lock"


Reviewed till here.

savil

@mikeland73 nice, overall lgtm. Please re-request review for when I should look again.

Please see above comments in the comment-box and in code for:

questions about areas i couldn't follow why changes were made
suggestions for minor improvements

A concern with this design is that scripts must be fully embedded in the plugin's devbox.json file, rather than moved into their own my_script.sh file where they can be properly handled by tooling (e.g. syntax-hightlighting, linting, etc.). Enabling that scenario would be a big win IMO.

One idea for a follow up PR is to add special-case support for detecting script files in plugin-configs, and downloading them as well.

savil · 2024-03-04T21:24:43Z

internal/plugin/manager.go

@@ -49,37 +43,3 @@ func (m *Manager) ApplyOptions(opts ...managerOption) {
 		opt(m)
 	}
 }
-


What is the role of the Plugin Manager, after this PR's changes?

After this PR it does:

Builds includables and creates files

Figures out what services a plugin has.

We may want to split those out. Maybe the services stuff goes into the services package and the includable stuff moves into devconf

savil · 2024-03-04T21:25:53Z

internal/plugin/plugin.go

 	// If true, we remove the package that triggered this plugin from the environment
 	// Useful when we want to replace with flake
 	RemoveTriggerPackage bool   `json:"__remove_trigger_package,omitempty"`
 	Version              string `json:"version"`
+	Source               Includable


Could we add a comment explaining what Source is?

savil · 2024-03-04T21:27:57Z

internal/plugin/reflike.go

 }

-func parseReflike(s string) (Includable, error) {
+func parseReflike(s, projectDir string) (Includable, error) {


what is s? Lets rename it to a bigger, meaningful term

same for withFilename below

Will do as part of cleanup

savil · 2024-03-04T21:31:06Z

internal/plugin/services.go

-	pkgs []*devpkg.Package,
-	includes []string,
-) (services.Services, error) {
+func (m *Manager) GetServices(configs []*Config) (services.Services, error) {


Could we make this a standalone package function? Seems like it no longer needs to be a struct method

mikeland73 · 2024-03-04T22:47:25Z

is there a way to disable a builtin plugin by having an includable plugin supersede it? For example, the builtin python plugin may be doing something a user feels is undesirable and would like to either disable it completely, or override it with their own python plugin

Currently you have to use --disable-plugin when adding a package and then add your own plugin. It's not perfect, but seems OK since this is a bit of an edge case.

A concern with this design is that scripts must be fully embedded in the plugin's devbox.json file, rather than moved into their own my_script.sh file where they can be properly handled by tooling (e.g. syntax-hightlighting, linting, etc.). Enabling that scenario would be a big win IMO.

One idea for a follow up PR is to add special-case support for detecting script files in plugin-configs, and downloading them as well.

This use case is doable, but requires a bit more work from the user.

If you want script files they must be copied over using create_files field. Copying over all files is a bit tricky because we need to figure out the file structure of the repo (we can do this with git or maybe github has an API, not sure. Either way, a decent amount of more work).

mikeland73 · 2024-03-05T00:18:19Z

@savil made requested changes (here and in #1867). PTAL

savil

nice!

savil · 2024-03-05T01:37:02Z

internal/devconfig/config.go

-func LoadConfigFromURL(url string) (*Config, error) {
-	res, err := http.Get(url)
+func LoadConfigFromURL(ctx context.Context, url string) (*Config, error) {
+	req, err := http.NewRequestWithContext(ctx, http.MethodHead, url, nil)


this should be http.MethodGet, right?

On line 84, we are trying to read res.Body which HEAD requests don't return

oof, good catch. Terrible copy pasta on my part

savil · 2024-03-05T01:39:50Z

internal/devconfig/configfile/file.go

+
+// SingleFilePackages returns the packages in the config file, but not the included ones.
+// Semi-awkwardly named to avoid confusion with the Packages method on Config.
+func (c *ConfigFile) SingleFilePackages() []Package {


TopLevelPackages ?

savil · 2024-03-05T01:40:08Z

internal/devconfig/configfile/packages.go

-	// Collection contains the set of package definitions
-	Collection []Package
+	// collection contains the set of package definitions
+	collection []Package


## Summary Stacked on #1850 * Remove RefLike * Remove experimental tyson support (v2 plugins are a reasonable substitute) ## How was it tested? CICD

sentry-io · 2024-03-07T19:17:18Z

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

‼️ **syscall.Errno: <redacted errors.withStack>: <redacted os.LinkError>: go.jetpack.io/devbox/internal/plugin in createS... View Issue

_{Did you find this useful? React with a 👍 or 👎}

mikeland73 changed the title ~~Landau/implement plugins v2~~ [plugins] Implement plugins v2 Feb 27, 2024

mikeland73 changed the base branch from main to landau/shift-plugins-to-config February 27, 2024 05:38

mikeland73 force-pushed the landau/implement-plugins-v2 branch from e87d345 to b8169c0 Compare February 28, 2024 06:02

mikeland73 marked this pull request as ready for review February 28, 2024 06:18

mikeland73 requested review from savil and gcurtis February 28, 2024 06:18

This was referenced Feb 28, 2024

[devbox.json] Add and implement imports field #1824

Closed

[plugins] Use reflikes for plugins #1845

Merged

mikeland73 force-pushed the landau/implement-plugins-v2 branch 3 times, most recently from f07d2a1 to 2eba821 Compare February 29, 2024 00:50

mikeland73 mentioned this pull request Feb 29, 2024

[cleanup] Remove reflike and tyson #1867

Merged

savil reviewed Feb 29, 2024

View reviewed changes

Base automatically changed from landau/shift-plugins-to-config to main February 29, 2024 21:16

mikeland73 added 11 commits February 29, 2024 13:23

[plugins] Implement plugins v2

47b2d75

Fix

8a3c517

Fixes

e10e122

Fixe

374f201

Fixes

909279a

Clean up

c43c881

Change builtin example

7ad242a

Fixes

d229b83

Revert lockfile

8603448

Fixed issues, added tests

5f78c5d

Update test

b647d04

mikeland73 force-pushed the landau/implement-plugins-v2 branch from 2eba821 to b647d04 Compare February 29, 2024 21:23

gcurtis reviewed Mar 4, 2024

View reviewed changes

savil reviewed Mar 4, 2024

View reviewed changes

Requested changes

1f2fa55

mikeland73 requested review from gcurtis and savil March 5, 2024 00:17

savil approved these changes Mar 5, 2024

View reviewed changes

gcurtis approved these changes Mar 5, 2024

View reviewed changes

mikeland73 added 3 commits March 5, 2024 11:40

Fixes

78cecdc

More examples

5666a0c

Merge branch 'main' into landau/implement-plugins-v2

029f649

mikeland73 merged commit 3e8115a into main Mar 5, 2024
24 checks passed

mikeland73 deleted the landau/implement-plugins-v2 branch March 5, 2024 20:07

mikeland73 added a commit that referenced this pull request Mar 5, 2024

[cleanup] Remove reflike and tyson (#1867)

9f11a98

## Summary Stacked on #1850 * Remove RefLike * Remove experimental tyson support (v2 plugins are a reasonable substitute) ## How was it tested? CICD

		pluginData := builtIn.PluginOnlyData
		includable.pluginData = &pluginData

		pkg.patchGlibc = sync.OnceValue(func() bool {
		return cfgPkg.PatchGlibc && nix.SystemIsLinux()

@@ @@ -49,37 +43,3 @@ func (m *Manager) ApplyOptions(opts ...managerOption) { @@
               		opt(m)
               	}
               }

[plugins] Implement plugins v2 #1850

[plugins] Implement plugins v2 #1850

Conversation

mikeland73 commented Feb 27, 2024 • edited Loading

Summary

How was it tested?

savil commented Feb 29, 2024

savil left a comment

Choose a reason for hiding this comment

savil Feb 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gcurtis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

savil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

savil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikeland73 commented Mar 4, 2024

mikeland73 commented Mar 5, 2024

savil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sentry-io bot commented Mar 7, 2024

Suspect Issues

mikeland73 commented Feb 27, 2024 •

edited

Loading

savil Feb 29, 2024 •

edited

Loading