Multiselect plugin upgrade #2091
Conversation
There was a problem hiding this comment.
There's a few things, which I think will be obvious when you're able to run the command (I left a message in the Discord thread but am not sure I understood the difficulty - feel free to ping me), but given you've not been able to run it and are new to Rust, this is a heroic effort! Thanks for this - I'm sure we can land it soon!
src/commands/plugins.rs
Outdated
| @@ -63,7 +63,6 @@ pub struct Install { | |||
| name = PLUGIN_NAME_OPT, | |||
| conflicts_with = PLUGIN_REMOTE_PLUGIN_MANIFEST_OPT, | |||
| conflicts_with = PLUGIN_LOCAL_PLUGIN_MANIFEST_OPT, | |||
| required_unless_present_any = [PLUGIN_REMOTE_PLUGIN_MANIFEST_OPT, PLUGIN_LOCAL_PLUGIN_MANIFEST_OPT], | |||
There was a problem hiding this comment.
Not sure if it's intentional that this PR changes the install command? I think you might have intended to remove this from line 170 instead?
There was a problem hiding this comment.
You’re right, obviously. Awkward mistake.
src/commands/plugins.rs
Outdated
| "Select plugins to upgrade. Use Space to select/deselect and Enter to confirm selection." | ||
| ); | ||
| let selected_indexes = match dialoguer::MultiSelect::new() | ||
| .items(&installed_in_catalogue) |
There was a problem hiding this comment.
This looks like it currently displays all plugins installed from the catalogue, even those without an eligible upgrade.
There was a problem hiding this comment.
You're correct. What I tried to do to check which plugins are eligible for an upgrade was to call get_manifest() with the latest version (using manifest_location(None)) for each plugin. If the function returns successfully, indicating an update is available, I push it to the new eligible_plugins vector with the descriptor and the new manifest to have both the old and latest versions.
| }) | ||
| .collect(); | ||
|
|
||
| eprintln!( |
There was a problem hiding this comment.
We need to handle the case where there are no upgrades available - it should print an informative message rather than an empty selector.
There was a problem hiding this comment.
Of course. Condition added in line 293.
src/commands/plugins.rs
Outdated
| let manager = PluginManager::try_default()?; | ||
| let manifest_location = ManifestLocation::PluginsRepository(PluginLookup::new( | ||
| &plugin.name, | ||
| Some(plugin.version.parse::<Version>()?), |
There was a problem hiding this comment.
I am not sure this is quite right. installed_in_catalogue contains the installed plugins at their installed versions. This therefore appears to use the already-installed version as the version to upgrade to. I may be misreading though?
There was a problem hiding this comment.
You are right. Based on my understanding of the code, it seems that to determine the version for an upgrade, I need to provide 'None' as the version when calling manifest_location(). From the tests I've conducted, I believe I have it correct this time.
src/commands/plugins.rs
Outdated
| @@ -335,7 +437,7 @@ pub struct List { | |||
| impl List { | |||
| pub async fn run(self) -> Result<()> { | |||
| let mut plugins = if self.installed { | |||
| Self::list_installed_plugins() | |||
| list_installed_plugins() | |||
| } else { | |||
| Self::list_catalogue_and_installed_plugins().await | |||
There was a problem hiding this comment.
Although not required for this PR, I would move list_catalogue_and_installed_plugins to be a module function too, so that a reader coming to this in 6 months is not puzzled by "why is one of these functions module level and the other one associated?"
There was a problem hiding this comment.
Hmm, that makes sense!
src/commands/plugins.rs
Outdated
| @@ -474,6 +538,26 @@ impl PluginDescriptor { | |||
| } | |||
| } | |||
|
|
|||
| impl std::fmt::Display for PluginDescriptor { | |||
There was a problem hiding this comment.
I'd be a little bit cautious about giving this as the display format for a plugin descriptor. List::print, for example, prints them in its own specific format.
My suggestion for the multiselect display would be either:
- Just the plugin name (e.g.
foo) - The plugin name with the current version and what it would upgrade to (e.g.
foo (from 1.0.0 to 1.2.0))
You can use strings rather than descriptors in the Multiselect items list as long as you ensure they keep the same order (have the same indexes) so that indexing back into your descriptor list still works e.g.
// using the "just the name" example
let names: Vec<_> = descriptors.iter().map(|d| d.name.to_string()).collect(); // preserves order
let indexes = dialoguer::Multiselect::new().items(&names)...;
let selected = elements_at(descriptors, indexes);
There was a problem hiding this comment.
I combined the two ideas you gave me and create a names vector, and using the macro format! I appended the versions to the name to display more information to the user as you suggested.
|
I've attempted to address all the comments correctly, and I believe we are closer to getting it right. This time, I was able to test it via the CLI. The tests I performed were as follows:
Looking forward for your feedback. |
There was a problem hiding this comment.
Thanks for this! I left a few more comments but the two I am concerned about are:
- The compatibility check while getting the list of installed plugins that are also in the catalogue (comment on line 267)
- The empty list messaging (command on line 294 but will need a tweak further up the file as well)
Otherwise this looks great. And thanks for taking the time to describe the tests you did!
src/commands/plugins.rs
Outdated
| .filter(|installed| { | ||
| catalogue_plugins.iter().any(|catalogue| { | ||
| installed.manifest == catalogue.manifest | ||
| && matches!(catalogue.compatibility, PluginCompatibility::Compatible) |
There was a problem hiding this comment.
I am not sure we need this check. We don't really care if the catalogue copy of the installed plugin is compatible. In fact, a possible case is:
- User installs Plugin 1.0 from catalogue. 1.0 is compatible with Spin 3.0 or below.
- User upgrades to Spin 4.1.
- User tries to run Plugin and it warns about compatibility.
- User tries to upgrade.
In this case we would not want to filter Plugin 1.0 out of the list.
We care about compatibility on upgrade versions but I don't think that's what this is looking at. I might be misunderstanding though? (As you've rightly noted, unit tests would be the way to catch cases like this - it's hard to simulate with actual plugins!)
There was a problem hiding this comment.
To check the compatibility of the upgrade versions, I believe using the PluginCompatibility::for_current() is the best way to do it. Correct me, if I'm wrong.
So, I'll add the condition below after the versions comparison:
if let PluginCompatibility::Compatible = PluginCompatibility::for_current(&manifest)
There was a problem hiding this comment.
That sounds good - thanks!
As a style detail, if let PluginCompatibility::Compatible = PC::for_current(...) might be more idiomatic as matches!(PC::for_current(...), PC::Compatible) - the let is a bit odd because no actual assignment is possible. You could also add PartialEq to the PluginCompatibility #[derive(...)] list, which would allow you to write if PC::Compatible == PC::for_current. None of this is a blocker though!
There was a problem hiding this comment.
You're totally right. Thanks for the hint!
src/commands/plugins.rs
Outdated
| .map(|(descriptor, manifest)| { | ||
| format!( | ||
| "{} from version {} to {}", | ||
| descriptor.name.clone(), |
There was a problem hiding this comment.
nit: I don't think you need to clone these. (On the other hand, I assume you're doing it because Rust complained if you didn't! And there's no harm in it for sure.)
There was a problem hiding this comment.
Yes, I was doing it because, when I was trying to use the descriptor in the dialoguer::MultiSelect, I couldn't move the values. But since I found a simpler way to display the names and versions it isn't needed anymore!
src/commands/plugins.rs
Outdated
| .get_manifest(&manifest_location, false, SPIN_VERSION) | ||
| .await | ||
| { | ||
| if installed_plugin.version != manifest.version() { |
There was a problem hiding this comment.
In theory we should check that the catalogue manifest version is greater than the installed version but I don't know if that's ever an issue in practice.
There was a problem hiding this comment.
Yeah, you're right, even if this is an issue in practice, in theory, we should check if the version is greater than the installed one.
What we could do is parse the strings to numbers and compare them. What do you think?
There was a problem hiding this comment.
If we were to do this, we would not parse to numbers but to the semver::Version type (see https://docs.rs/semver/latest/semver/). The Manifest type has a try_version() method which can give you this And semver::Version allows you to use comparison operators such as < and > (see e.g. https://github.com/fermyon/spin/blob/1d662e3774f85106de4b212ae48d0d4760f47ee2/crates/plugins/src/badger/mod.rs#L285).
That said, it's up to you whether you want to wrestle with this. Unfortunately, it is not quite as easy as just calling try_version() and doing a comparison, because try_version can return an error (if the version is malformed). Which means we have to think about error handling. Rust makes it very easy to simply bomb out (via the ? operator), but that could mean one bad version string broke the whole experience. One option would be to write a helper function that handled the error cases by falling back to string comparison, along the lines of
fn is_potential_upgrade(current: &PluginManifest, candidate: &PluginManifest) -> bool {
match (current.try_version(), candidate.try_version()) {
(Ok(cur_ver), Ok(cand_ver)) => cand_ver > cur_ver,
_ => current.version() != candidate.version(),
}
}
and then this line would be if is_potential_upgrade(&installed_plugin, &manifest). (Disclaimer: written in the GitHub comment box, not tested, unlikely to compile as is!)
It's up to you whether you want to invest this amount of effort - it's not a blocker as far as I'm concerned.
There was a problem hiding this comment.
Why current.version() != candidate.version() if something goes wrong? I know this is the condition we have at the moment, but shouldn't we return false if something goes wrong with try_version()'s?
And also, I think the is_potencial function works perfectly!
There was a problem hiding this comment.
Suppose we returned false if one of the versions failed to parse. Now suppose that somehow (again, should never happen) the user's installed version fails to parse. Now they don't have the option to upgrade.
This could be avoided by having it return false if only the target version fails to parse. But since we're displaying the from and to versions, I'd recommend erring on the side of leaving the choice with the user.
Again, it should be moot because there should never be a malformed version in the catalogue. But that was the motivation for the more forgiving fallback.
Signed-off-by: joaogdemacedo <joaogdemacedo@gmail.com>
Signed-off-by: joaogdemacedo <joaogdemacedo@gmail.com>
Signed-off-by: joaogdemacedo <joaogdemacedo@gmail.com>
Change error message: - from "No plugins found to upgrade"; - to "All plugins are up to date"; Co-authored-by: itowlson <github@hestia.cc> Signed-off-by: João De Macedo <joaogdemacedo@gmail.com>
Signed-off-by: joaogdemacedo <joaogdemacedo@gmail.com>
Signed-off-by: joaogdemacedo <joaogdemacedo@gmail.com>
Head branch was pushed to by a user without write access
joaogdemacedo
force-pushed
the
multiselect-plugin-upgrade
branch
from
8c42579 to
a362755
Compare
Trying to add a multi-select plugin upgrade (issue #1173) as my first PR.
I still haven't tested it via CLI yet, because I'm still wondering how I can run the command with the changes I've made. I can't just run 'spin plugins upgrade', right?
Looking forward for your feedback.