Fix artifact selection bug (#53)

Artifact selection uses an outer loop over all assets, and an inner loop over possible valid names. This means that it will pick the first asset that matches any valid names, instead of the best match for the earliest name in the list.

The fix should just be to switch the loops!

Author: ZoteTheMighty

src/tool_cache.rs

@@ -18,9 +18,28 @@ use crate::{
     error::{ForemanError, ForemanResult},
     fs,
     paths::ForemanPaths,
-    tool_provider::ToolProvider,
+    tool_provider::{Release, ToolProvider},
 };
 
+fn choose_asset(release: &Release, platform_keywords: &[&str]) -> Option<usize> {
+    log::trace!(
+        "Checking for name with compatible os/arch pair from platform-derived list: {:?}",
+        platform_keywords
+    );
+    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
+    );
+    Some(asset_index)
+}
+
 /// Contains the current state of all of the tools that Foreman manages.
 #[derive(Debug, PartialEq, Serialize, Deserialize)]
 pub struct ToolCache {
@@ -109,11 +128,7 @@ impl ToolCache {
                     Version::parse(&release.tag_name[1..]).ok()
                 })?;
 
-                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())?;
 
                 Some((version, asset_index, release))
             })
@@ -223,8 +238,63 @@ 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, based on the release that surfaced it
+    #[test]
+    fn select_correct_asset() {
+        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, &["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 {
         use super::*;