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 plugin version comparison #13907

Merged
merged 2 commits into from
Jul 11, 2024
Merged

Fix plugin version comparison #13907

merged 2 commits into from
Jul 11, 2024

Conversation

msujew
Copy link
Member

@msujew msujew commented Jul 10, 2024

What it does

Closes #13904

I'm confused by how this wasn't caught earlier. Even though our method was supposed to return gte, it did run lte internally. I've simply adjusted the called method to match the name and documentation. See also:

image

How to test

Try installing the Astro VS Code extension via the vsx-registry view. It should work as expected.

Review checklist

Reminder for reviewers

@msujew msujew added the vsx-registry Issues related to Open VSX Registry Integration label Jul 10, 2024
@rschnekenbu
Copy link
Contributor

I remember discussing that point when reviewing #13118. I'll have a double check then.

Copy link
Contributor

@rschnekenbu rschnekenbu left a comment

Choose a reason for hiding this comment

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

I think we will have an issue for builtins if we fix this bug.
See #13118 (comment). With the existing implementation, we check the builtins version is lower than our API compatibility version.

I would suggest to change the order of the parameters here:

return getLatestCompatibleVersion(allVersions => this.versionGreaterThanOrEqualTo(allVersions.version, this.supportedApiVersion));
. If not, we will finally install builtins with a version greater than our API compatibility.

@msujew msujew requested a review from rschnekenbu July 10, 2024 16:02
Copy link
Contributor

@rschnekenbu rschnekenbu left a comment

Choose a reason for hiding this comment

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

Looks good to me! I could install astro (latest version) and also I could have Github builtin 1.88.1 installed.

@msujew msujew merged commit f12a854 into master Jul 11, 2024
14 checks passed
@msujew msujew deleted the msujew/fix-version-comparison branch July 11, 2024 09:34
@github-actions github-actions bot added this to the 1.52.0 milestone Jul 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
vsx-registry Issues related to Open VSX Registry Integration
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Plugin install fail from open-vsx.org
2 participants