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

Fix plugins versions being alpha compared #2947

Merged
merged 1 commit into from
Dec 4, 2024

Conversation

itowlson
Copy link
Contributor

@itowlson itowlson commented Dec 4, 2024

Fixes #2946

Signed-off-by: itowlson <ivan.towlson@fermyon.com>
Copy link
Member

@vdice vdice left a comment

Choose a reason for hiding this comment

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

I'm still confused on how the previous logic incorrectly determined 0.9.0 to be greater than 0.10.0... in any case, I wonder if now might be a good time to add a few unit tests here? (Or do we already have some?)

@itowlson
Copy link
Contributor Author

itowlson commented Dec 4, 2024

@vdice The previous logic used lexical comparison of the version strings. 0=0, .=., but 9>1. (The PluginManifest::version field is a String not a Version, I assume because it is a serialisation type and needs to be forgiving of strings like "canary".)

There's a lot of places I would like to add unit tests for the plugin manager, but it requires some significant refactoring to do that. For example, the function I change here doesn't just validate the installability of the plugin, it also reads from disk and prints to the terminal. So testing it becomes a pain. I can pull out the bit that checks if this is a downgrade and test that function, but at that point we're barely above the level of testing if semver::Version implements Ord correctly. So: I agree: this does need a test; but I am not sure how to write a useful one in the current code structure.

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.

Thank you for fixing this @itowlson

@itowlson itowlson merged commit 7ec2df8 into fermyon:main Dec 4, 2024
17 checks passed
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.

Plugin version number calculation seems off
4 participants