Skip to content

branchcuts: add bumping of release gating jobs from sippy config #4854

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Prucek wants to merge 2 commits into
base: main
Choose a base branch
from
Open

branchcuts: add bumping of release gating jobs from sippy config #4854

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4605cf5
branchcuts: add bumping of release gating jobs from sippy config
Prucek Nov 25, 2025
0e57c9d
bumper: add env vars bumping
Prucek Nov 26, 2025
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
103 changes: 96 additions & 7 deletions pkg/branchcuts/bumper/gen-release-jobs-bumper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ type GeneratedReleaseGatingJobsBumper struct {
getFilesRegexp *regexp.Regexp
jobsDir string
newIntervalValue int
sippyConfigPath string // could be fetched from https://github.com/openshift/sippy/blob/main/config/openshift.yaml
useSippyConfig bool
}

var _ Bumper[*cioperatorcfg.DataWithInfo] = &GeneratedReleaseGatingJobsBumper{}
Expand All @@ -38,14 +40,40 @@ func NewGeneratedReleaseGatingJobsBumper(ocpVer, jobsDir string, newIntervalValu
mmRegexp := fmt.Sprintf("%d\\.%d", mm.Major, mm.Minor)
getFilesRegexp := regexp.MustCompile(fmt.Sprintf(releaseJobsRegexPatternFormat, mmRegexp))
return &GeneratedReleaseGatingJobsBumper{
mm,
getFilesRegexp,
jobsDir,
newIntervalValue,
mm: mm,
getFilesRegexp: getFilesRegexp,
jobsDir: jobsDir,
newIntervalValue: newIntervalValue,
useSippyConfig: false,
}, nil
}

// NewGeneratedReleaseGatingJobsBumperWithSippy creates a bumper that uses Sippy config to determine which files to bump
func NewGeneratedReleaseGatingJobsBumperWithSippy(ocpVer, jobsDir, sippyConfigPath string, newIntervalValue int) (*GeneratedReleaseGatingJobsBumper, error) {
mm, err := ocplifecycle.ParseMajorMinor(ocpVer)
if err != nil {
return nil, fmt.Errorf("parse release: %w", err)
}
mmRegexp := fmt.Sprintf("%d\\.%d", mm.Major, mm.Minor)
getFilesRegexp := regexp.MustCompile(fmt.Sprintf(releaseJobsRegexPatternFormat, mmRegexp))
return &GeneratedReleaseGatingJobsBumper{
mm: mm,
getFilesRegexp: getFilesRegexp,
jobsDir: jobsDir,
newIntervalValue: newIntervalValue,
sippyConfigPath: sippyConfigPath,
useSippyConfig: true,
}, nil
}

func (b *GeneratedReleaseGatingJobsBumper) GetFiles() ([]string, error) {
if b.useSippyConfig {
return b.getFilesFromSippy()
}
return b.getFilesFromRegex()
}

func (b *GeneratedReleaseGatingJobsBumper) getFilesFromRegex() ([]string, error) {
files := make([]string, 0)
err := filepath.Walk(b.jobsDir, func(path string, info fs.FileInfo, err error) error {
if b.getFilesRegexp.Match([]byte(path)) {
Expand All @@ -56,6 +84,25 @@ func (b *GeneratedReleaseGatingJobsBumper) GetFiles() ([]string, error) {
return files, err
}

func (b *GeneratedReleaseGatingJobsBumper) getFilesFromSippy() ([]string, error) {
releaseVersion := fmt.Sprintf("%d.%d", b.mm.Major, b.mm.Minor)
configRootDir := filepath.Dir(filepath.Dir(b.jobsDir))

configFiles, err := GetConfigFilesForReleaseFromSippy(b.sippyConfigPath, releaseVersion, configRootDir)
if err != nil {
logrus.WithError(err).Warnf("failed to get config files from Sippy for release %s, falling back to regex", releaseVersion)
return b.getFilesFromRegex()
}

if len(configFiles) == 0 {
logrus.Warnf("no config files found from Sippy for release %s, falling back to regex", releaseVersion)
return b.getFilesFromRegex()
}

logrus.Infof("found %d config files from Sippy for release %s", len(configFiles), releaseVersion)
return configFiles, nil
}
Comment on lines +87 to +104
Copy link

@coderabbitai coderabbitai bot Nov 25, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

Potential path resolution issue.

Line 89 computes configRootDir by going up two directories from jobsDir. If jobsDir is /repo/ci-operator/jobs, then configRootDir becomes /repo. However, GetConfigFilesForReleaseFromSippy and GetConfigFilesFromSippyJobs expect configDir to be the actual config directory (e.g., /repo/ci-operator/config).

Inside GetConfigFilesFromSippyJobs (sippy.go line 167), it computes jobsRootDir = filepath.Join(filepath.Dir(configDir), "jobs"). If configDir is /repo, this yields /jobs instead of /repo/ci-operator/jobs.

Consider fixing the path:

 func (b *GeneratedReleaseGatingJobsBumper) getFilesFromSippy() ([]string, error) {
 	releaseVersion := fmt.Sprintf("%d.%d", b.mm.Major, b.mm.Minor)
-	configRootDir := filepath.Dir(filepath.Dir(b.jobsDir))
+	configRootDir := filepath.Join(filepath.Dir(b.jobsDir), "config")
 
 	configFiles, err := GetConfigFilesForReleaseFromSippy(b.sippyConfigPath, releaseVersion, configRootDir)

🏁 Script executed:

#!/bin/bash
# Verify the directory structure assumptions in the codebase
# Check how jobsDir is typically structured

rg -n "jobsDir" pkg/branchcuts/bumper/ -A2 -B2

# Check the expected directory layout
ast-grep --pattern 'ci-operator/config'
ast-grep --pattern 'ci-operator/jobs'

Length of output: 7101

I need to verify the actual implementation in sippy.go to confirm the path resolution issue.

Let me check the actual sippy.go implementation to verify the path handling:

Now I need to examine the actual sippy.go implementation to verify the path logic:

🏁 Script executed:

#!/bin/bash
# Find and display sippy.go functions to verify the path calculation
rg -n "GetConfigFilesForReleaseFromSippy|GetConfigFilesFromSippyJobs" pkg/branchcuts/bumper/ -A20 | head -100

Length of output: 5211

Path calculation error causes incorrect job directory resolution.

Line 89 computes configRootDir by going up two directories from jobsDir, passing the repo root instead of the config directory path. At sippy.go line 167, GetConfigFilesFromSippyJobs computes jobsRootDir := filepath.Join(filepath.Dir(configDir), "jobs"). With configDir set to the repo root (e.g., /tmp/xyz), this yields /tmp/jobs instead of the correct /tmp/xyz/ci-operator/jobs, causing job metadata lookups to fail.

Fix the path derivation:

 func (b *GeneratedReleaseGatingJobsBumper) getFilesFromSippy() ([]string, error) {
 	releaseVersion := fmt.Sprintf("%d.%d", b.mm.Major, b.mm.Minor)
-	configRootDir := filepath.Dir(filepath.Dir(b.jobsDir))
+	configRootDir := filepath.Join(filepath.Dir(b.jobsDir), "config")
 
 	configFiles, err := GetConfigFilesForReleaseFromSippy(b.sippyConfigPath, releaseVersion, configRootDir)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
func (b *GeneratedReleaseGatingJobsBumper) getFilesFromSippy() ([]string, error) {
releaseVersion := fmt.Sprintf("%d.%d", b.mm.Major, b.mm.Minor)
configRootDir := filepath.Dir(filepath.Dir(b.jobsDir))
configFiles, err := GetConfigFilesForReleaseFromSippy(b.sippyConfigPath, releaseVersion, configRootDir)
if err != nil {
logrus.WithError(err).Warnf("failed to get config files from Sippy for release %s, falling back to regex", releaseVersion)
return b.getFilesFromRegex()
}
if len(configFiles) == 0 {
logrus.Warnf("no config files found from Sippy for release %s, falling back to regex", releaseVersion)
return b.getFilesFromRegex()
}
logrus.Infof("found %d config files from Sippy for release %s", len(configFiles), releaseVersion)
return configFiles, nil
}
func (b *GeneratedReleaseGatingJobsBumper) getFilesFromSippy() ([]string, error) {
releaseVersion := fmt.Sprintf("%d.%d", b.mm.Major, b.mm.Minor)
configRootDir := filepath.Join(filepath.Dir(b.jobsDir), "config")
configFiles, err := GetConfigFilesForReleaseFromSippy(b.sippyConfigPath, releaseVersion, configRootDir)
if err != nil {
logrus.WithError(err).Warnf("failed to get config files from Sippy for release %s, falling back to regex", releaseVersion)
return b.getFilesFromRegex()
}
if len(configFiles) == 0 {
logrus.Warnf("no config files found from Sippy for release %s, falling back to regex", releaseVersion)
return b.getFilesFromRegex()
}
logrus.Infof("found %d config files from Sippy for release %s", len(configFiles), releaseVersion)
return configFiles, nil
}
🤖 Prompt for AI Agents 
In pkg/branchcuts/bumper/gen-release-jobs-bumper.go around lines 87 to 104, the
code computes configRootDir by calling filepath.Dir(filepath.Dir(b.jobsDir))
which yields the repo root and causes GetConfigFilesForReleaseFromSippy to
resolve the wrong jobs path; change it to compute the config directory directly
(set configRootDir := filepath.Dir(b.jobsDir)) so it points at the ci-operator
config directory (e.g., /tmp/xyz/ci-operator) before calling
GetConfigFilesForReleaseFromSippy.


func (b *GeneratedReleaseGatingJobsBumper) Unmarshall(file string) (*cioperatorcfg.DataWithInfo, error) {
cfgDataByFilename, err := cioperatorcfg.LoadDataByFilename(file)
if err != nil {
Expand All @@ -79,6 +126,12 @@ func (b *GeneratedReleaseGatingJobsBumper) BumpFilename(
return "", err
}
dataWithInfo.Info.Metadata.Variant = newVariant
newBranch, err := ReplaceWithNextVersion(dataWithInfo.Info.Metadata.Branch, b.mm.Major)
if err != nil {
return "", err
}
dataWithInfo.Info.Metadata.Branch = newBranch

return dataWithInfo.Info.Metadata.Basename(), nil
}

Expand All @@ -102,10 +155,16 @@ func (b *GeneratedReleaseGatingJobsBumper) BumpContent(dataWithInfo *cioperatorc
return nil, err
}

// Bump variant in zz_generated_metadata
if err := ReplaceWithNextVersionInPlace(&config.Metadata.Variant, major); err != nil {
return nil, err
}

// Bump branch in zz_generated_metadata (e.g., release-4.21 -> release-4.22)
if err := ReplaceWithNextVersionInPlace(&config.Metadata.Branch, major); err != nil {
return nil, err
}

if config.Tests != nil {
for i := 0; i < len(config.Tests); i++ {
if config.Tests[i].Interval != nil {
Expand Down Expand Up @@ -178,6 +237,9 @@ func bumpTests(config *cioperatorapi.ReleaseBuildConfiguration, major int) error
continue
}

if err := bumpStepEnvVars(test.MultiStageTestConfiguration.Environment, major); err != nil {
return err
}
if err := bumpTestSteps(test.MultiStageTestConfiguration.Pre, major); err != nil {
return err
}
Expand All @@ -188,8 +250,10 @@ func bumpTests(config *cioperatorapi.ReleaseBuildConfiguration, major int) error
return err
}

if err := ReplaceWithNextVersionInPlace(test.MultiStageTestConfiguration.Workflow, major); err != nil {
return err
if test.MultiStageTestConfiguration.Workflow != nil {
if err := ReplaceWithNextVersionInPlace(test.MultiStageTestConfiguration.Workflow, major); err != nil {
return err
}
}

config.Tests[i] = test
Expand All @@ -198,6 +262,27 @@ func bumpTests(config *cioperatorapi.ReleaseBuildConfiguration, major int) error
return nil
}

func bumpStepEnvVars(env cioperatorapi.TestEnvironment, major int) error {
if env == nil {
return nil
}

for envKey := range env {
envValue := env[envKey]
mm, err := ocplifecycle.ParseMajorMinor(envValue)
if err != nil {
continue
Copy link
Contributor

@danilo-gemoli danilo-gemoli Nov 26, 2025

Choose a reason for hiding this comment

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

Use the logger and continue, do not swallow the error silently,

Copy link
Member Author

@Prucek Prucek Nov 26, 2025

Choose a reason for hiding this comment

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

with this I wanted to check if an env variable is parsable as major.minor then we can work with it, if not, I don't care about it. There is no error, it's just not a openshift version

}
if envKey == ocpReleaseEnvVarName || mm.Major == major {
if err := ReplaceWithNextVersionInPlace(&envValue, major); err != nil {
return err
}
env[envKey] = envValue
}
}
return nil
}

func bumpTestSteps(tests []cioperatorapi.TestStep, major int) error {
if tests == nil {
return nil
Expand All @@ -218,7 +303,11 @@ func bumpTestStepEnvVars(multistageTest cioperatorapi.TestStep, major int) error
}
for i := 0; i < len(multistageTest.Environment); i++ {
env := multistageTest.Environment[i]
if env.Name == ocpReleaseEnvVarName {
mm, err := ocplifecycle.ParseMajorMinor(*env.Default)
if err != nil {
continue
}
if env.Name == ocpReleaseEnvVarName || mm.Major == major {
if err := ReplaceWithNextVersionInPlace(env.Default, major); err != nil {
return err
}
Expand Down
16 changes: 16 additions & 0 deletions pkg/branchcuts/bumper/gen-release-jobs-bumper_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,10 @@ func TestBumpObject(t *testing.T) {
{
Interval: strRef("24h"),
MultiStageTestConfiguration: &cioperatorapi.MultiStageTestConfiguration{
Environment: cioperatorapi.TestEnvironment{
"OCP_VERSION": "4.10",
"Some_Other_VAR": "4.10",
},
Workflow: strRef("w-4.10-4.11"),
Test: []cioperatorapi.TestStep{
{
Expand All @@ -120,6 +124,10 @@ func TestBumpObject(t *testing.T) {
Name: "OCP_VERSION",
Default: strRef("4.10"),
},
{
Name: "Some_Other_VAR",
Default: strRef("4.10"),
},
},
},
},
Expand Down Expand Up @@ -163,6 +171,10 @@ func TestBumpObject(t *testing.T) {
{
Interval: strRef("168h"),
MultiStageTestConfiguration: &cioperatorapi.MultiStageTestConfiguration{
Environment: cioperatorapi.TestEnvironment{
"OCP_VERSION": "4.11",
"Some_Other_VAR": "4.11",
},
Workflow: strRef("w-4.11-4.12"),
Test: []cioperatorapi.TestStep{
{
Expand All @@ -172,6 +184,10 @@ func TestBumpObject(t *testing.T) {
Name: "OCP_VERSION",
Default: strRef("4.11"),
},
{
Name: "Some_Other_VAR",
Default: strRef("4.11"),
},
},
},
},
Expand Down
Loading