From 476cad37bb3379505ecb89937d07d17eaba09d8a Mon Sep 17 00:00:00 2001 From: Paul Doyle Date: Tue, 24 May 2022 19:28:37 -0700 Subject: [PATCH 1/5] Log decisions around asset name choice --- src/tool_cache.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/tool_cache.rs b/src/tool_cache.rs index e7acdcf..c0fb254 100644 --- a/src/tool_cache.rs +++ b/src/tool_cache.rs @@ -109,12 +109,14 @@ impl ToolCache { Version::parse(&release.tag_name[1..]).ok() })?; + log::trace!("Checking for name with compatible os/arch pair from platform-derived list: {:?}", platform_keywords()); let asset_index = release.assets.iter().position(|asset| { platform_keywords() .iter() .any(|keyword| asset.name.contains(keyword)) })?; + log::debug!("Found matching artifact: {}", release.assets[asset_index].name); Some((version, asset_index, release)) }) .collect(); From 56a2b7eed5bf080f8299f452fe8070e114dc3d9f Mon Sep 17 00:00:00 2001 From: Paul Doyle Date: Wed, 25 May 2022 09:47:19 -0700 Subject: [PATCH 2/5] Factor out code that needs separate testing --- src/tool_cache.rs | 56 ++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 48 insertions(+), 8 deletions(-) diff --git a/src/tool_cache.rs b/src/tool_cache.rs index c0fb254..d6eea3a 100644 --- a/src/tool_cache.rs +++ b/src/tool_cache.rs @@ -18,9 +18,24 @@ use crate::{ error::{ForemanError, ForemanResult}, fs, paths::ForemanPaths, - tool_provider::ToolProvider, + tool_provider::{ToolProvider, Release}, }; +fn choose_asset(release: &Release, platform_keywords: &[&str]) -> Option { + log::trace!( + "Checking for name with compatible os/arch pair from platform-derived list: {:?}", + platform_keywords + ); + let asset_index = release.assets.iter().position(|asset| { + platform_keywords + .iter() + .any(|keyword| asset.name.contains(keyword)) + })?; + + log::debug!("Found matching artifact: {}", release.assets[asset_index].name); + Some(asset_index) +} + /// Contains the current state of all of the tools that Foreman manages. #[derive(Debug, PartialEq, Serialize, Deserialize)] pub struct ToolCache { @@ -109,14 +124,8 @@ impl ToolCache { Version::parse(&release.tag_name[1..]).ok() })?; - log::trace!("Checking for name with compatible os/arch pair from platform-derived list: {:?}", platform_keywords()); - let asset_index = release.assets.iter().position(|asset| { - platform_keywords() - .iter() - .any(|keyword| asset.name.contains(keyword)) - })?; + let asset_index = choose_asset(&release, platform_keywords())?; - log::debug!("Found matching artifact: {}", release.assets[asset_index].name); Some((version, asset_index, release)) }) .collect(); @@ -225,8 +234,39 @@ fn tool_identifier_to_exe_name(tool: &ToolSpec, version: &Version) -> String { mod test { use tempfile::tempdir; + use crate::tool_provider::ReleaseAsset; + use super::*; + // Regression test for LUAFDN-1041 + #[test] + fn select_correct_asset() { + let platform_keywords = &["macos-x86_64", "darwin-x86_64", "macos", "darwin"]; + let release = Release { + prerelease: false, + tag_name: "v0.5.2".to_string(), + assets: vec![ + ReleaseAsset { + name: "tool-linux.zip".to_string(), + url: "https://example.com/some/repo/releases/assets/1".to_string(), + }, + ReleaseAsset { + name: "tool-macos-arm64.zip".to_string(), + url: "https://example.com/some/repo/releases/assets/2".to_string(), + }, + ReleaseAsset { + name: "tool-macos-x86_64.zip".to_string(), + url: "https://example.com/some/repo/releases/assets/3".to_string(), + }, + ReleaseAsset { + name: "tool-win64.zip".to_string(), + url: "https://example.com/some/repo/releases/assets/4".to_string(), + }, + ], + }; + assert_eq!(choose_asset(&release, platform_keywords), Some(2)); + } + mod load { use super::*; From c5d8059aa68d991950895904e108e3e31df4e0db Mon Sep 17 00:00:00 2001 From: Paul Doyle Date: Wed, 25 May 2022 09:52:36 -0700 Subject: [PATCH 3/5] swap loops so that we find best match for most specific keyword before we find the first asset that matches ANY keyword --- src/tool_cache.rs | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/tool_cache.rs b/src/tool_cache.rs index d6eea3a..07a4b9d 100644 --- a/src/tool_cache.rs +++ b/src/tool_cache.rs @@ -26,10 +26,8 @@ fn choose_asset(release: &Release, platform_keywords: &[&str]) -> Option "Checking for name with compatible os/arch pair from platform-derived list: {:?}", platform_keywords ); - let asset_index = release.assets.iter().position(|asset| { - platform_keywords - .iter() - .any(|keyword| asset.name.contains(keyword)) + let asset_index = platform_keywords.iter().find_map(|keyword| { + release.assets.iter().position(|asset| asset.name.contains(keyword)) })?; log::debug!("Found matching artifact: {}", release.assets[asset_index].name); @@ -238,7 +236,7 @@ mod test { use super::*; - // Regression test for LUAFDN-1041 + // Regression test for LUAFDN-1041, based on the release that surfaced it #[test] fn select_correct_asset() { let platform_keywords = &["macos-x86_64", "darwin-x86_64", "macos", "darwin"]; From 075afeaece2611809e3d711613dd90897ad8af46 Mon Sep 17 00:00:00 2001 From: Paul Doyle Date: Wed, 25 May 2022 09:57:35 -0700 Subject: [PATCH 4/5] Make test a bit more thorough --- src/tool_cache.rs | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/src/tool_cache.rs b/src/tool_cache.rs index 07a4b9d..9940dfc 100644 --- a/src/tool_cache.rs +++ b/src/tool_cache.rs @@ -239,7 +239,6 @@ mod test { // Regression test for LUAFDN-1041, based on the release that surfaced it #[test] fn select_correct_asset() { - let platform_keywords = &["macos-x86_64", "darwin-x86_64", "macos", "darwin"]; let release = Release { prerelease: false, tag_name: "v0.5.2".to_string(), @@ -262,7 +261,17 @@ mod test { }, ], }; - assert_eq!(choose_asset(&release, platform_keywords), Some(2)); + assert_eq!(choose_asset(&release, &["win32", "win64", "windows"]), Some(3)); + assert_eq!(choose_asset(&release, &["macos-x86_64", "darwin-x86_64", "macos", "darwin"]), Some(2)); + assert_eq!(choose_asset(&release, &[ + "macos-arm64", + "darwin-arm64", + "macos-x86_64", + "darwin-x86_64", + "macos", + "darwin", + ]), Some(1)); + assert_eq!(choose_asset(&release, &["linux"]), Some(0)); } mod load { From 3cc8ac5a7d66d72b9123696e7751afcbcc27883a Mon Sep 17 00:00:00 2001 From: Paul Doyle Date: Wed, 25 May 2022 09:59:18 -0700 Subject: [PATCH 5/5] cargo fmt --- src/tool_cache.rs | 47 ++++++++++++++++++++++++++++++++++------------- 1 file changed, 34 insertions(+), 13 deletions(-) diff --git a/src/tool_cache.rs b/src/tool_cache.rs index 9940dfc..77a7d4e 100644 --- a/src/tool_cache.rs +++ b/src/tool_cache.rs @@ -18,7 +18,7 @@ use crate::{ error::{ForemanError, ForemanResult}, fs, paths::ForemanPaths, - tool_provider::{ToolProvider, Release}, + tool_provider::{Release, ToolProvider}, }; fn choose_asset(release: &Release, platform_keywords: &[&str]) -> Option { @@ -27,10 +27,16 @@ fn choose_asset(release: &Release, platform_keywords: &[&str]) -> Option platform_keywords ); let asset_index = platform_keywords.iter().find_map(|keyword| { - release.assets.iter().position(|asset| asset.name.contains(keyword)) + release + .assets + .iter() + .position(|asset| asset.name.contains(keyword)) })?; - log::debug!("Found matching artifact: {}", release.assets[asset_index].name); + log::debug!( + "Found matching artifact: {}", + release.assets[asset_index].name + ); Some(asset_index) } @@ -261,16 +267,31 @@ mod test { }, ], }; - assert_eq!(choose_asset(&release, &["win32", "win64", "windows"]), Some(3)); - assert_eq!(choose_asset(&release, &["macos-x86_64", "darwin-x86_64", "macos", "darwin"]), Some(2)); - assert_eq!(choose_asset(&release, &[ - "macos-arm64", - "darwin-arm64", - "macos-x86_64", - "darwin-x86_64", - "macos", - "darwin", - ]), Some(1)); + assert_eq!( + choose_asset(&release, &["win32", "win64", "windows"]), + Some(3) + ); + assert_eq!( + choose_asset( + &release, + &["macos-x86_64", "darwin-x86_64", "macos", "darwin"] + ), + Some(2) + ); + assert_eq!( + choose_asset( + &release, + &[ + "macos-arm64", + "darwin-arm64", + "macos-x86_64", + "darwin-x86_64", + "macos", + "darwin", + ] + ), + Some(1) + ); assert_eq!(choose_asset(&release, &["linux"]), Some(0)); }