diff --git a/cmd/branchingconfigmanagers/tide-config-manager/main.go b/cmd/branchingconfigmanagers/tide-config-manager/main.go index 7e7ac595061..dae5199d96e 100644 --- a/cmd/branchingconfigmanagers/tide-config-manager/main.go +++ b/cmd/branchingconfigmanagers/tide-config-manager/main.go @@ -484,18 +484,20 @@ func (ve *verifiedEvent) ModifyQuery(q *prowconfig.TideQuery, repo string) { q.Labels = sets.List(reqLabels) } -// isVersionedBranch checks if a branch name matches release-4.x or openshift-4.x pattern +// isVersionedBranch checks if a branch name matches release-X.y or openshift-X.y pattern for modern versions (4.x+) func isVersionedBranch(branch string) bool { - if strings.HasPrefix(branch, "release-4.") { - versionPart := strings.TrimPrefix(branch, "release-4.") - if isValidMinorVersion(versionPart) { + // Check for release-X.y pattern (supporting any major version >= 4) + if strings.HasPrefix(branch, "release-") { + versionPart := strings.TrimPrefix(branch, "release-") + if isValidVersionString(versionPart) { return true } } - if strings.HasPrefix(branch, "openshift-4.") { - versionPart := strings.TrimPrefix(branch, "openshift-4.") - if isValidMinorVersion(versionPart) { + // Check for openshift-X.y pattern (supporting any major version >= 4) + if strings.HasPrefix(branch, "openshift-") { + versionPart := strings.TrimPrefix(branch, "openshift-") + if isValidVersionString(versionPart) { return true } } @@ -503,6 +505,39 @@ func isVersionedBranch(branch string) bool { return false } +// isValidVersionString checks if a version string is in X.y format where X >= 4 +func isValidVersionString(version string) bool { + parts := strings.Split(version, ".") + if len(parts) != 2 { + return false + } + + // Check if major version is a number >= 4 + majorStr, minorStr := parts[0], parts[1] + + // Validate major version + if len(majorStr) == 0 { + return false + } + for _, char := range majorStr { + if char < '0' || char > '9' { + return false + } + } + + // Convert to int and check if >= 4 + major := 0 + for _, char := range majorStr { + major = major*10 + int(char-'0') + } + if major < 4 { + return false + } + + // Validate minor version using existing function + return isValidMinorVersion(minorStr) +} + // isValidMinorVersion checks if a string represents a valid minor version (e.g., "9", "10", "15") func isValidMinorVersion(version string) bool { if version == "" { diff --git a/cmd/branchingconfigmanagers/tide-config-manager/main_test.go b/cmd/branchingconfigmanagers/tide-config-manager/main_test.go index 05ddc4849ba..90a8a9e556c 100644 --- a/cmd/branchingconfigmanagers/tide-config-manager/main_test.go +++ b/cmd/branchingconfigmanagers/tide-config-manager/main_test.go @@ -609,6 +609,36 @@ func TestIsVersionedBranch(t *testing.T) { branch: "openshift-4.15", expected: true, }, + { + name: "valid release-5.x branch", + branch: "release-5.1", + expected: true, + }, + { + name: "valid openshift-5.x branch", + branch: "openshift-5.2", + expected: true, + }, + { + name: "valid release-5.x branch with double digit minor", + branch: "release-5.10", + expected: true, + }, + { + name: "valid openshift-5.x branch with double digit minor", + branch: "openshift-5.15", + expected: true, + }, + { + name: "invalid release-5.x branch with version 0", + branch: "release-5.0", + expected: false, + }, + { + name: "invalid openshift-5.x branch with version 0", + branch: "openshift-5.0", + expected: false, + }, { name: "invalid release branch with version 0", branch: "release-4.0", @@ -674,6 +704,56 @@ func TestIsVersionedBranch(t *testing.T) { branch: "openshift-4.", expected: false, }, + { + name: "invalid release-5.x branch with non-numeric version", + branch: "release-5.abc", + expected: false, + }, + { + name: "invalid openshift-5.x branch with non-numeric version", + branch: "openshift-5.x", + expected: false, + }, + { + name: "release-5.x branch without version", + branch: "release-5.", + expected: false, + }, + { + name: "openshift-5.x branch without version", + branch: "openshift-5.", + expected: false, + }, + { + name: "valid release-6.x branch (future-proofing)", + branch: "release-6.1", + expected: true, + }, + { + name: "valid openshift-6.x branch (future-proofing)", + branch: "openshift-6.2", + expected: true, + }, + { + name: "valid release-10.x branch (future-proofing)", + branch: "release-10.5", + expected: true, + }, + { + name: "valid openshift-15.x branch (future-proofing)", + branch: "openshift-15.20", + expected: true, + }, + { + name: "invalid release-3.x branch (too old)", + branch: "release-3.11", + expected: false, + }, + { + name: "invalid openshift-3.x branch (too old)", + branch: "openshift-3.11", + expected: false, + }, } for _, tt := range tests { @@ -685,3 +765,150 @@ func TestIsVersionedBranch(t *testing.T) { }) } } + +func TestNewPreGeneralAvailability(t *testing.T) { + tests := []struct { + name string + current string + expected []string + }{ + { + name: "4.9 version", + current: "4.9", + expected: []string{"release-4.9", "openshift-4.9"}, + }, + { + name: "5.1 version", + current: "5.1", + expected: []string{"release-5.1", "openshift-5.1"}, + }, + { + name: "4.15 version", + current: "4.15", + expected: []string{"release-4.15", "openshift-4.15"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + delegate := newSharedDataDelegate() + event := newPreGeneralAvailability(tt.current, delegate) + + // Check that the event contains the expected branches + for _, expectedBranch := range tt.expected { + if !event.openshiftReleaseBranches.Has(expectedBranch) { + t.Errorf("Expected branch %s not found in openshiftReleaseBranches", expectedBranch) + } + } + + // Check that delegate is set + if event.sharedDataDelegate == nil { + t.Error("sharedDataDelegate should not be nil") + } + }) + } +} + +func TestPreGeneralAvailabilityEventModifyQuery(t *testing.T) { + delegate := newSharedDataDelegate() + event := newPreGeneralAvailability("4.9", delegate) + + tests := []struct { + name string + inputLabels []string + inputBranches []string + expectedLabels []string + repo string + }{ + { + name: "adds staff-eng-approved when backport-risk-assessed present for release branch", + inputLabels: []string{backportRiskAssessed}, + inputBranches: []string{"release-4.9"}, + expectedLabels: []string{backportRiskAssessed, staffEngApproved}, + repo: "openshift/dummy", + }, + { + name: "adds staff-eng-approved when backport-risk-assessed present for openshift branch", + inputLabels: []string{backportRiskAssessed}, + inputBranches: []string{"openshift-4.9"}, + expectedLabels: []string{backportRiskAssessed, staffEngApproved}, + repo: "openshift/dummy", + }, + { + name: "adds staff-eng-approved for multiple matching branches", + inputLabels: []string{backportRiskAssessed, "other-label"}, + inputBranches: []string{"release-4.9", "openshift-4.9"}, + expectedLabels: []string{backportRiskAssessed, "other-label", staffEngApproved}, + repo: "openshift/dummy", + }, + { + name: "no change when backport-risk-assessed not present", + inputLabels: []string{"other-label"}, + inputBranches: []string{"release-4.9"}, + expectedLabels: []string{"other-label"}, + repo: "openshift/dummy", + }, + { + name: "no change for non-matching branches", + inputLabels: []string{backportRiskAssessed}, + inputBranches: []string{"release-4.8", "main"}, + expectedLabels: []string{backportRiskAssessed}, + repo: "openshift/dummy", + }, + { + name: "no change when staff-eng-approved already present", + inputLabels: []string{backportRiskAssessed, staffEngApproved}, + inputBranches: []string{"release-4.9"}, + expectedLabels: []string{backportRiskAssessed, staffEngApproved}, + repo: "openshift/dummy", + }, + { + name: "handles empty branches", + inputLabels: []string{backportRiskAssessed}, + inputBranches: []string{}, + expectedLabels: []string{backportRiskAssessed}, + repo: "openshift/dummy", + }, + { + name: "handles mixed branches with some matching", + inputLabels: []string{backportRiskAssessed}, + inputBranches: []string{"main", "release-4.9", "release-4.8"}, + expectedLabels: []string{backportRiskAssessed, staffEngApproved}, + repo: "openshift/dummy", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + query := &prowconfig.TideQuery{ + Labels: tt.inputLabels, + IncludedBranches: tt.inputBranches, + } + + event.ModifyQuery(query, tt.repo) + + actualLabels := sets.New[string](query.Labels...) + expectedLabels := sets.New[string](tt.expectedLabels...) + + if !actualLabels.Equal(expectedLabels) { + t.Errorf("Expected labels: %v, got: %v", sets.List(expectedLabels), sets.List(actualLabels)) + } + }) + } +} + +func TestPreGeneralAvailabilityEventGetDataFromProwConfig(t *testing.T) { + delegate := newSharedDataDelegate() + event := newPreGeneralAvailability("4.9", delegate) + + // This method is currently empty, but we test that it doesn't panic + t.Run("does not panic", func(t *testing.T) { + defer func() { + if r := recover(); r != nil { + t.Errorf("GetDataFromProwConfig panicked: %v", r) + } + }() + + event.GetDataFromProwConfig(&prowconfig.ProwConfig{}) + }) +} diff --git a/pkg/api/leases.go b/pkg/api/leases.go index 2a1c42e3ef8..3029b666bfe 100644 --- a/pkg/api/leases.go +++ b/pkg/api/leases.go @@ -1,6 +1,7 @@ package api import ( + "fmt" "strconv" "strings" ) @@ -42,28 +43,65 @@ func IPPoolLeaseForTest(s *MultiStageTestConfigurationLiteral, metadata Metadata } const ( - openshiftBranch = "openshift-4." - releaseBranch = "release-4." - minimumVersion = 16 + minimumMajorVersion = 4 + minimumMinorVersion = 16 ) // Currently, we only have the ability to utilize IP pools in 4.16 and later, we want to make sure not to allocate them -// on earlier versions +// on earlier versions. 5.x and later versions are supported. func branchValidForIPPoolLease(branch string) bool { if branch == "master" || branch == "main" { return true } - var version string - if strings.HasPrefix(branch, openshiftBranch) { - version = strings.TrimPrefix(branch, openshiftBranch) + + // Check for openshift-X.Y or release-X.Y pattern + var majorVersion, minorVersion int + var err error + + if strings.HasPrefix(branch, "openshift-") { + versionPart := strings.TrimPrefix(branch, "openshift-") + if majorVersion, minorVersion, err = parseMajorMinorVersion(versionPart); err != nil { + return false + } + } else if strings.HasPrefix(branch, "release-") { + versionPart := strings.TrimPrefix(branch, "release-") + if majorVersion, minorVersion, err = parseMajorMinorVersion(versionPart); err != nil { + return false + } + } else { + return false + } + + // 5.x and later are supported + if majorVersion >= 5 { + return true + } + + // For 4.x, check minimum minor version + if majorVersion == 4 { + return minorVersion >= minimumMinorVersion } - if strings.HasPrefix(branch, releaseBranch) { - version = strings.TrimPrefix(branch, releaseBranch) + + // Earlier major versions not supported + return false +} + +// parseMajorMinorVersion parses a version string like "4.16" or "5.1" into major and minor components +func parseMajorMinorVersion(version string) (int, int, error) { + parts := strings.Split(version, ".") + if len(parts) != 2 { + return 0, 0, fmt.Errorf("invalid version format: %s", version) } - number, err := strconv.Atoi(version) + + major, err := strconv.Atoi(parts[0]) if err != nil { - return false + return 0, 0, fmt.Errorf("invalid major version: %s", parts[0]) + } + + minor, err := strconv.Atoi(parts[1]) + if err != nil { + return 0, 0, fmt.Errorf("invalid minor version: %s", parts[1]) } - return number >= minimumVersion + return major, minor, nil } diff --git a/pkg/api/metadata.go b/pkg/api/metadata.go index daeab83aea9..c82b3478202 100644 --- a/pkg/api/metadata.go +++ b/pkg/api/metadata.go @@ -111,14 +111,14 @@ func IsCiopConfigCM(name string) bool { } var releaseBranches = regexp.MustCompile(`^(release|enterprise|openshift)-([1-3])\.[0-9]+(?:\.[0-9]+)?$`) -var fourXBranches = regexp.MustCompile(`^(release|enterprise|openshift)-(4\.[0-9]+)$`) +var modernReleaseBranches = regexp.MustCompile(`^(release|enterprise|openshift)-((?:[4-9]|[1-9][0-9]+)\.[0-9]+)$`) func FlavorForBranch(branch string) string { var flavor string if branch == "master" || branch == "main" { flavor = branch - } else if m := fourXBranches.FindStringSubmatch(branch); m != nil { - flavor = m[2] // the 4.x release string + } else if m := modernReleaseBranches.FindStringSubmatch(branch); m != nil { + flavor = m[2] // the X.y release string (4.x, 5.x, 6.x, etc.) } else if m := releaseBranches.FindStringSubmatch(branch); m != nil { flavor = m[2] + ".x" } else { diff --git a/pkg/rehearse/jobs.go b/pkg/rehearse/jobs.go index 3fd3399a720..0765310b48e 100644 --- a/pkg/rehearse/jobs.go +++ b/pkg/rehearse/jobs.go @@ -53,20 +53,10 @@ const ( pjRehearse = "pj-rehearse" ) -// Number of openshift versions -var numVersion = 50 - // Global map that contains relevance of known branches var relevancy = map[string]int{ - "master": numVersion + 1, - "main": numVersion + 1, -} - -func init() { - for i := 1; i < numVersion; i++ { - relevancy[fmt.Sprintf("release-4.%d", i)] = i - relevancy[fmt.Sprintf("openshift-4.%d", i)] = i - } + "master": 50000, // Ensure master/main always has highest priority + "main": 50000, } // NewProwJobClient creates a ProwJob client with a dry run capability @@ -756,9 +746,43 @@ func SelectJobsForChangedRegistry(regSteps []registry.Node, allPresubmits presub return selectedPresubmits, selectedPeriodics } +// calculateRelevancy returns the relevancy score for a branch name +func calculateRelevancy(branch string) int { + // Check static entries first (master/main) + if score, exists := relevancy[branch]; exists { + return score + } + + // Parse release-X.Y or openshift-X.Y patterns + var versionPart string + if strings.HasPrefix(branch, "release-") { + versionPart = strings.TrimPrefix(branch, "release-") + } else if strings.HasPrefix(branch, "openshift-") { + versionPart = strings.TrimPrefix(branch, "openshift-") + } else { + return 0 // Unknown branch format gets lowest priority + } + + // Parse major.minor version + parts := strings.Split(versionPart, ".") + if len(parts) != 2 { + return 0 + } + + major, err1 := strconv.Atoi(parts[0]) + minor, err2 := strconv.Atoi(parts[1]) + if err1 != nil || err2 != nil || major < 4 || minor < 1 { + return 0 + } + + // Calculate relevancy: major version gets 1000 points per version, minor gets 1 point + // 4.1 = 100+1 = 101, 4.10 = 100+10 = 110, 5.1 = 1100+1 = 1101, etc. + return (major-4)*1000 + 100 + minor +} + // Compare two branches by their relevancy func moreRelevant(one, two *config.DataWithInfo) bool { - return relevancy[one.Info.Metadata.Branch] > relevancy[two.Info.Metadata.Branch] + return calculateRelevancy(one.Info.Metadata.Branch) > calculateRelevancy(two.Info.Metadata.Branch) } func getClusterTypes(jobs map[string][]prowconfig.Presubmit) []string {