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: assembly fails check for minor version differences #27

Merged
merged 2 commits into from
Aug 13, 2024

Conversation

TheRealAmazonKendra
Copy link
Contributor

We now allow use of any version within the same major version.

Fixes #

We now allow use of any version within the same major version.
Copy link
Contributor

@scanlonp scanlonp left a comment

Choose a reason for hiding this comment

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

I don't see a test for failing or succeeding on a lower major version. This is the only behavior that could change here.

Good to approve if we want to fail on lower major versions.

lib/manifest.ts Outdated
// first validate the version should be accepted.
if (semver.gt(actual, maxSupported) && !options?.skipVersionCheck) {
// first validate the version should be accepted. all versions within the same minor version are fine
if (maxSupported !== semver.major(actual) && !options?.skipVersionCheck) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Did this allow lower major versions before? Now it will be confined to only the specific major version.

Is that what we want, or do we still want to do a greater than check but just on the major version?

Copy link
Contributor

Choose a reason for hiding this comment

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

follow up nit: if we are just limiting the version to a single major version, the "maximum version supported" language does not have much meaning any more

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suppose we can allow lower but it doesn't actually make a ton of sense to do so. However, we only ever checked for higher before so I'll stick with that for now and we can revisit it later.

@mergify mergify bot merged commit 42679d5 into main Aug 13, 2024
12 checks passed
@mergify mergify bot deleted the TheRealAmazonKendra/minor-version branch August 13, 2024 18:34
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.

2 participants