-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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(shim): avoid panic if user has malformed lockfile #8461
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,7 +5,7 @@ | |
|
||
use camino::Utf8PathBuf; | ||
use dunce::canonicalize as fs_canonicalize; | ||
use semver::Version; | ||
use semver::{Version, VersionReq}; | ||
Check warning on line 8 in crates/turborepo-lib/src/shim/local_turbo_state.rs
|
||
use serde::Deserialize; | ||
use tracing::debug; | ||
use turbopath::{AbsoluteSystemPath, AbsoluteSystemPathBuf}; | ||
|
@@ -185,37 +185,39 @@ | |
} | ||
|
||
pub fn turbo_version_has_shim(version: &str) -> bool { | ||
let version = Version::parse(version).unwrap(); | ||
// only need to check major and minor (this will include canaries) | ||
if version.major == 1 { | ||
return version.minor >= 7; | ||
if let Ok(version) = Version::parse(version) { | ||
// only need to check major and minor (this will include canaries) | ||
if version.major == 1 { | ||
return version.minor >= 7; | ||
} | ||
version.major > 1 | ||
} else { | ||
// In the case that we don't get passed a valid semver we should avoid a panic. | ||
// We shouldn't hit this we introduce back infering package version from schema | ||
// or package.json. | ||
true | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is technically too loose, but the correct answer for the past 21 months of |
||
} | ||
|
||
version.major > 1 | ||
} | ||
|
||
#[cfg(test)] | ||
mod test { | ||
use test_case::test_case; | ||
|
||
use super::*; | ||
|
||
#[test] | ||
fn test_skip_infer_version_constraint() { | ||
let canary = "1.7.0-canary.0"; | ||
let newer_canary = "1.7.0-canary.1"; | ||
let newer_minor_canary = "1.7.1-canary.6"; | ||
let release = "1.7.0"; | ||
let old = "1.6.3"; | ||
let old_canary = "1.6.2-canary.1"; | ||
let new = "1.8.0"; | ||
let new_major = "2.1.0"; | ||
|
||
assert!(turbo_version_has_shim(release)); | ||
assert!(turbo_version_has_shim(canary)); | ||
assert!(turbo_version_has_shim(newer_canary)); | ||
assert!(turbo_version_has_shim(newer_minor_canary)); | ||
assert!(turbo_version_has_shim(new)); | ||
assert!(turbo_version_has_shim(new_major)); | ||
assert!(!turbo_version_has_shim(old)); | ||
assert!(!turbo_version_has_shim(old_canary)); | ||
#[test_case("1.7.0-canary.0", true; "canary")] | ||
#[test_case("1.7.0-canary.1", true; "newer_canary")] | ||
#[test_case("1.7.1-canary.6", true; "newer_minor_canary")] | ||
#[test_case("1.7.0", true; "release")] | ||
#[test_case("1.6.3", false; "old")] | ||
#[test_case("1.6.2-canary.1", false; "old_canary")] | ||
#[test_case("1.8.0", true; "new")] | ||
#[test_case("2.1.0", true; "new major")] | ||
#[test_case("*", true; "star")] | ||
#[test_case("2.0", true; "version 2 0")] | ||
#[test_case("latest", true; "latest")] | ||
#[test_case("canary", true; "canary tag")] | ||
fn test_skip_infer_version_constraint(version: &str, expected: bool) { | ||
assert_eq!(turbo_version_has_shim(version), expected); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: unused import warning