Re: [pbs-devel] [PATCH proxmox-offline-mirror 4/6] helper: improve handling of multiple keys when activating them

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

On November 9, 2023 4:34 pm, Stefan Sterz wrote:
> [..]
> 
>      let server_id = proxmox_subscription::get_hardware_address()?;
> -    let subscription = state.subscriptions.iter().find(|s| {
> -        if let Some(key) = s.key.as_ref() {
> -            if let Ok(found_product) = key[..3].parse::<ProductType>() {
> -                return product == found_product;
> -            }
> +
> +    let mut subscriptions: Vec<(ProductType, &SubscriptionInfo)> = state
> +        .subscriptions
> +        .iter()
> +        .filter(|sub| sub.serverid.as_ref() == Some(&server_id))
> +        .filter_map(|sub| sub.get_product_type().ok().map(|prod| (prod, sub)))
> +        .filter(|(found_product, _)| {
> +            (product.is_none() || Some(found_product) == product.as_ref())
> +                && found_product != &ProductType::Pom
> +        })
> +        .collect();
> +
> +    if subscriptions.is_empty() {
> +        bail!("No matching subscription key found for server ID '{server_id}'");
> +    }
> +
> +    subscriptions.sort_by(|(prod_a, sub_a), (prod_b, sub_b)| {
> +        if prod_a == prod_b {
> +            return sub_b
> +                .get_next_due_date()
> +                .ok()
> +                .cmp(&sub_a.get_next_due_date().ok());
>          }
> -        false
> +
> +        prod_a.cmp(prod_b)
>      });
> 
> -    match subscription {
> -        Some(subscription) => {
> -            eprintln!("Setting offline subscription key for {product}..");
> -            match set_subscription_key(product, subscription) {
> -                Ok(output) if !output.is_empty() => eprintln!("success: {output}"),
> -                Ok(_) => eprintln!("success."),
> -                Err(err) => eprintln!("error: {err}"),
> -            }
> -            Ok(())
> +    subscriptions.dedup_by(|(prod_a, _), (prod_b, _)| prod_a == prod_b);

this is a bit opaque from a readability standpoint, would something like
this as follow-up be agreeable for you? it makes the whole "try to find
the single key per product" a bit more explicit.

diff --git a/src/bin/proxmox-offline-mirror-helper.rs b/src/bin/proxmox-offline-mirror-helper.rs
index a231e84..def10ad 100644
--- a/src/bin/proxmox-offline-mirror-helper.rs
+++ b/src/bin/proxmox-offline-mirror-helper.rs
@@ -303,7 +303,7 @@ async fn setup_offline_key(
 
     let server_id = proxmox_subscription::get_hardware_address()?;
 
-    let mut subscriptions: Vec<(ProductType, &SubscriptionInfo)> = state
+    let subscriptions: HashMap<ProductType, &SubscriptionInfo> = state
         .subscriptions
         .iter()
         .filter(|sub| sub.serverid.as_ref() == Some(&server_id))
@@ -312,25 +312,21 @@ async fn setup_offline_key(
             (product.is_none() || Some(found_product) == product.as_ref())
                 && found_product != &ProductType::Pom
         })
-        .collect();
+        .fold(HashMap::new(), |mut map, (found_product, sub)| {
+            if let Some(existing) = map.get(&found_product) {
+                if existing.get_next_due_date().ok() < sub.get_next_due_date().ok() {
+                    map.insert(found_product, sub);
+                }
+            } else {
+                map.insert(found_product, sub);
+            }
+            map
+        });
 
     if subscriptions.is_empty() {
         bail!("No matching subscription key found for server ID '{server_id}'");
     }
 
-    subscriptions.sort_by(|(prod_a, sub_a), (prod_b, sub_b)| {
-        if prod_a == prod_b {
-            return sub_b
-                .get_next_due_date()
-                .ok()
-                .cmp(&sub_a.get_next_due_date().ok());
-        }
-
-        prod_a.cmp(prod_b)
-    });
-
-    subscriptions.dedup_by(|(prod_a, _), (prod_b, _)| prod_a == prod_b);
-
     for (product, subscription) in subscriptions {
         eprintln!("Setting offline subscription key for {product}...");
         match set_subscription_key(&product, subscription) {

> +
> +    for (product, subscription) in subscriptions {
> +        eprintln!("Setting offline subscription key for {product}...");
> +        match set_subscription_key(&product, subscription) {
> +            Ok(output) if !output.is_empty() => eprintln!("success: {output}"),
> +            Ok(_) => eprintln!("success."),
> +            Err(err) => eprintln!("error: {err}"),
>          }
> -        None => bail!("No matching subscription key found for product '{product}' and server ID '{server_id}'"),
>      }
> +
> +    Ok(())
>  }
> 
>  #[api(
> --
> 2.39.2