feat: Add support for Spin plugins #735

kate-goldenring · 2022-09-02T18:20:01Z

Adds support for Spin Plugins -- SIP 006.

Spin plugins are additional functionality and subcommands that can be added to Spin via this new spin plugin CLI command.

A Spin plugin is defined by a JSON plugin manifest. A plugin manifest defines a plugin’s name, version, license, homepage (i.e. GitHub repo), compatible Spin version, and gives a short description of the plugin. It also points to the plugin source for various operating systems and platforms. This manifest is reference for plugin installs and upgrades.

The spin-plugins repository defines the plugin manifest JSON schema and contains an index of installable Spin and community maintained plugins.

These changes were equally contributed and co-authored by @karthik2804 -- thanks for all fabulous 🍐ing!

Testing locally

Test the flow of installing an example plugin that echos This is an example Spin plugin! when run. It lives in the centralized Spin plugins repository here.

Clone this branch, build spin with make build, and install spin cargo install --locked --path . or replace spin below with the path to the binary ./target/release/spin.

Installing a plugin from the spin-plugins repo

# install latest version of example plugin (to ignore prompting add --yes)
spin plugin install example 
# execute plugin
spin example
# uninstall plugin
spin uninstall example

Upgrading a plugin

spin plugin install example  --yes --version 0.1.0
spin upgrade example
# now could downgrade with: spin plugin upgrade example -d --version 0.1.0

Installing plugin from remote manifest

spin plugin install --url https://github.com/fermyon/spin-plugins/blob/main/manifests/example/example.json

Installing plugin from local manifest
See test_spin_plugin_install_command in integration.rs.

Signed-off-by: Kate Goldenring <[email protected]> Co-authored-by: karthik Ganeshram <[email protected]>

also add Spin compatibility checks, version checks, upgrading via installation. Signed-off-by: karthik Ganeshram <[email protected]> Co-authored-by: Kate Goldenring <[email protected]>

Signed-off-by: Kate Goldenring <[email protected]>

Signed-off-by: karthik Ganeshram <[email protected]>

Signed-off-by: Kate Goldenring <[email protected]> Co-authored-by: Karthik Ganeshram <[email protected]>

itowlson

A few thoughts on the command handlers - I've not had chance to look at the actual crate yet but will do so tomorrow. Most of what I have is stylistic issues rather than functionality, and some of it is subjective I know. Anyway, this is fantastic to see - thanks!

itowlson · 2022-09-05T05:06:36Z

src/commands/plugins.rs

+    /// Name of Spin plugin.
+    #[clap(
+        name = "PLUGIN_NAME",
+        conflicts_with = "REMOTE_PLUGIN_MANIFEST",


Use consts for argument names; that way, if a name changes, it doesn't have to be kept in sync across multiple places.

Good point. Added in 8da5853

itowlson · 2022-09-05T05:56:11Z

src/commands/external.rs

+    let plugin_name = args.first().ok_or_else(|| anyhow!("Expected subcommand"))?;
+    let plugins_dir = get_spin_plugins_directory()?;
+    check_plugin_spin_compatibility(plugin_name, env!("VERGEN_BUILD_SEMVER"), &plugins_dir)?;
+    let path = plugins_dir.join(plugin_name);


Maybe turn this little stanza into a function to keep the detail of path construction out of the flow?

Moved path construction into PluginStore and PluginLookup types for referencing up installed plugins/manifests and looking up plugins from the repository, respectively.

itowlson · 2022-09-05T05:56:47Z

src/commands/external.rs

+    command.envs(&get_env_vars_map(&path)?);
+    log::info!("Executing command {:?}", command);
+    // Allow user to interact with stdio/stdout of child process
+    let status = command.status().await?;


Will this preserve the exit code? (or do we not care)

The exit code is logged as info.

Yeah, I was thinking if a script wanted to use it. In particular, a script will want to know if the plugin succeeded or failed; at the very least, we should return a nonzero exit code if the plugin does, which I don't think happens at the moment? (do correct me if I'm missing it!)

Do you mean in the case where someone has a script that executes spin <plugin> and wants to check on its status to verify that this step succeeded before moving on to the next? If so, you are correct. We are currently not doing this but I agree this must be added. Thanks for pointing it out!

Exactly, or e.g. imagine a spin test plugin being run as part of a CI script.

itowlson · 2022-09-05T05:59:40Z

src/commands/external.rs

+    if cfg!(target_os = "windows") {
+        binary.set_extension("exe");
+    }
+    let mut command = Command::new(binary);


Again, I'd be inclined to hive command object construction into a function, though given it needs path as well as binary maybe it all gets a bit fiddly...

hid it behind a installed_binary_path function

itowlson · 2022-09-05T06:04:26Z

src/commands/external.rs

+        (
+            "SPIN_PLUGIN_PATH".to_string(),
+            path.to_str()
+                .ok_or_else(|| anyhow!("Could not convert plugin path to string"))?


Can you do path.display().to_string() and avoid the Result? Or should we use the underlying OS string (as_os_str) for this and the Spin binary path and pass OsStrings to envs?

src/commands/plugins.rs

itowlson · 2022-09-05T06:33:47Z

src/commands/plugins.rs

+                            name
+                        );
+                    } else {
+                        return Err(e);


Correct to bail on error rather than upgrading remaining plugins? This could mean one bad plugin blocking others from upgrading.

made it so upgrading continues. Good call out!

itowlson · 2022-09-05T06:38:27Z

src/commands/plugins.rs

+            let name = self
+                .name
+                .ok_or_else(|| anyhow!("plugin name is required for upgrades"))?;
+            // If downgrade is allowed, first uninstall the plugin


Why is this needed for downgrades if it isn't for upgrades?

src/commands/plugins.rs

itowlson

Super happy to see this - I have a lot of nits, but they are largely just nits.

The main things that I would like to see are:

Name more things - create more functions and variables
Define some sort of Store or Layout type to manage all the paths - there's a lot of duplication and at some point someone is going to join a path to the wrong parent
Decouple the installer from the specifics of the CLI environment
Ensure errors give good actionable data e.g. file names (or that users can get this info somehow)
Ensure that changing your mind (saying no at the confirmation prompt) is a no-op (I think it might not be in a downgrade scenario)
Distinguish errors (we can't or won't do this) from warnings where the user may want to go ahead anyway even knowing the risks

That said, even these could be iterated on if we want to get this in quickly - the functionality looks great.

itowlson · 2022-09-05T21:03:18Z

crates/plugins/src/plugin_manifest.rs

+use serde::{Deserialize, Serialize};
+
+/// Expected schema of a plugin manifest. Should match the latest Spin plugin
+/// manifest JSON schema:


Sorry for raising this now rather than during the SIP, but all our other manifests are TOML - is there a reason we switched to JSON for this one? (I am okay with an answer of "after giving TOML a try, we prefer JSON after all" if that's the reason!)

We went with JSON because of the large ecosystem around JSON schema documentation and JSON parsing for schema matching. However, I did not realize that TOML also can be parsed with JSON schema; though, I cannot see examples. Have you seen examples of TOML schemas?

The "Even Better TOML" VS Code extension does some stuff with TOML schemas. But yeah, it's not as widely used. It's a definite advantage of JSON.

Should we migrate to JSON for our other manifests to take advantage of schema support, or does the plugin manifest have specific considerations (e.g. higher complexity, less human authoring) that make schema support more important (or human readability less important) than in other manifests?

I like how JSON schemas enable specifying field options (ie OS enum), verify names (regex and enums), can specify manditory and optional fields, and act as documentation on how to construct a manifest. However, the Spin manifest tomls are refreshingly easy to read. I wonder if we can make manifests TOML but when validating them, convert them to JSON and check them against a schema. That could give us the best of both worlds. (Unfortunately the TOML schema parser is unmaintained: https://github.com/json-schema-everywhere/pajv, which is which the initial convert to JSON step may be needed)

I played around with this in this branch of spin-plugins

It feels a little messy. In the end, @itowlson, I think your note summarizes it: spin plugin manifests are less frequently authored and can use the benefits of json tooling (schema validation). However, spin app manifests are validated when they are run, so they dont need json schema validation. So i think using json for spin plugin manifests and toml for app manifests is appropriate

crates/plugins/src/install.rs

itowlson · 2022-09-05T21:44:39Z

crates/plugins/src/install.rs

+        }
+    }
+
+    /// Installs a Spin plugin. First attempts to retrieve the plugin manifest.


This function is 140 lines - can we identify cohesive chunks that could be broken out into functions with intention-revealing names?

I made it chunky :)

itowlson · 2022-09-05T21:49:22Z

crates/plugins/src/install.rs

+    /// architecture. Verifies the checksum of the source, unpacks and installs
+    /// it into the plugins directory.
+    pub async fn install(&self) -> Result<()> {
+        let plugin_manifest: PluginManifest = match &self.manifest_location {


This match expression gets quite long. Could each arm be a smol function (get_manifest_from_url|file|repo)? Would it fit better on the ManifestLocation type? (it looks like one arm uses one local field so not sure, just raising it for consideration)

I agree that the remote repo pulling could be separated out. this motivated me creating a PluginLookup type that focuses on the repo fetching and lookups

crates/plugins/src/version_check.rs

itowlson · 2022-09-05T23:27:48Z

crates/plugins/src/version_check.rs

+            supported,
+            e
+        )
+    })?;


Is there a way for the user to go "okay there's a typo in the version spec but I want it anyway"?

crates/plugins/src/version_check.rs

kate-goldenring · 2022-09-09T02:22:40Z

Thank you @itowlson for this incredible review! This should be in much better shape now. The following still need to be addressed:

Executing plugins as subcommands: Should it be spin example or spin plugin run example. Force mapping to plugins would resolve clap issues on mistyped supported commands and help with conflicts with plugin naming and internal commands (feat: Add support for Spin plugins #735 (comment))
Should we add update that will conditionally pull the latest from spin-plugins repo. Or pull during every install
Should we allow plugins to be installed if they do not pass the version check? This was the original idea but removed from the SIP after more conversation: SIP: Spin Plugins #712 (comment)
I am not sure I have appropriately moved the parsing component to the CLI as you suggested @itowlson

itowlson · 2022-09-09T03:12:46Z

@kate-goldenring for me the "override version check" is a way to bridge a mismatch between developers and users. Consider:

Alice writes a plugin and tests it against Spin 0.5, the latest at that time. Alice doesn't know whether a future version of Spin will break her plugin, so wisely sets its compatibility to 0.5.
Bob installs Alice's plugin and loves it.
Carol adds MySQL support and releases it as Spin 0.6, and Bob (who loves MySQL almost as much as he loves Alice's plugin) upgrades. Oh no! Bob's favourite plugin stops working.
Bob raises an issue on the plugin repo. But Alice has gone on a well-deserved holiday, and won't be back for two months...

The trouble is partly that we're not on useful semver yet, so the change from 0.5 to 0.6 communicates little - they might be backward compatible, or might not. The author has to be conservative and assume minor or patch releases could be breaking, but that means the user gets hosed when a release is only a bugfix or new feature, yet their plugins stop working anyway.

Hope that makes sense. I do take the point that the user could hose themselves by running an incompatible plugin, so a dire warning is fine - but blocking it completely feels too strict for a project where users are on the bleeding edge anyway! As always, happy to discuss further.

(And apologies for not flagging this at the SIP stage - I realise how frustrating it is to have it relitigated.)

kate-goldenring · 2022-09-09T16:03:42Z

@itowlson that does make sense. My only concern is what do we do when plugins are executed? We currently are also checking version there in external.rs because what if:

install spin v0.4.0
install plugin example which supports =0.4.0
upgrade to spin v0.5.0
run spin example

Currently we throw an error (and should also recommend upgrading). I am not sure how we could pass an override-version-check flag to an external command. We could just log here that are running a version that is incompatible and continue. But that doesnt seem ideal either.

Signed-off-by: Kate Goldenring <[email protected]>

src/commands/external.rs

…tdout Signed-off-by: Kate Goldenring <[email protected]>

karthik2804 · 2022-09-11T01:56:03Z

We could pass the override-version-check to the external command as a positional argument. The argument can be more descriptive to ensure the user knows the risk.

The positional argument can be either structured as

spin <version_check_flag> <plugin> <args>

or

spin <plugin> <args> <version_check_flag>

itowlson · 2022-09-11T21:06:57Z

@kate-goldenring Another possibility is to prompt if a version mismatch is detected, with options of run, don't run, and "I know this is safe, don't ask again." If the user chooses "don't ask again," it would apply only to the exact combination of plugin and Spin versions. E.g. if the user upgraded to Spin 0.5.1 then we'd prompt for spin example again.

However, we'd still likely need @karthik2804's "skip version check" option as well, so that people could run "incompatible but known safe" plugins in CI. So this is strictly more effort, and we'd need to decide if we felt the added convenience was worth it.

We'd also need to pay attention to how any prompt worked in CI environments. You don't want to upgrade your CI environment to 0.5.0 and find all your workflows stall without meaningful error! Maybe this is a can of worms... (Or maybe this wouldn't be an issue because without intervention they'd fail at the plugin install stage.)

kate-goldenring · 2022-09-12T17:57:24Z

@itowlson prompting seems feasible. A few follow up thoughts/questions:

Is the current way prompting done sufficient? I want to make sure i am addressing that initial concern of where that lived before extending it for incompatibility prompting.
would the -y / yes-to-all flag also auto yes for this (on install)?
For dont-ask-again, we'd need to store this state. We could store this state by writing to the json manifests some metadata that demarcates this.

itowlson · 2022-09-12T18:53:50Z

Yes, the current way of prompting is okay.
Ooh... that's a good question. We'd be combining two things if it did - skip "are you sure" prompts on the happy path, and skip "wait are you REALLY sure" on the not so happy path. The latter feels like a distinct decision, yet would be needed so that CI environments didn't blindly install incompatible plugins - this is @karthik2804's override option. But having multiple yes flags might be overkill - would we be able to document it clearly? And how would we handle CI when the flag wasn't passed - would they block?

Maybe this is too much of a can of worms. I am struggling rather with how to offer an experience that would be safe in CI but forgiving to interactive users. Perhaps always failing on incompatibility (and allowing Karthik's flag to override) is safer...

itowlson · 2022-09-12T18:55:39Z

I am conscious the whole override thing is relitigating the SIP so I'm happy to drop it for this PR if you prefer - nothing stops us coming back to it later.

kate-goldenring · 2022-09-12T21:57:34Z

@bacongobbler what are your thoughts on adding an --override-version-check flag: #735 (comment)? It opposes the original logic of the SIP that was motivated in part by your comment here #712 (comment).

itowlson

@kate-goldenring @karthik2804 Thanks for this - to me at least this iteration is much easier to follow, and it partitions responsibilities in a way I really liked. There are a few things I think still need a bit of tweaking, but they are local (e.g. more info on errors, renaming here and there) rather than structural. Great stuff!

crates/plugins/src/lookup.rs

crates/plugins/src/manager.rs

crates/plugins/src/prompt.rs

src/commands/plugins.rs

…nstall functions Signed-off-by: Kate Goldenring <[email protected]>

itowlson

I really like how this is looking! And I'm excited to see where this leads us in the future!

bacongobbler · 2022-09-14T07:27:21Z

@bacongobbler what are your thoughts on adding an --override-version-check flag: #735 (comment)?

@itowlson raises a good point. Because Spin is not yet 1.0, we cannot guarantee compatibility for plugin developers between Spin 0.x and Spin 0.x+1. It is very likely that we'll have plugin developers pin their plugins to a specific release for the time being until we can guarantee backwards compatibility (1.0.0). That will cause this situation to turn up fairly often.

Post-1.0, the same situation could occur if Spin 2.0.0 was released and Alice was no longer maintaining the plugin. Bob would likely want to use Alice's plugin against Spin 2.0, but because Alice stepped away from the project, nobody is available to update it. Though in that case, perhaps Bob should consider forking Alice's plugin to ensure compatibility with 2.0.0...

However, given that

This is an opt-in flag that will help plugin users in a "break in case of emergency" event, and
It still provides the plugin author some form of protection from letting their users install plugins that were incompatible with this version of Spin

I'm okay with the feature flag.

One point of feedback that you're welcome to disregard: have you thought about putting this behind an interactive prompt window instead? Say for example Bob was installing Alice's plugin with Spin 0.6:

$ spin plugin install plugin-by-alice
WARNING: This plugin has not been tested for compatibility with Spin v0.6.0. Proceed? (y/N) y
Installing...

With --override-version-check bypassing the prompt.

kate-goldenring · 2022-09-14T15:09:35Z

@bacongobbler @itowlson sounds like we've reached consensus on adding an --override-version-check (or maybe better --override-compatibility-check) flag. For the sake of stabilizing the state of this PR, let's add that to a subsequent PR after this.

tests/integration.rs

lann

Mostly minor style nits / suggestions. Nothing impacting correctness except potentially the to_string_lossy stuff, though that's pretty unlikely to cause problems.

No blockers from my perspective.

src/commands/external.rs

crates/plugins/src/git.rs

crates/plugins/src/manifest.rs

crates/plugins/src/store.rs

src/commands/external.rs

src/commands/plugins.rs

kate-goldenring · 2022-09-19T01:32:00Z

@lann Thank you for the great pointers here -- learned a lot! I think I addressed your comments -- but happy to iterate if you have more feedback.

lann

Looks good! A few more style suggestions on the changes, but again nothing blocking.

crates/plugins/src/error.rs

crates/plugins/src/manager.rs

crates/plugins/src/manifest.rs

crates/plugins/src/lookup.rs

crates/plugins/src/manifest.rs

crates/plugins/src/manager.rs

Signed-off-by: Kate Goldenring <[email protected]>

lann

Code LGTM. Unfortunately I didn't participate as much as others in the SIP review so I don't have much to say on functionality.

kate-goldenring and others added 4 commits September 2, 2022 10:51

feat: add support for spin plugins

3e6e130

Signed-off-by: Kate Goldenring <[email protected]> Co-authored-by: karthik Ganeshram <[email protected]>

feat: support uninstall sub-function for spin plugins

146d5c4

also add Spin compatibility checks, version checks, upgrading via installation. Signed-off-by: karthik Ganeshram <[email protected]> Co-authored-by: Kate Goldenring <[email protected]>

feat: add support for upgrade sub-command for Spin plugins

53bcc3c

Signed-off-by: Kate Goldenring <[email protected]>

feat: add environment variables to be passed to plugin subcommand

be5aa26

Signed-off-by: karthik Ganeshram <[email protected]>

kate-goldenring requested review from radu-matei, lann, vdice, itowlson and bacongobbler September 2, 2022 18:21

kate-goldenring force-pushed the spin-plugins branch from 4a90393 to 7b56ff2 Compare September 2, 2022 20:22

fix: add integration tests and code clean up

0fb6ead

Signed-off-by: Kate Goldenring <[email protected]> Co-authored-by: Karthik Ganeshram <[email protected]>

kate-goldenring force-pushed the spin-plugins branch from 7b56ff2 to 0fb6ead Compare September 2, 2022 20:23

itowlson requested changes Sep 5, 2022

View reviewed changes

kate-goldenring force-pushed the spin-plugins branch from 9806046 to 8da5853 Compare September 9, 2022 01:46

kate-goldenring force-pushed the spin-plugins branch from 8da5853 to cedf319 Compare September 9, 2022 16:34

Clean up spin plugins and refactor to use PluginStore and PluginManager

2fc0f23

Signed-off-by: Kate Goldenring <[email protected]>

kate-goldenring force-pushed the spin-plugins branch from cedf319 to 2fc0f23 Compare September 9, 2022 16:50

Support for spin plugin update

cb41d38

Signed-off-by: Kate Goldenring <[email protected]>

kate-goldenring commented Sep 9, 2022

View reviewed changes

src/commands/external.rs Outdated Show resolved Hide resolved

kate-goldenring force-pushed the spin-plugins branch 3 times, most recently from 36056e7 to 8cd0c2c Compare September 9, 2022 22:05

fix: update spin plugin integration test to use env vars and assess s…

629cccc

…tdout Signed-off-by: Kate Goldenring <[email protected]>

kate-goldenring force-pushed the spin-plugins branch from 8cd0c2c to 629cccc Compare September 10, 2022 00:47

itowlson requested changes Sep 13, 2022

View reviewed changes

Add context to spin plugin errors and return types to install and uni…

d9d9417

…nstall functions Signed-off-by: Kate Goldenring <[email protected]>

itowlson approved these changes Sep 13, 2022

View reviewed changes

lann mentioned this pull request Sep 15, 2022

Spin core refactor #763

Merged

lann reviewed Sep 15, 2022

View reviewed changes

tests/integration.rs Outdated Show resolved Hide resolved

lann reviewed Sep 15, 2022

View reviewed changes

lann approved these changes Sep 19, 2022

View reviewed changes

kate-goldenring force-pushed the spin-plugins branch 2 times, most recently from 6e13841 to e5d319e Compare September 19, 2022 15:49

lann reviewed Sep 19, 2022

View reviewed changes

crates/plugins/src/manifest.rs Outdated Show resolved Hide resolved

crates/plugins/src/manager.rs Outdated Show resolved Hide resolved

Implement custom Plugin error, clean up lossy use, fix matching on bool

9d85795

Signed-off-by: Kate Goldenring <[email protected]>

kate-goldenring force-pushed the spin-plugins branch from e5d319e to 9d85795 Compare September 19, 2022 16:35

lann approved these changes Sep 19, 2022

View reviewed changes

radu-matei approved these changes Sep 19, 2022

View reviewed changes

kate-goldenring merged commit 4e52721 into fermyon:main Sep 19, 2022

kate-goldenring deleted the spin-plugins branch September 19, 2022 21:28

kate-goldenring mentioned this pull request Sep 21, 2022

feat: add support to override Spin compatibility checks for plugins #780

Merged

feat: Add support for Spin plugins #735

feat: Add support for Spin plugins #735

Conversation

kate-goldenring commented Sep 2, 2022

Adds support for Spin Plugins -- SIP 006.

Testing locally

itowlson 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

karthik2804 Sep 7, 2022 • 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

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

itowlson 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

kate-goldenring commented Sep 9, 2022 • edited Loading

itowlson commented Sep 9, 2022

kate-goldenring commented Sep 9, 2022

karthik2804 commented Sep 11, 2022

itowlson commented Sep 11, 2022

kate-goldenring commented Sep 12, 2022

itowlson commented Sep 12, 2022

itowlson commented Sep 12, 2022

kate-goldenring commented Sep 12, 2022

itowlson left a comment

Choose a reason for hiding this comment

itowlson left a comment

Choose a reason for hiding this comment

bacongobbler commented Sep 14, 2022 • edited Loading

kate-goldenring commented Sep 14, 2022

lann left a comment

Choose a reason for hiding this comment

kate-goldenring commented Sep 19, 2022

lann left a comment

Choose a reason for hiding this comment

lann left a comment

Choose a reason for hiding this comment

karthik2804 Sep 7, 2022 •

edited

Loading

kate-goldenring commented Sep 9, 2022 •

edited

Loading

bacongobbler commented Sep 14, 2022 •

edited

Loading