Updates after review: unification of isVersion fcns and error handling

Signed-off-by: Jakub Guzik <jguzik@redhat.com>

Author: jmguzik

cmd/branchingconfigmanagers/frequency-reducer/main.go

@@ -79,6 +79,18 @@ func updateIntervalFieldsForMatchedSteps(
 	if err != nil {
 		return
 	}
+
+	pastVersion, err := version.GetPastVersion()
+	if err != nil {
+		logrus.Warningf("Can't get past version for %s: %v", version.GetVersion(), err)
+		pastVersion = ""
+	}
+	pastPastVersion, err := version.GetPastPastVersion()
+	if err != nil {
+		logrus.Debugf("Can't get past-past version for %s: %v", version.GetVersion(), err)
+		pastPastVersion = ""
+	}
+
 	if configuration.Info.Metadata.Org == "openshift" || configuration.Info.Metadata.Org == "openshift-priv" {
 		for _, test := range configuration.Configuration.Tests {
 			if !strings.Contains(test.As, "mirror-nightly-image") && !strings.Contains(test.As, "promote-") {
@@ -93,7 +105,7 @@ func updateIntervalFieldsForMatchedSteps(
 						if !correctCron {
 							*test.Cron = generateMonthlyCron()
 						}
-					} else if testVersion.GetVersion() == version.GetPastPastVersion() {
+					} else if pastPastVersion != "" && testVersion.GetVersion() == pastPastVersion {
 						correctCron, err := isExecutedAtMostXTimesAMonth(*test.Cron, 2)
 						if err != nil {
 							logrus.Warningf("Can't parse cron string %s", *test.Cron)
@@ -102,7 +114,7 @@ func updateIntervalFieldsForMatchedSteps(
 						if !correctCron {
 							*test.Cron = generateBiWeeklyCron()
 						}
-					} else if testVersion.GetVersion() == version.GetPastVersion() {
+					} else if pastVersion != "" && testVersion.GetVersion() == pastVersion {
 						correctCron, err := isExecutedAtMostXTimesAMonth(*test.Cron, 4)
 						if err != nil {
 							logrus.Warningf("Can't parse cron string %s", *test.Cron)
@@ -125,7 +137,7 @@ func updateIntervalFieldsForMatchedSteps(
 							test.Cron = &cronExpr
 							test.Interval = nil
 						}
-					} else if testVersion.GetVersion() == version.GetPastPastVersion() {
+					} else if pastPastVersion != "" && testVersion.GetVersion() == pastPastVersion {
 						duration, err := time.ParseDuration(*test.Interval)
 						if err != nil {
 							logrus.Warningf("Can't parse interval string %s", *test.Cron)
@@ -136,7 +148,7 @@ func updateIntervalFieldsForMatchedSteps(
 							test.Cron = &cronExpr
 							test.Interval = nil
 						}
-					} else if testVersion.GetVersion() == version.GetPastVersion() {
+					} else if pastVersion != "" && testVersion.GetVersion() == pastVersion {
 						duration, err := time.ParseDuration(*test.Interval)
 						if err != nil {
 							logrus.Warningf("Can't parse interval string %s", *test.Cron)

cmd/branchingconfigmanagers/tide-config-manager/main.go

@@ -491,63 +491,14 @@ func isVersionedBranch(branch string) bool {
 	for _, prefix := range prefixes {
 		if strings.HasPrefix(branch, prefix) {
 			versionPart := strings.TrimPrefix(branch, prefix)
-			if isValidVersion(versionPart) {
+			if api.IsValidOCPVersion(versionPart) {
 				return true
 			}
 		}
 	}
 	return false
 }
 
-// isValidVersion checks if a string represents a valid version like "4.15", "5.0", etc.
-func isValidVersion(version string) bool {
-	parts := strings.Split(version, ".")
-	if len(parts) != 2 {
-		return false
-	}
-
-	// Validate major version (must be 4 or higher)
-	major := parts[0]
-	if major == "" {
-		return false
-	}
-	for _, char := range major {
-		if char < '0' || char > '9' {
-			return false
-		}
-	}
-	majorNum := 0
-	for _, char := range major {
-		majorNum = majorNum*10 + int(char-'0')
-	}
-	if majorNum < 4 {
-		return false
-	}
-
-	// Validate minor version
-	minor := parts[1]
-	if !isValidMinorVersion(minor) {
-		return false
-	}
-
-	return true
-}
-
-// isValidMinorVersion checks if a string represents a valid minor version (e.g., "0", "9", "10", "15")
-func isValidMinorVersion(version string) bool {
-	if version == "" {
-		return false
-	}
-
-	for _, char := range version {
-		if char < '0' || char > '9' {
-			return false
-		}
-	}
-
-	return true
-}
-
 func (ve *verifiedEvent) GetDataFromProwConfig(*prowconfig.ProwConfig) {
 }
 
@@ -631,9 +582,13 @@ func reconcile(currentOCPVersion, lifecyclePhase string, config *prowconfig.Prow
 		)
 	}
 	if lifecyclePhase == GeneralAvailability {
+		pastVersion, err := currentVersion.GetPastVersion()
+		if err != nil {
+			return fmt.Errorf("failed to get past version for %s: %w", currentVersion.GetVersion(), err)
+		}
 		_, err = shardprowconfig.ShardProwConfig(config, target,
 			newGeneralAvailabilityEvent(
-				currentVersion.GetPastVersion(),
+				pastVersion,
 				currentVersion.GetVersion(),
 				currentVersion.GetFutureVersion(),
 				repos),

cmd/cvp-trigger/main.go

@@ -8,7 +8,6 @@ import (
 	"os"
 	"path/filepath"
 	"sort"
-	"strconv"
 	"strings"
 	"time"
 
@@ -154,8 +153,7 @@ func (o options) validateOptions() error {
 	if o.ocpVersion == "" {
 		return fmt.Errorf("required parameter %s was not provided", ocpVersionOption)
 	}
-	// Validate ocp-version format (must be X.Y where X >= 4)
-	if !isValidOCPVersion(o.ocpVersion) {
+	if !api.IsValidOCPVersion(o.ocpVersion) {
 		return fmt.Errorf("ocp-version must be in format X.Y where X >= 4 (e.g., 4.15, 5.0)")
 	}
 
@@ -181,26 +179,6 @@ func (o options) validateOptions() error {
 	return nil
 }
 
-// isValidOCPVersion validates that the version is in format X.Y where X >= 4
-func isValidOCPVersion(version string) bool {
-	parts := strings.Split(version, ".")
-	if len(parts) != 2 {
-		return false
-	}
-
-	major, err := strconv.Atoi(parts[0])
-	if err != nil {
-		return false
-	}
-
-	_, err = strconv.Atoi(parts[1])
-	if err != nil {
-		return false
-	}
-
-	return major >= 4
-}
-
 // getPeriodicJob returns a Prow Job or an error if the provided
 // periodic job name is not found
 func getPeriodicJob(jobName string, config *prowconfig.Config) (*pjapi.ProwJob, error) {

pkg/api/ocplifecycle/ocplifecycle.go

@@ -211,24 +211,22 @@ func (m MajorMinor) Less(other MajorMinor) bool {
 }
 
 // GetPastVersion returns the previous version using VersionTransitionOverrides.
-func (m MajorMinor) GetPastVersion() string {
+// Returns an error if no previous version can be determined (e.g., missing override for X.0).
+func (m MajorMinor) GetPastVersion() (string, error) {
 	current := m.GetVersion()
-	prev, err := api.GetPreviousVersionSimple(current)
-	if err != nil {
-		return ""
-	}
-	return prev
+	return api.GetPreviousVersionSimple(current)
 }
 
 // GetPastPastVersion returns the version before the previous version.
-func (m MajorMinor) GetPastPastVersion() string {
-	pastVersion := m.GetPastVersion()
-	if pastVersion == "" {
-		return ""
+// Returns an error if any step in the chain fails.
+func (m MajorMinor) GetPastPastVersion() (string, error) {
+	pastVersion, err := m.GetPastVersion()
+	if err != nil {
+		return "", fmt.Errorf("failed to get past version: %w", err)
 	}
 	pastMM, err := ParseMajorMinor(pastVersion)
 	if err != nil {
-		return ""
+		return "", fmt.Errorf("failed to parse past version %s: %w", pastVersion, err)
 	}
 	return pastMM.GetPastVersion()
 }

pkg/api/version.go

@@ -42,6 +42,15 @@ func ParseVersion(version string) (ParsedVersion, error) {
 	return ParsedVersion{Major: major, Minor: minor}, nil
 }
 
+// IsValidOCPVersion validates that the version is in format X.Y where X >= 4.
+func IsValidOCPVersion(version string) bool {
+	parsed, err := ParseVersion(version)
+	if err != nil {
+		return false
+	}
+	return parsed.Major >= 4
+}
+
 // GetPreviousVersion returns the primary previous version. For X.0 without override,
 // it finds the highest (X-1).* from availableVersions.
 func GetPreviousVersion(current string, availableVersions []string) (string, error) {