Skip to content
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

Implement spin plugin list #972

Merged
merged 1 commit into from
Dec 15, 2022
Merged

Implement spin plugin list #972

merged 1 commit into from
Dec 15, 2022

Conversation

itowlson
Copy link
Contributor

@itowlson itowlson commented Dec 14, 2022

Fixes #937.

I'd really value comments on the structure of this - even more than usual, because I kind of felt that a bunch of this must already be in there but I wasn't sure where to find it. So please do flag up if logic is in the wrong places or are already done somewhere else.

The output with this code is loosely based on how apt looks on my WSL Ubuntu system, and looks like this:

$ spin plugin list
arm-only 0.1.0 [incompatible]
example 0.1.0
example 0.2.0
js2wasm 0.1.0 [installed]
not-spin-ver 9.1.0 [incompatible]
not-spin-ver 10.2.0 [requires other Spin version]
windows-only 0.1.0 [incompatible]

$ spin plugin list --installed
js2wasm 0.1.0 [installed]

It could alternatively be printed in tabular format, and/or with colour highlighting, and/or with emoticons - the PluginDescriptor hopefully decouples the printing logic enough that this would be easy to fiddle with.

At the moment, this glides silently over plugins that are in such a corrupted state that we can't even load them. This is preferable to being unable to list plugins at all just because one got mangled while I was trying to exit vi. It would be nice to print a warning if any were skipped, but it's a bit fiddly to do without losing the friendly ? syntax - I can have a noodle on it if people think it's worth it though.

Signed-off-by: itowlson ivan.towlson@fermyon.com

Copy link
Contributor

@kate-goldenring kate-goldenring left a comment

Choose a reason for hiding this comment

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

Just some minor comments but this is great! I love how you implemented this to look similar with apt and make it clear the "status" of a plugin (compatible with host, compatible with Spin, installed, etc) and how easy it will be to add the different print styles in future if desired.

I think glossing over invalidly formatted plugins is okay. They shouldn't be able to be merged into the plugins repo as they would fail the json schema workflow. Something could get through but as you mention, it is likely not worth the syntactical disruption.

crates/plugins/src/lookup.rs Show resolved Hide resolved
crates/plugins/src/store.rs Outdated Show resolved Hide resolved
crates/plugins/tests/not-spin-ver/not-spin-ver.json Outdated Show resolved Hide resolved
src/commands/plugins.rs Outdated Show resolved Hide resolved
src/commands/plugins.rs Outdated Show resolved Hide resolved
crates/plugins/tests/README.md Show resolved Hide resolved
crates/plugins/src/store.rs Show resolved Hide resolved
Copy link
Contributor

@kate-goldenring kate-goldenring left a comment

Choose a reason for hiding this comment

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

Looks like a Rust fmt fix and this is good to go

@itowlson
Copy link
Contributor Author

Huh. cargo fmt isn't requiring any changes for me...

@itowlson
Copy link
Contributor Author

OH IT'S A NEW CLIPPY THANKS FOR THE BREAKING CHANGE ON A MINOR VERSION RUST

Signed-off-by: itowlson <ivan.towlson@fermyon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

request to add spin plugin list command
2 participants