fix: support version compatibility check for pre-release versions #2069

KeranYang · 2024-09-17T22:06:38Z

TODO - once this PR gets approved, I will update the minNumaflowVersion in each SDK to add more guidelines on what version string to choose.

closes #2064

Signed-off-by: Keran Yang <[email protected]>

codecov · 2024-09-17T22:17:43Z

Codecov Report

Attention: Patch coverage is 97.60479% with 8 lines in your changes missing coverage. Please review.

Project coverage is 63.96%. Comparing base (6914341) to head (7d477ce).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
rust/monovertex/src/server_info.rs	98.13%	6 Missing ⚠️
pkg/sdkclient/serverinfo/types.go	75.00%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2069      +/-   ##
==========================================
+ Coverage   63.68%   63.96%   +0.28%     
==========================================
  Files         320      321       +1     
  Lines       29345    29572     +227     
==========================================
+ Hits        18687    18916     +229     
+ Misses       9641     9637       -4     
- Partials     1017     1019       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Keran Yang <[email protected]>

kohlisid · 2024-09-18T06:38:37Z

Hey @KeranYang
Thanks for making these changes! :D
Have you tested if we would we need similar changes in the Monovertex versioning as well? If yes, We can create an issue for that and do in a follow up PR.

KeranYang · 2024-09-18T14:49:30Z

@kohlisid thank you. I will make similar changes to mono vertex using this PR. Please stay tuned for new commits.

Signed-off-by: Keran Yang <[email protected]>

vigith

please do no use unwrap, it is like panic. use map_error instead and convert to Result, then short-circuit the return using ?.

vigith · 2024-09-19T00:54:06Z

rust/monovertex/src/server_info.rs

@@ -79,6 +79,39 @@ pub async fn check_for_server_compatibility(

 /// Checks if the given version meets the specified constraint.
 fn check_constraint(version: &Version, constraint: &str) -> error::Result<()> {
+    let binding = version.to_string();
+    // extract the major.minor.patch version
+    let mmp_version = binding.split('-').next().unwrap();


what if there is no -?

it returns the original string itself.

Signed-off-by: Keran Yang <[email protected]>

KeranYang · 2024-09-19T13:57:10Z

@kohlisid monovertex change added, please review. Thanks!

vigith · 2024-09-19T15:21:20Z

rust/monovertex/src/server_info.rs

        .map_err(|e| Error::ServerInfoError(format!("Error parsing Numaflow version: {}", e)))?;

    // Create a version constraint based on the minimum numaflow version
    let numaflow_constraint = format!(">={}", min_numaflow_version);
    check_constraint(&numaflow_version_semver, &numaflow_constraint).map_err(|e| {
        Error::ServerInfoError(format!(
            "numaflow version {} must be upgraded to at least {}, in order to work with current SDK version {}",
-            numaflow_version_semver, min_numaflow_version, e
+            numaflow_version_semver, human_readable(min_numaflow_version.to_string()), e


why are you calling to_string()? just implement Debug for Version via NewType.

@vigith , thanks for introducing to me the Debug trait. After learning a bit of it, I would like to keep my change as it is. My use case is to convert the version string used by constraint to a human-readable string. I don't want users to see "please update numaflow to 1.3.1-z", which is confusing, I want them to see "1-3-1".

This is a very simple string conversion. Using Debug will require me to create a new type which wraps a string and create object of that struct during compatibility check, which is extra code. Please let me know if my understanding is off.

isn't "human-readable string" all about Debug Trait is?

As discussed offline, I created a separate issue to track. #2076

vigith · 2024-09-19T15:21:54Z

rust/monovertex/src/server_info.rs

+// e.g., if the given version is "0.8.0rc100", human-readable version is "0.8.0".
+// if the given version is "0.8.0-z", "0.8.0".
+// if "0.8.0-rc1", "0.8.0-rc1".
+fn human_readable(ver: String) -> String {


impl Debug via the above asked NewType?

whynowy · 2024-09-19T15:27:21Z

pkg/sdkclient/serverinfo/types.go

+"0.8.0b1", "0.8.0b1" for python.
+2. The minimum supported version is a stable version.
+In this case, put (almost) the largest available pre-release version of the stable version in the map.
+E.g., if the minimum supported version is "0.8.0", then put "0.8.0-z" for java, go, rust, "0.8.0rc100" for python.


Curious, why using -z?

Yes, it's mainly because of the golang semver library "considering pre-releases to be invalid if the ranges does not include one". So we have to put a pre-release version in the constraint. To do so, we need to put the largest pre-release, hence -z and rc100 for python.

I will improve the docs accordingly.

Signed-off-by: Keran Yang <[email protected]>

) Signed-off-by: Keran Yang <[email protected]>

KeranYang added 2 commits September 17, 2024 18:06

fix: version compatibility check issue

44372d9

Signed-off-by: Keran Yang <[email protected]>

.

9267602

Signed-off-by: Keran Yang <[email protected]>

KeranYang changed the title ~~fix: version compatibility check issue~~ (WIP) fix: version compatibility check issue Sep 17, 2024

KeranYang added 2 commits September 17, 2024 18:59

fix logs

58e9170

Signed-off-by: Keran Yang <[email protected]>

.

02ec69a

Signed-off-by: Keran Yang <[email protected]>

KeranYang changed the title ~~(WIP) fix: version compatibility check issue~~ fix: version compatibility check issue Sep 17, 2024

KeranYang marked this pull request as ready for review September 17, 2024 23:27

KeranYang requested review from whynowy and vigith as code owners September 17, 2024 23:27

KeranYang requested review from ayildirim21, kohlisid and yhl25 September 17, 2024 23:27

KeranYang marked this pull request as draft September 18, 2024 18:25

KeranYang changed the title ~~fix: version compatibility check issue~~ (WIP) fix: version compatibility check issue Sep 18, 2024

KeranYang added 4 commits September 18, 2024 14:27

todo - test for rc as minimum - rust better logging

66046d7

Signed-off-by: Keran Yang <[email protected]>

human-readable log

3f0cd52

Signed-off-by: Keran Yang <[email protected]>

unit tests for pre-release

cd00108

Signed-off-by: Keran Yang <[email protected]>

clean up

057af8e

Signed-off-by: Keran Yang <[email protected]>

KeranYang changed the title ~~(WIP) fix: version compatibility check issue~~ fix: version compatibility check issue Sep 18, 2024

Merge branch 'main' into vcc

339bfbe

KeranYang changed the title ~~fix: version compatibility check issue~~ fix: support version compatibility check for pre-release versions Sep 18, 2024

KeranYang marked this pull request as ready for review September 18, 2024 23:46

.

5101e72

Signed-off-by: Keran Yang <[email protected]>

vigith reviewed Sep 19, 2024

View reviewed changes

address comments

fa302ba

Signed-off-by: Keran Yang <[email protected]>

KeranYang requested a review from vigith September 19, 2024 13:19

Merge branch 'main' into vcc

9bae235

nats

46b0188

Signed-off-by: Keran Yang <[email protected]>

vigith reviewed Sep 19, 2024

View reviewed changes

whynowy reviewed Sep 19, 2024

View reviewed changes

KeranYang added 2 commits September 19, 2024 15:25

.

558dc0a

Signed-off-by: Keran Yang <[email protected]>

.

a7cd007

Signed-off-by: Keran Yang <[email protected]>

KeranYang requested review from whynowy and vigith September 19, 2024 20:16

whynowy approved these changes Sep 19, 2024

View reviewed changes

vigith approved these changes Sep 19, 2024

View reviewed changes

Merge branch 'main' into vcc

7d477ce

KeranYang merged commit ed543ad into numaproj:main Sep 19, 2024
25 checks passed

KeranYang deleted the vcc branch September 19, 2024 21:06

whynowy pushed a commit that referenced this pull request Sep 26, 2024

fix: support version compatibility check for pre-release versions (#2069

9c2e8f8

) Signed-off-by: Keran Yang <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: support version compatibility check for pre-release versions #2069

fix: support version compatibility check for pre-release versions #2069

KeranYang commented Sep 17, 2024 •

edited

Loading

codecov bot commented Sep 17, 2024 •

edited

Loading

kohlisid commented Sep 18, 2024

KeranYang commented Sep 18, 2024

vigith left a comment •

edited

Loading

vigith Sep 19, 2024 •

edited

Loading

KeranYang Sep 19, 2024

KeranYang commented Sep 19, 2024 •

edited

Loading

vigith Sep 19, 2024

KeranYang Sep 19, 2024

vigith Sep 19, 2024

KeranYang Sep 19, 2024

vigith Sep 19, 2024

whynowy Sep 19, 2024

KeranYang Sep 19, 2024

fix: support version compatibility check for pre-release versions #2069

fix: support version compatibility check for pre-release versions #2069

Conversation

KeranYang commented Sep 17, 2024 • edited Loading

codecov bot commented Sep 17, 2024 • edited Loading

Codecov Report

kohlisid commented Sep 18, 2024

KeranYang commented Sep 18, 2024

vigith left a comment • edited Loading

Choose a reason for hiding this comment

vigith Sep 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KeranYang commented Sep 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KeranYang commented Sep 17, 2024 •

edited

Loading

codecov bot commented Sep 17, 2024 •

edited

Loading

vigith left a comment •

edited

Loading

vigith Sep 19, 2024 •

edited

Loading

KeranYang commented Sep 19, 2024 •

edited

Loading