Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 16 additions & 4 deletions cmd/branchingconfigmanagers/frequency-reducer/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -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-") {
Expand All @@ -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)
Expand All @@ -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)
Expand All @@ -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)
Expand All @@ -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)
Expand Down
43 changes: 14 additions & 29 deletions cmd/branchingconfigmanagers/tide-config-manager/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -484,40 +484,21 @@ 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
// where X is a major version (4 or higher) and Y is a minor version
func isVersionedBranch(branch string) bool {
if strings.HasPrefix(branch, "release-4.") {
versionPart := strings.TrimPrefix(branch, "release-4.")
if isValidMinorVersion(versionPart) {
return true
}
}

if strings.HasPrefix(branch, "openshift-4.") {
versionPart := strings.TrimPrefix(branch, "openshift-4.")
if isValidMinorVersion(versionPart) {
return true
prefixes := []string{"release-", "openshift-"}
for _, prefix := range prefixes {
if strings.HasPrefix(branch, prefix) {
versionPart := strings.TrimPrefix(branch, prefix)
if api.IsValidOCPVersion(versionPart) {
return true
}
}
}

return false
}

// isValidMinorVersion checks if a string represents a valid minor version (e.g., "9", "10", "15")
func isValidMinorVersion(version string) bool {
if version == "" {
return false
}

for _, char := range version {
if char < '0' || char > '9' {
return false
}
}

return version != "0"
}

func (ve *verifiedEvent) GetDataFromProwConfig(*prowconfig.ProwConfig) {
}

Expand Down Expand Up @@ -601,9 +582,13 @@ func reconcile(currentOCPVersion, lifecyclePhase string, config *prowconfig.Prow
)
}
if lifecyclePhase == GeneralAvailability {
pastVersion, pastErr := currentVersion.GetPastVersion()
if pastErr != nil {
return fmt.Errorf("failed to get past version for %s: %w", currentVersion.GetVersion(), pastErr)
}
_, err = shardprowconfig.ShardProwConfig(config, target,
newGeneralAvailabilityEvent(
currentVersion.GetPastVersion(),
pastVersion,
currentVersion.GetVersion(),
currentVersion.GetFutureVersion(),
repos),
Expand Down
33 changes: 29 additions & 4 deletions cmd/branchingconfigmanagers/tide-config-manager/main_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -610,14 +610,14 @@ func TestIsVersionedBranch(t *testing.T) {
expected: true,
},
{
name: "invalid release branch with version 0",
name: "valid release-4.0 branch",
branch: "release-4.0",
expected: false,
expected: true,
},
{
name: "invalid openshift branch with version 0",
name: "valid openshift-4.0 branch",
branch: "openshift-4.0",
expected: false,
expected: true,
},
{
name: "invalid release branch with non-numeric version",
Expand Down Expand Up @@ -674,6 +674,31 @@ func TestIsVersionedBranch(t *testing.T) {
branch: "openshift-4.",
expected: false,
},
{
name: "valid release-5.0 branch",
branch: "release-5.0",
expected: true,
},
{
name: "valid openshift-5.0 branch",
branch: "openshift-5.0",
expected: true,
},
{
name: "valid release-5.1 branch",
branch: "release-5.1",
expected: true,
},
{
name: "valid openshift-5.1 branch",
branch: "openshift-5.1",
expected: true,
},
{
name: "valid release-5.10 branch",
branch: "release-5.10",
expected: true,
},
}

for _, tt := range tests {
Expand Down
7 changes: 1 addition & 6 deletions cmd/ci-operator-config-mirror/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import (
"fmt"
"os"
"path/filepath"
"strings"

"github.com/sirupsen/logrus"

Expand Down Expand Up @@ -237,7 +236,7 @@ func privateBuildRoot(buildRoot *api.BuildRootImageConfiguration) {

func privateBaseImages(baseImages map[string]api.ImageStreamTagReference) {
for name, reference := range baseImages {
if reference.Namespace == ocpNamespace && isIntegrationImageStream(reference.Name) {
if reference.Namespace == ocpNamespace && api.IsValidOCPVersion(reference.Name) {
reference.Name = fmt.Sprintf("%s-priv", reference.Name)
reference.Namespace = privatePromotionNamespace
baseImages[name] = reference
Expand Down Expand Up @@ -265,7 +264,3 @@ func privatePromotionConfiguration(promotion *api.PromotionConfiguration) {
func strP(str string) *string {
return &str
}

func isIntegrationImageStream(name string) bool {
return strings.HasPrefix(name, "4.")
}
4 changes: 2 additions & 2 deletions cmd/cvp-trigger/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -153,8 +153,8 @@ func (o options) validateOptions() error {
if o.ocpVersion == "" {
return fmt.Errorf("required parameter %s was not provided", ocpVersionOption)
}
if !strings.HasPrefix(o.ocpVersion, "4") {
return fmt.Errorf("ocp-version must be 4.x or higher")
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)")
}

if o.operatorPackageName == "" {
Expand Down
2 changes: 1 addition & 1 deletion cmd/ocp-build-data-enforcer/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ func gatherOptions() (*options, error) {
o := &options{PRCreationOptions: &prcreation.PRCreationOptions{}}
o.PRCreationOptions.AddFlags(flag.CommandLine)
flag.StringVar(&o.ocpBuildDataRepoDir, "ocp-build-data-repo-dir", "../ocp-build-data", "The directory in which the ocp-build-data repository is")
flag.StringVar(&o.majorMinor.Major, "major", "4", "The major version to target")
flag.StringVar(&o.majorMinor.Minor, "minor", "6", "The minor version to target")
Comment on lines 41 to 43
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Use the selected major/minor when choosing the upstream branch.

With the new --major/--minor flags, processDockerfile still fetches from the hard-coded release-4.6 (Line 115), so targeting 5.x+ will pull the wrong Dockerfile and produce incorrect diffs/PRs.

Proposed fix
@@
-	errGroup := &errgroup.Group{}
+	errGroup := &errgroup.Group{}
+	releaseBranch := fmt.Sprintf("release-%s", opts.majorMinor.String())
 	for idx := range configs {
 		idx := idx
 		errGroup.Go(func() error {
-			return processDockerfile(configs[idx], diffProcessor.addDiff)
+			return processDockerfile(configs[idx], diffProcessor.addDiff, releaseBranch)
 		})
 	}
@@
-func processDockerfile(config ocpbuilddata.OCPImageConfig, processor diffProcessorFunc) error {
+func processDockerfile(config ocpbuilddata.OCPImageConfig, processor diffProcessorFunc, releaseBranch string) error {
@@
-	getter := github.FileGetterFactory(config.PublicRepo.Org, config.PublicRepo.Repo, "release-4.6")
+	getter := github.FileGetterFactory(config.PublicRepo.Org, config.PublicRepo.Repo, releaseBranch)

Also applies to: 90-94, 109-116

🤖 Prompt for AI Agents
In `@cmd/ocp-build-data-enforcer/main.go` around lines 41 - 43, processDockerfile
and other upstream-branch selections are still hard-coded to "release-4.6";
update the logic that chooses the upstream branch to use the configured flags
(o.majorMinor.Major and o.majorMinor.Minor) instead of the literal "4" and "6".
Locate where the branch string is constructed (references: processDockerfile and
the three other branch-selection sites mentioned) and replace the hard-coded
value with a formatted branch name built from o.majorMinor.Major and
o.majorMinor.Minor (e.g. "release-<major>.<minor>") so fetching and diffs use
the selected version.

flag.BoolVar(&o.createPRs, "create-prs", false, "If the tool should create PRs")
flag.IntVar(&o.prCreationCeiling, "pr-creation-ceiling", 5, "The maximum number of PRs to upsert")
Expand All @@ -61,7 +62,6 @@ func main() {
if err != nil {
logrus.WithError(err).Fatal("Failed to gather options")
}
opts.majorMinor.Major = "4"

configs, err := ocpbuilddata.LoadImageConfigs(opts.ocpBuildDataRepoDir, opts.majorMinor)
if err != nil {
Expand Down
7 changes: 5 additions & 2 deletions cmd/registry-replacer/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ func gatherOptions() (*options, error) {
flag.Var(o.ensureCorrectPromotionDockerfileIngoredRepos, "ensure-correct-promotion-dockerfile-ignored-repos", "Repos that are being ignored when ensuring the correct promotion dockerfile in org/repo notation. Can be passed multiple times.")
flag.IntVar(&o.maxConcurrency, "concurrency", 500, "Maximum number of concurrent in-flight goroutines to handle files.")
flag.StringVar(&o.ocpBuildDataRepoDir, "ocp-build-data-repo-dir", "../ocp-build-data", "The directory in which the ocp-build-data repository is")
flag.StringVar(&o.currentRelease.Major, "current-release-major", "4", "The major version of the current release that is getting forwarded to from the master branch")
flag.StringVar(&o.currentRelease.Minor, "current-release-minor", "6", "The minor version of the current release that is getting forwarded to from the master branch")
flag.BoolVar(&o.pruneUnusedReplacements, "prune-unused-replacements", false, "If replacements that match nothing should get pruned from the config. Note that if --apply-replacements is set to false pruning will not take place.")
flag.BoolVar(&o.pruneUnusedBaseImages, "prune-unused-base-images", false, "If base images that match nothing should get pruned from the config")
Expand All @@ -90,9 +91,11 @@ func gatherOptions() (*options, error) {
errs = append(errs, errors.New("--ocp-build-data-repo-dir must be set when --ensure-correct-promotion-dockerfile is set"))
}
if o.currentRelease.Minor == "" {
errs = append(errs, errors.New("--current-release must be set when --ensure-correct-promotion-dockerfile is set"))
errs = append(errs, errors.New("--current-release-minor must be set when --ensure-correct-promotion-dockerfile is set"))
}
if o.currentRelease.Major == "" {
errs = append(errs, errors.New("--current-release-major must be set when --ensure-correct-promotion-dockerfile is set"))
}
o.currentRelease.Major = "4"
}

return o, utilerrors.NewAggregate(errs)
Expand Down
51 changes: 38 additions & 13 deletions pkg/api/leases.go
Original file line number Diff line number Diff line change
Expand Up @@ -42,9 +42,10 @@ func IPPoolLeaseForTest(s *MultiStageTestConfigurationLiteral, metadata Metadata
}

const (
openshiftBranch = "openshift-4."
releaseBranch = "release-4."
minimumVersion = 16
// minimumIPPoolMajorVersion and minimumIPPoolMinorVersion define the minimum OCP version
// that supports IP pools (4.16+)
minimumIPPoolMajorVersion = 4
minimumIPPoolMinorVersion = 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
Expand All @@ -53,17 +54,41 @@ 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)
}
if strings.HasPrefix(branch, releaseBranch) {
version = strings.TrimPrefix(branch, releaseBranch)
}
number, err := strconv.Atoi(version)
if err != nil {

major, minor, ok := parseVersionFromBranch(branch)
if !ok {
return false
}

return number >= minimumVersion
// Version 5.x and later is always valid
if major > minimumIPPoolMajorVersion {
return true
}
// For version 4.x, check if minor version meets minimum requirement
return major == minimumIPPoolMajorVersion && minor >= minimumIPPoolMinorVersion
}

// parseVersionFromBranch extracts major and minor version from branch names like
// "openshift-4.16", "release-5.0", etc.
func parseVersionFromBranch(branch string) (major, minor int, ok bool) {
prefixes := []string{"openshift-", "release-"}
for _, prefix := range prefixes {
if strings.HasPrefix(branch, prefix) {
versionStr := strings.TrimPrefix(branch, prefix)
parts := strings.Split(versionStr, ".")
if len(parts) != 2 {
continue
}
majorNum, err := strconv.Atoi(parts[0])
if err != nil {
continue
}
minorNum, err := strconv.Atoi(parts[1])
if err != nil {
continue
}
return majorNum, minorNum, true
}
}
return 0, 0, false
}
30 changes: 30 additions & 0 deletions pkg/api/leases_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,36 @@ func TestIPPoolLeaseForTest(t *testing.T) {
tests: MultiStageTestConfigurationLiteral{ClusterProfile: ClusterProfileAWS},
metadata: Metadata{Branch: "release-4.10"},
},
{
name: "aws, with 5.0 branch (should be valid)",
tests: MultiStageTestConfigurationLiteral{ClusterProfile: ClusterProfileAWS},
metadata: Metadata{Branch: "release-5.0"},
expected: StepLease{
ResourceType: "aws-ip-pools",
Env: DefaultIPPoolLeaseEnv,
Count: 13,
},
},
{
name: "aws, with openshift-5.0 branch (should be valid)",
tests: MultiStageTestConfigurationLiteral{ClusterProfile: ClusterProfileAWS},
metadata: Metadata{Branch: "openshift-5.0"},
expected: StepLease{
ResourceType: "aws-ip-pools",
Env: DefaultIPPoolLeaseEnv,
Count: 13,
},
},
{
name: "aws, with 5.1 branch (should be valid)",
tests: MultiStageTestConfigurationLiteral{ClusterProfile: ClusterProfileAWS},
metadata: Metadata{Branch: "release-5.1"},
expected: StepLease{
ResourceType: "aws-ip-pools",
Env: DefaultIPPoolLeaseEnv,
Count: 13,
},
},
}
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
Expand Down
6 changes: 3 additions & 3 deletions pkg/api/metadata.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 fourXAndLaterBranches = regexp.MustCompile(`^(release|enterprise|openshift)-([4-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 := fourXAndLaterBranches.FindStringSubmatch(branch); m != nil {
flavor = m[2] // the 4.x, 5.x, etc. release string
} else if m := releaseBranches.FindStringSubmatch(branch); m != nil {
flavor = m[2] + ".x"
} else {
Expand Down
Loading