Add semver check for dev and beta version (#5757)

* Add semver check for dev and beta version Signed-off-by: pmahindrakar-oss <[email protected]> * add unsupported version tests Signed-off-by: pmahindrakar-oss <[email protected]> * passing context and refactored to table driven utests Signed-off-by: pmahindrakar-oss <[email protected]> --------- Signed-off-by: pmahindrakar-oss <[email protected]>
flyteorg · Sep 18, 2024 · 2d9c35b · 2d9c35b
1 parent fd841ba
commit 2d9c35b
Show file tree

Hide file tree

Showing 3 changed files with 134 additions and 52 deletions.
diff --git a/flytepropeller/pkg/controller/config/config.go b/flytepropeller/pkg/controller/config/config.go
@@ -36,6 +36,7 @@ package config
 import (
 	"context"
 	"fmt"
+	"regexp"
 	"time"
 
 	"github.com/Masterminds/semver"
@@ -133,6 +134,9 @@ var (
 			MaxSizeInMBForOffloading: 1000, // 1 GB is the default size before failing fast.
 		},
 	}
+
+	// This regex is used to sanitize semver versions passed to IsSupportedSDK checks for literal offloading feature.
+	sanitizeProtoRegex = regexp.MustCompile(`v?(\d+\.\d+\.\d+)`)
 )
 
 // Config that uses the flytestdlib Config module to generate commandline and load config files. This configuration is
@@ -187,18 +191,24 @@ type LiteralOffloadingConfig struct {
 }
 
 // IsSupportedSDKVersion returns true if the provided SDK and version are supported by the literal offloading config.
-func (l LiteralOffloadingConfig) IsSupportedSDKVersion(sdk string, versionString string) bool {
+func (l LiteralOffloadingConfig) IsSupportedSDKVersion(ctx context.Context, sdk string, versionString string) bool {
+	regexMatches := sanitizeProtoRegex.FindStringSubmatch(versionString)
+	if len(regexMatches) > 1 {
+		logger.Infof(ctx, "original: %s, semVer: %s", versionString, regexMatches[1])
+	} else {
+		logger.Warnf(ctx, "no match found for: %s", versionString)
+		return false
+	}
+	version, err := semver.NewVersion(regexMatches[1])
+	if err != nil {
+		logger.Warnf(ctx, "Failed to parse version %s", versionString)
+		return false
+	}
 	if leastSupportedVersion, ok := l.SupportedSDKVersions[sdk]; ok {
 		c, err := semver.NewConstraint(fmt.Sprintf(">= %s", leastSupportedVersion))
 		if err != nil {
 			// This should never happen
-			logger.Warnf(context.TODO(), "Failed to parse version constraint %s", leastSupportedVersion)
-			return false
-		}
-		version, err := semver.NewVersion(versionString)
-		if err != nil {
-			// This should never happen
-			logger.Warnf(context.TODO(), "Failed to parse version %s", versionString)
+			logger.Warnf(ctx, "Failed to parse version constraint %s", leastSupportedVersion)
 			return false
 		}
 		return c.Check(version)

diff --git a/flytepropeller/pkg/controller/config/config_test.go b/flytepropeller/pkg/controller/config/config_test.go
@@ -1,54 +1,126 @@
 package config
 
 import (
+	"context"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
 )
 
 func TestIsSupportedSDKVersion(t *testing.T) {
-	t.Run("supported version", func(t *testing.T) {
-		config := LiteralOffloadingConfig{
-			SupportedSDKVersions: map[string]string{
-				"flytekit": "0.16.0",
-			},
-		}
-		assert.True(t, config.IsSupportedSDKVersion("flytekit", "0.16.0"))
-	})
-
-	t.Run("unsupported version", func(t *testing.T) {
-		config := LiteralOffloadingConfig{
-			SupportedSDKVersions: map[string]string{
-				"flytekit": "0.16.0",
-			},
-		}
-		assert.False(t, config.IsSupportedSDKVersion("flytekit", "0.15.0"))
-	})
-
-	t.Run("unsupported SDK", func(t *testing.T) {
-		config := LiteralOffloadingConfig{
-			SupportedSDKVersions: map[string]string{
-				"flytekit": "0.16.0",
-			},
-		}
-		assert.False(t, config.IsSupportedSDKVersion("unknown", "0.16.0"))
-	})
-
-	t.Run("invalid version", func(t *testing.T) {
-		config := LiteralOffloadingConfig{
-			SupportedSDKVersions: map[string]string{
-				"flytekit": "0.16.0",
-			},
-		}
-		assert.False(t, config.IsSupportedSDKVersion("flytekit", "invalid"))
-	})
+	ctx := context.Background()
+	tests := []struct {
+		name           string
+		config         LiteralOffloadingConfig
+		sdk            string
+		version        string
+		expectedResult bool
+	}{
+		{
+			name: "supported version",
+			config: LiteralOffloadingConfig{
+				SupportedSDKVersions: map[string]string{
+					"flytekit": "0.16.0",
+				},
+			},
+			sdk:            "flytekit",
+			version:        "0.16.0",
+			expectedResult: true,
+		},
+		{
+			name: "unsupported version",
+			config: LiteralOffloadingConfig{
+				SupportedSDKVersions: map[string]string{
+					"flytekit": "0.16.0",
+				},
+			},
+			sdk:            "flytekit",
+			version:        "0.15.0",
+			expectedResult: false,
+		},
+		{
+			name: "unsupported SDK",
+			config: LiteralOffloadingConfig{
+				SupportedSDKVersions: map[string]string{
+					"flytekit": "0.16.0",
+				},
+			},
+			sdk:            "unknown",
+			version:        "0.16.0",
+			expectedResult: false,
+		},
+		{
+			name: "invalid version",
+			config: LiteralOffloadingConfig{
+				SupportedSDKVersions: map[string]string{
+					"flytekit": "0.16.0",
+				},
+			},
+			sdk:            "flytekit",
+			version:        "invalid",
+			expectedResult: false,
+		},
+		{
+			name: "invalid constraint",
+			config: LiteralOffloadingConfig{
+				SupportedSDKVersions: map[string]string{
+					"flytekit": "invalid",
+				},
+			},
+			sdk:            "flytekit",
+			version:        "0.16.0",
+			expectedResult: false,
+		},
+		{
+			name: "supported dev version",
+			config: LiteralOffloadingConfig{
+				SupportedSDKVersions: map[string]string{
+					"flytekit": "1.13.4",
+				},
+			},
+			sdk:            "flytekit",
+			version:        "1.13.4.dev12+g990b450ea.d20240917",
+			expectedResult: true,
+		},
+		{
+			name: "supported beta version",
+			config: LiteralOffloadingConfig{
+				SupportedSDKVersions: map[string]string{
+					"flytekit": "1.13.4",
+				},
+			},
+			sdk:            "flytekit",
+			version:        "v1.13.6b0",
+			expectedResult: true,
+		},
+		{
+			name: "unsupported dev version",
+			config: LiteralOffloadingConfig{
+				SupportedSDKVersions: map[string]string{
+					"flytekit": "1.13.4",
+				},
+			},
+			sdk:            "flytekit",
+			version:        "1.13.3.dev12+g990b450ea.d20240917",
+			expectedResult: false,
+		},
+		{
+			name: "unsupported beta version",
+			config: LiteralOffloadingConfig{
+				SupportedSDKVersions: map[string]string{
+					"flytekit": "1.13.4",
+				},
+			},
+			sdk:            "flytekit",
+			version:        "v1.13.3b0",
+			expectedResult: false,
+		},
+	}
 
-	t.Run("invalid constraint", func(t *testing.T) {
-		config := LiteralOffloadingConfig{
-			SupportedSDKVersions: map[string]string{
-				"flytekit": "invalid",
-			},
-		}
-		assert.False(t, config.IsSupportedSDKVersion("flytekit", "0.16.0"))
-	})
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			result := tt.config.IsSupportedSDKVersion(ctx, tt.sdk, tt.version)
+			assert.Equal(t, tt.expectedResult, result)
+		})
+	}
 }
diff --git a/flytepropeller/pkg/controller/nodes/common/utils.go b/flytepropeller/pkg/controller/nodes/common/utils.go
@@ -147,7 +147,7 @@ func CheckOffloadingCompat(ctx context.Context, nCtx interfaces.NodeExecutionCon
 		return &phaseInfo
 	}
 	runtimeData := taskNode.CoreTask().GetMetadata().GetRuntime()
-	if !literalOffloadingConfig.IsSupportedSDKVersion(runtimeData.GetType().String(), runtimeData.GetVersion()) {
+	if !literalOffloadingConfig.IsSupportedSDKVersion(ctx, runtimeData.GetType().String(), runtimeData.GetVersion()) {
 		if !literalOffloadingConfig.Enabled {
 			errMsg := fmt.Sprintf("task [%s] is trying to consume offloaded literals but feature is not enabled", taskID)
 			logger.Errorf(ctx, errMsg)