-
-
Notifications
You must be signed in to change notification settings - Fork 18
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
[#32] Add tag option to download a specific tool version #61
Changes from 1 commit
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 |
---|---|---|
|
@@ -62,12 +62,14 @@ fn decode_config_asset(table: &Map<String, Value>) -> ConfigAsset { | |
let repo = str_by_key(table, "repo"); | ||
let exe_name = str_by_key(table, "exe_name"); | ||
let asset_name = decode_asset_name(table); | ||
let tag = str_by_key(table, "tag"); | ||
|
||
ConfigAsset { | ||
owner, | ||
repo, | ||
exe_name, | ||
asset_name, | ||
tag, | ||
} | ||
} | ||
|
||
|
@@ -161,6 +163,7 @@ mod tests { | |
macos: None, | ||
windows: None, | ||
}, | ||
tag: None, | ||
}, | ||
)]), | ||
}; | ||
|
@@ -193,6 +196,7 @@ mod tests { | |
macos: None, | ||
windows: None, | ||
}, | ||
tag: None, | ||
}, | ||
), | ||
( | ||
|
@@ -206,6 +210,7 @@ mod tests { | |
macos: None, | ||
windows: None, | ||
}, | ||
tag: None, | ||
}, | ||
), | ||
]), | ||
|
@@ -239,6 +244,7 @@ mod tests { | |
macos: None, | ||
windows: None, | ||
}, | ||
tag: None, | ||
}, | ||
)]), | ||
}; | ||
|
@@ -275,6 +281,7 @@ mod tests { | |
macos: Some("C3-PO".to_owned()), | ||
windows: Some("IG-88".to_owned()), | ||
}, | ||
tag: None, | ||
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. The test |
||
}, | ||
)]), | ||
}; | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -1,6 +1,6 @@ | ||||||
use crate::config::schema::ConfigAsset; | ||||||
use crate::model::asset_name::AssetName; | ||||||
use crate::model::tool::{Tool, ToolError, ToolInfo}; | ||||||
use crate::model::tool::{Tool, ToolError, ToolInfo, ToolInfoTag}; | ||||||
use crate::sync::db::lookup_tool; | ||||||
|
||||||
pub fn configure_tool(tool_name: &str, config_asset: &ConfigAsset) -> Tool { | ||||||
|
@@ -32,6 +32,11 @@ fn full_configure(config_asset: &ConfigAsset) -> Option<ToolInfo> { | |||||
let owner = config_asset.owner.clone()?; | ||||||
let repo = config_asset.repo.clone()?; | ||||||
let exe_name = config_asset.exe_name.clone()?; | ||||||
let tag = config_asset | ||||||
.tag | ||||||
.clone() | ||||||
.map(|version| ToolInfoTag::Specific(version)) | ||||||
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. I'm not sure but I believe this simplification should compile 🤔
Suggested change
|
||||||
.unwrap_or(ToolInfoTag::Latest); | ||||||
|
||||||
Some(ToolInfo { | ||||||
owner, | ||||||
|
@@ -42,6 +47,7 @@ fn full_configure(config_asset: &ConfigAsset) -> Option<ToolInfo> { | |||||
macos: config_asset.asset_name.macos.clone(), | ||||||
windows: config_asset.asset_name.windows.clone(), | ||||||
}, | ||||||
tag, | ||||||
}) | ||||||
} | ||||||
|
||||||
|
@@ -78,6 +84,11 @@ impl ToolInfo { | |||||
.clone() | ||||||
.or_else(|| self.asset_name.windows.clone()), | ||||||
}, | ||||||
tag: config_asset | ||||||
.tag | ||||||
.clone() | ||||||
.map(|version| ToolInfoTag::Specific(version)) | ||||||
.unwrap_or(ToolInfoTag::Latest), | ||||||
} | ||||||
} | ||||||
} | ||||||
|
@@ -99,6 +110,7 @@ mod tests { | |||||
macos: None, | ||||||
windows: None, | ||||||
}, | ||||||
tag: None, | ||||||
}; | ||||||
|
||||||
assert_eq!( | ||||||
|
@@ -120,6 +132,7 @@ mod tests { | |||||
macos: None, | ||||||
windows: None, | ||||||
}, | ||||||
tag: None, | ||||||
}; | ||||||
|
||||||
assert_eq!( | ||||||
|
@@ -141,6 +154,7 @@ mod tests { | |||||
macos: None, | ||||||
windows: None, | ||||||
}, | ||||||
tag: None, | ||||||
}; | ||||||
|
||||||
assert_eq!( | ||||||
|
@@ -164,6 +178,7 @@ mod tests { | |||||
macos: None, | ||||||
windows: None, | ||||||
}, | ||||||
tag: Some(String::from("1.2.3")), | ||||||
}; | ||||||
|
||||||
assert_eq!( | ||||||
|
@@ -185,6 +200,7 @@ mod tests { | |||||
macos: Some(String::from("my-macos")), | ||||||
windows: Some(String::from("yours-windows")), | ||||||
}, | ||||||
tag: Some(String::from("1.2.3")), | ||||||
}; | ||||||
|
||||||
assert_eq!( | ||||||
|
@@ -197,7 +213,8 @@ mod tests { | |||||
linux: Some("my-linux".to_string()), | ||||||
macos: Some("my-macos".to_string()), | ||||||
windows: Some("yours-windows".to_string()), | ||||||
} | ||||||
}, | ||||||
tag: ToolInfoTag::Specific("1.2.3".to_string()), | ||||||
}) | ||||||
); | ||||||
} | ||||||
|
@@ -215,6 +232,7 @@ mod tests { | |||||
macos: None, | ||||||
windows: None, | ||||||
}, | ||||||
tag: None, | ||||||
}; | ||||||
|
||||||
assert_eq!( | ||||||
|
@@ -227,7 +245,8 @@ mod tests { | |||||
linux: Some("unknown-linux-musl".to_string()), | ||||||
macos: Some("apple-darwin".to_string()), | ||||||
windows: Some("x86_64-pc-windows-msvc".to_string()), | ||||||
} | ||||||
}, | ||||||
tag: ToolInfoTag::Latest, | ||||||
}) | ||||||
); | ||||||
} | ||||||
|
@@ -245,6 +264,7 @@ mod tests { | |||||
macos: Some(String::from("my-macos")), | ||||||
windows: Some(String::from("yours-windows")), | ||||||
}, | ||||||
tag: Some(String::from("3.2.1")), | ||||||
}; | ||||||
|
||||||
assert_eq!( | ||||||
|
@@ -257,7 +277,8 @@ mod tests { | |||||
linux: Some("my-linux".to_string()), | ||||||
macos: Some("my-macos".to_string()), | ||||||
windows: Some("yours-windows".to_string()), | ||||||
} | ||||||
}, | ||||||
tag: ToolInfoTag::Specific("3.2.1".to_string()), | ||||||
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. Really appreciate updated tests 🙏🏻 |
||||||
}) | ||||||
); | ||||||
} | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,6 +12,7 @@ pub struct Downloader<'a> { | |
pub owner: &'a str, | ||
pub repo: &'a str, | ||
pub asset_name: &'a str, | ||
pub specific_tag: &'a Option<&'a str>, | ||
pub pb_msg: &'a ProgressBar, | ||
pub sync_progress: &'a SyncProgress, | ||
} | ||
|
@@ -24,11 +25,20 @@ pub struct DownloadInfo { | |
|
||
impl<'a> Downloader<'a> { | ||
fn release_url(&self) -> String { | ||
format!( | ||
"https://api.github.com/repos/{owner}/{repo}/releases/latest", | ||
owner = self.owner, | ||
repo = self.repo | ||
) | ||
if let Some(tag) = self.specific_tag { | ||
format!( | ||
"https://api.github.com/repos/{owner}/{repo}/releases/tags/{tag}", | ||
owner = self.owner, | ||
repo = self.repo, | ||
tag = tag, | ||
) | ||
} else { | ||
format!( | ||
"https://api.github.com/repos/{owner}/{repo}/releases/latest", | ||
owner = self.owner, | ||
repo = self.repo, | ||
) | ||
} | ||
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. I propose a slight simplification to this logic. Let's rename This way, this formatting could be just: format!(
"https://api.github.com/repos/{owner}/{repo}/releases/{version}",
owner = self.owner,
repo = self.repo,
version = tag.to_str_version(),
) |
||
} | ||
|
||
fn asset_url(&self, asset_id: u32) -> String { | ||
|
@@ -118,3 +128,42 @@ pub fn add_auth_header(req: ureq::Request) -> ureq::Request { | |
Ok(token) => req.set("Authorization", &format!("token {}", token)), | ||
} | ||
} | ||
|
||
#[cfg(test)] | ||
mod tests { | ||
use super::*; | ||
|
||
#[test] | ||
fn release_url_with_no_specific_tag_is_correct() { | ||
let downloader = Downloader { | ||
owner: "OWNER", | ||
repo: "REPO", | ||
asset_name: "ASSET_NAME", | ||
specific_tag: &None, | ||
pb_msg: &ProgressBar::hidden(), | ||
sync_progress: &SyncProgress::new(vec!["tool".to_string()]), | ||
}; | ||
|
||
assert_eq!( | ||
downloader.release_url(), | ||
"https://api.github.com/repos/OWNER/REPO/releases/latest" | ||
); | ||
} | ||
|
||
#[test] | ||
fn release_url_with_specific_tag_is_correct() { | ||
let downloader = Downloader { | ||
owner: "OWNER", | ||
repo: "REPO", | ||
asset_name: "ASSET_NAME", | ||
specific_tag: &Some("SPECIFIC_TAG"), | ||
pb_msg: &ProgressBar::hidden(), | ||
sync_progress: &SyncProgress::new(vec!["tool".to_string()]), | ||
}; | ||
|
||
assert_eq!( | ||
downloader.release_url(), | ||
"https://api.github.com/repos/OWNER/REPO/releases/tags/SPECIFIC_TAG" | ||
); | ||
} | ||
} |
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.
Thanks for updating the documentation as well! 👏🏻