[pbs-devel] [PATCH proxmox-backup 1/2] api types: version: add helper for min version checks

[pbs-devel] [PATCH proxmox-backup 1/2] api types: version: add helper for min version checks

[Christian Ebner]

Add a helper method to the ApiVersion type to reduce possible errors
when comparing api versions for feature compatibility checks.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 pbs-api-types/src/version.rs | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/pbs-api-types/src/version.rs b/pbs-api-types/src/version.rs
index 80f87e372..5f8efb663 100644
--- a/pbs-api-types/src/version.rs
+++ b/pbs-api-types/src/version.rs
@@ -1,4 +1,5 @@
 //! Defines the types for the api version info endpoint
+use std::cmp::Ordering;
 use std::convert::TryFrom;
 
 use anyhow::{format_err, Context};
@@ -68,3 +69,35 @@ impl TryFrom<ApiVersionInfo> for ApiVersion {
         })
     }
 }
+
+impl ApiVersion {
+    pub fn new(
+        major: ApiVersionMajor,
+        minor: ApiVersionMinor,
+        release: ApiVersionRelease,
+        repoid: String,
+    ) -> Self {
+        Self {
+            major,
+            minor,
+            release,
+            repoid,
+        }
+    }
+
+    pub fn is_min_required(&self, version: ApiVersion) -> bool {
+        match (
+            version.major.cmp(&self.major),
+            version.minor.cmp(&self.minor),
+            version.release.cmp(&self.release),
+        ) {
+            (Ordering::Less, _, _) => true,
+            (Ordering::Greater, _, _) => false,
+            (Ordering::Equal, Ordering::Less, _) => true,
+            (Ordering::Equal, Ordering::Greater, _) => false,
+            (Ordering::Equal, Ordering::Equal, Ordering::Less) => true,
+            (Ordering::Equal, Ordering::Equal, Ordering::Equal) => true,
+            (Ordering::Equal, Ordering::Equal, Ordering::Greater) => false,
+        }
+    }
+}
-- 
2.39.5



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

[pbs-devel] [PATCH proxmox-backup 2/2] sync: push: use min version helper for api compatibility checks

[Christian Ebner]

Use the compatibility check helper to reduce possible errors when
comparing api version.

No functional change intended.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 src/server/push.rs | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/src/server/push.rs b/src/server/push.rs
index 95c7c6bff..957eb1ab2 100644
--- a/src/server/push.rs
+++ b/src/server/push.rs
@@ -129,13 +129,12 @@ impl PushParameters {
         let api_version = ApiVersion::try_from(version_info)?;
 
         // push assumes namespace support on the remote side, fail early if missing
-        if api_version.major < 2 || (api_version.major == 2 && api_version.minor < 2) {
+        if !api_version.is_min_required(ApiVersion::new(2, 2, 0, String::new())) {
             bail!("Unsupported remote api version, minimum v2.2 required");
         }
 
-        let supports_prune_delete_stats = api_version.major > 3
-            || (api_version.major == 3 && api_version.minor >= 3)
-            || (api_version.major == 3 && api_version.minor == 2 && api_version.release >= 11);
+        let supports_prune_delete_stats =
+            api_version.is_min_required(ApiVersion::new(3, 2, 11, String::new()));
 
         let target = PushTarget {
             remote,
-- 
2.39.5



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

Re: [pbs-devel] [PATCH proxmox-backup 1/2] api types: version: add helper for min version checks

[Thomas Lamprecht]

Am 28.11.24 um 13:49 schrieb Christian Ebner:
> Add a helper method to the ApiVersion type to reduce possible errors
> when comparing api versions for feature compatibility checks.
> 
> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
> ---
>  pbs-api-types/src/version.rs | 33 +++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
> 
> diff --git a/pbs-api-types/src/version.rs b/pbs-api-types/src/version.rs
> index 80f87e372..5f8efb663 100644
> --- a/pbs-api-types/src/version.rs
> +++ b/pbs-api-types/src/version.rs
> @@ -1,4 +1,5 @@
>  //! Defines the types for the api version info endpoint
> +use std::cmp::Ordering;
>  use std::convert::TryFrom;
>  
>  use anyhow::{format_err, Context};
> @@ -68,3 +69,35 @@ impl TryFrom<ApiVersionInfo> for ApiVersion {
>          })
>      }
>  }
> +
> +impl ApiVersion {
> +    pub fn new(
> +        major: ApiVersionMajor,
> +        minor: ApiVersionMinor,
> +        release: ApiVersionRelease,
> +        repoid: String,
> +    ) -> Self {
> +        Self {
> +            major,
> +            minor,
> +            release,
> +            repoid,
> +        }
> +    }
> +
> +    pub fn is_min_required(&self, version: ApiVersion) -> bool {
> +        match (
> +            version.major.cmp(&self.major),
> +            version.minor.cmp(&self.minor),
> +            version.release.cmp(&self.release),
> +        ) {
> +            (Ordering::Less, _, _) => true,
> +            (Ordering::Greater, _, _) => false,
> +            (Ordering::Equal, Ordering::Less, _) => true,
> +            (Ordering::Equal, Ordering::Greater, _) => false,
> +            (Ordering::Equal, Ordering::Equal, Ordering::Less) => true,
> +            (Ordering::Equal, Ordering::Equal, Ordering::Equal) => true,
> +            (Ordering::Equal, Ordering::Equal, Ordering::Greater) => false,
> +        }
> +    }
> +}


Why not impl the Ord trait here instead?

Then the call-site could be

let supports_prune_delete_stats = api_version >= ApiVersion::new(3, 2, 11, String::new());

And maybe a separate type for the triple without the commit hash on which you
also impl the same and then avoid that slightly confusing String::new() hack.


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

Re: [pbs-devel] [PATCH proxmox-backup 1/2] api types: version: add helper for min version checks

[Christian Ebner]

On 11/28/24 14:10, Thomas Lamprecht wrote:
> Why not impl the Ord trait here instead?
> 
> Then the call-site could be
> 
> let supports_prune_delete_stats = api_version >= ApiVersion::new(3, 2, 11, String::new());

Ah, yes that's way nicer and allows also for exact version matching.

> And maybe a separate type for the triple without the commit hash on which you
> also impl the same and then avoid that slightly confusing String::new() hack.

Acked, will send a new version incorporating your feedback, thanks!



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

Re: [pbs-devel] [PATCH proxmox-backup 1/2] api types: version: add helper for min version checks

[Thomas Lamprecht]

Am 28.11.24 um 14:14 schrieb Christian Ebner:
> On 11/28/24 14:10, Thomas Lamprecht wrote:
>> Why not impl the Ord trait here instead?
>>
>> Then the call-site could be
>>
>> let supports_prune_delete_stats = api_version >= ApiVersion::new(3, 2, 11, String::new());
> 
> Ah, yes that's way nicer and allows also for exact version matching.
> 
>> And maybe a separate type for the triple without the commit hash on which you
>> also impl the same and then avoid that slightly confusing String::new() hack.
> 
> Acked, will send a new version incorporating your feedback, thanks!
> 

Note that while I'm quite sure of the first thing the last thing was just a
idea from top of my head, not sure how much it improves,  but maybe having
a separate VersionTriple or SemanticVersion type might make a few things nicer
to use.


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

Re: [pbs-devel] [PATCH proxmox-backup 1/2] api types: version: add helper for min version checks

[Christian Ebner]

On 11/28/24 14:18, Thomas Lamprecht wrote:
> Am 28.11.24 um 14:14 schrieb Christian Ebner:
>> On 11/28/24 14:10, Thomas Lamprecht wrote:
>>> Why not impl the Ord trait here instead?
>>>
>>> Then the call-site could be
>>>
>>> let supports_prune_delete_stats = api_version >= ApiVersion::new(3, 2, 11, String::new());
>>
>> Ah, yes that's way nicer and allows also for exact version matching.
>>
>>> And maybe a separate type for the triple without the commit hash on which you
>>> also impl the same and then avoid that slightly confusing String::new() hack.
>>
>> Acked, will send a new version incorporating your feedback, thanks!
>>
> 
> Note that while I'm quite sure of the first thing the last thing was just a
> idea from top of my head, not sure how much it improves,  but maybe having
> a separate VersionTriple or SemanticVersion type might make a few things nicer
> to use.

Okay, well I was rather opting for dropping the `repoid` from 
`ApiVersion` instead of introducing another type, as that is currently 
not used and was just included for completeness. The `repoid` can 
already be obtained directly from the `ApiVersionInfo`, returned by the 
version api endpoint so this information is redundant anyways.


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

Re: [pbs-devel] [PATCH proxmox-backup 1/2] api types: version: add helper for min version checks

[Thomas Lamprecht]

Am 28.11.24 um 14:49 schrieb Christian Ebner:
> On 11/28/24 14:18, Thomas Lamprecht wrote:
>> Am 28.11.24 um 14:14 schrieb Christian Ebner:
>>> On 11/28/24 14:10, Thomas Lamprecht wrote:
>>>> Why not impl the Ord trait here instead?
>>>>
>>>> Then the call-site could be
>>>>
>>>> let supports_prune_delete_stats = api_version >= ApiVersion::new(3, 2, 11, String::new());
>>>
>>> Ah, yes that's way nicer and allows also for exact version matching.
>>>
>>>> And maybe a separate type for the triple without the commit hash on which you
>>>> also impl the same and then avoid that slightly confusing String::new() hack.
>>>
>>> Acked, will send a new version incorporating your feedback, thanks!
>>>
>>
>> Note that while I'm quite sure of the first thing the last thing was just a
>> idea from top of my head, not sure how much it improves,  but maybe having
>> a separate VersionTriple or SemanticVersion type might make a few things nicer
>> to use.
> 
> Okay, well I was rather opting for dropping the `repoid` from 
> `ApiVersion` instead of introducing another type, as that is currently 
> not used and was just included for completeness. The `repoid` can 
> already be obtained directly from the `ApiVersionInfo`, returned by the 
> version api endpoint so this information is redundant anyways.

It sounds like you checked out surrounding code more closely, I did not,
so it's probably better to go with your idea for now.


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

Re: [pbs-devel] [PATCH proxmox-backup 1/2] api types: version: add helper for min version checks

[Christian Ebner]

superseded-by version 2:
https://lore.proxmox.com/pbs-devel/20241128160721.583578-1-c.ebner@proxmox.com/T/


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel