DPTP-4435: branchcuts: find all release gating jobs

[petr-muller]

Find all release gating job configs using the same method used for determining which jobs will be consumed by TRT tooling. Code partially adapted from #4854.

When run over base state https://github.com/petr-muller/release/tree/dptp-4435-mock-prebranch-gating-jobs-final-base, it results in petr-muller/release@e8d717f over what the current tool creates in petr-muller/release@c8af926

---

branchcuts: bump versions in envvar values

Code adapted from #4854 by @Prucek

Results in the following changes over the previous commit: petr-muller/release@2822640

---

branchcuts: bump versions in base_images tags
Some configs import base_images from specified version-like tags like ci/tests-private:4.22. When tags are bare versions we can bumpt them too. We cannot bump tags where a version is a substring because cases like rhel-9-golang-1.24-openshift-4.22 cannot be handled by simply bumping the OCP version substring.

Results in the following changes over the previous commit: petr-muller/release@45a900a

---

branchcuts: ignore release gating jobs in openshift-priv

Results in the following changes over the previous commit: petr-muller/release@9c24588

---

branchcuts: ignore release gating jobs in non-variant configs
Nonvariant configs contain configuration that applies on the branch
directly and typically contains presubmits and postsubmits, not release
gating periodics.

It seems the only exception to this rule is this file (which is currently
missed by all other tooling):

ci-operator/config/shiftstack/ci/shiftstack-ci-release-4.22.yaml

Non-variant release-4.22 would typically be maintained by automation,
not manually, and TRT-facing jobs would be defined in a __periodics
file or something similar. If we can rename the config (which may be
non-trivial because job names carry meaning) the config may get picked
up by this tool again.

Results in the following changes over the previous commit: petr-muller/release@faca085

---

refactor: move ProvidesSignalForVersion to ocplifecycle

The method is no longer prowgen-internal, it is now a more general
ci-operator interrogation method that is used both for marking jobs for
TRT consumption (through job-release label) and on the other side for
recognizing which jobs would be marked as such, so they can be handled
by tooling e.g. involved in branchcuts.

[openshift-ci-robot]

Pipeline controller notification
This repo is configured to use the pipeline controller. Second-stage tests will be triggered either automatically or after lgtm label is added, depending on the repository configuration. The pipeline controller will automatically detect which contexts are required and will utilize /test Prow commands to trigger the second stage.

For optional jobs, comment /test ? to see a list of all defined jobs. To trigger manually all jobs from second stage use /pipeline required command.

This repository is configured in: automatic mode

[openshift-ci]

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

[openshift-ci-robot]

@petr-muller: This pull request references DPTP-4435 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set.

Details

In response to this:

Builds on #4881 and attempts to find all release gating job configs using the same method used for determining which jobs will be consumed by TRT tooling. Actual bump code taken from #4854 by @Prucek. Not polished yet, will polish after testing.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

[coderabbitai]

Note

Other AI code review bot(s) detected

CodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review.

Walkthrough

Pluggable file-finding strategies (regexp and signal) were introduced for generated release-gating job discovery; constructors and CLI now accept a fileFinder parameter; bump logic expanded (env vars, base images, metadata); tests extended; and a signal-providing helper was exported.

Changes

Cohort / File(s)	Change Summary
Command entry point cmd/branchingconfigmanagers/generated-release-gating-jobs/main.go	Removed releaseJobsPath; added file-finder CLI flag and fileFinder option field; pass fileFinder into NewGeneratedReleaseGatingJobsBumper.
Core bumper logic pkg/branchcuts/bumper/gen-release-jobs-bumper.go	Added pluggable file finders (FileFinderRegexp, FileFinderSignal) and getFilesFn; implemented makeGetFilesByRegexp and makeGetFilesProvidingSignal; constructor now accepts fileFinder; replaced direct fs-walk with chosen GetFiles; added BumpContent, changed BumpFilename signature, expanded bumping of env vars, base images, tests, and zz_generated_metadata; added error aggregation.
Bumper tests pkg/branchcuts/bumper/gen-release-jobs-bumper_test.go	Updated NewGeneratedReleaseGatingJobsBumper call sites to include fileFinder; added tests for bumpStepEnvVars, bumpTestStepEnvVars, and bumpBaseImages; adjusted expectations.
Prow/job helper pkg/prowgen/jobbase.go	Removed local providesSignalForVersion; now calls ocplifecycle.ProvidesSignalForVersion.
OCPLifecycle API pkg/api/ocplifecycle/ocplifecycle.go	Added exported ProvidesSignalForVersion(configSpec *cioperatorapi.ReleaseBuildConfiguration) string to return candidate version signal when present.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

✨ Finishing touches

• 📝 Generate docstrings

---

🧹 Recent nitpick comments

cmd/branchingconfigmanagers/generated-release-gating-jobs/main.go (1)

34-34: Consider adding validation for the file-finder flag.

The flag accepts any string, but only regexp and signal are valid values. While the bumper constructor will return an error for invalid values, adding explicit validation in gatherOptions() would provide earlier, clearer feedback.

♻️ Optional: Add validation

+	if o.fileFinder != bumper.FileFinderRegexp && o.fileFinder != bumper.FileFinderSignal {
+		errs = append(errs, fmt.Errorf("invalid file-finder: %s (must be 'regexp' or 'signal')", o.fileFinder))
+	}

---

📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting

📥 Commits

Reviewing files that changed from the base of the PR and between c05d952 and 0b8d4f8.

📒 Files selected for processing (5)

• cmd/branchingconfigmanagers/generated-release-gating-jobs/main.go
• pkg/api/ocplifecycle/ocplifecycle.go
• pkg/branchcuts/bumper/gen-release-jobs-bumper.go
• pkg/branchcuts/bumper/gen-release-jobs-bumper_test.go
• pkg/prowgen/jobbase.go

🚧 Files skipped from review as they are similar to previous changes (1)

• pkg/api/ocplifecycle/ocplifecycle.go

🧰 Additional context used

📓 Path-based instructions (1)

**

⚙️ CodeRabbit configuration file

-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.

Files:

• pkg/prowgen/jobbase.go
• cmd/branchingconfigmanagers/generated-release-gating-jobs/main.go
• pkg/branchcuts/bumper/gen-release-jobs-bumper.go
• pkg/branchcuts/bumper/gen-release-jobs-bumper_test.go

🧬 Code graph analysis (3)

pkg/prowgen/jobbase.go (1)

pkg/api/ocplifecycle/ocplifecycle.go (1)

• ProvidesSignalForVersion (262-267)

cmd/branchingconfigmanagers/generated-release-gating-jobs/main.go (1)

pkg/branchcuts/bumper/gen-release-jobs-bumper.go (1)

• NewGeneratedReleaseGatingJobsBumper (80-104)

pkg/branchcuts/bumper/gen-release-jobs-bumper.go (2)

pkg/api/ocplifecycle/ocplifecycle.go (3)

• MajorMinor (199-202)
• ProvidesSignalForVersion (262-267)
• ParseMajorMinor (237-252)

pkg/config/load.go (2)

• OperateOnCIOperatorConfigDir (327-329)
• DataWithInfo (415-418)

🔇 Additional comments (17)

pkg/branchcuts/bumper/gen-release-jobs-bumper.go (9)

14-18: LGTM!

Clean import organization with the new errors package from k8s.io/apimachinery/pkg/util/errors for error aggregation.

---

21-36: LGTM!

Good abstraction introducing getFilesFn type and file finder strategy constants. This enables clean pluggable file discovery.

---

54-76: LGTM!

Well-structured signal-based file finder with clear filtering logic. The comments explain the rationale for ignoring openshift-priv and non-variant configs.

---

80-104: LGTM!

Clean implementation of the strategy pattern for file finding. The switch handles both strategies and properly returns an error for unknown values.

---

125-141: LGTM!

Good addition of Branch bumping alongside Variant. The ignored filename parameter aligns with the updated API design.

---

143-183: LGTM!

Comprehensive content bumping covering base images, releases, tests, metadata, and intervals. Good error propagation throughout.

---

185-209: LGTM!

Error aggregation is now properly returned. The logic to only bump plain version tags (avoiding complex tags like rhel-9-golang-1.24-openshift-4.22) is well-reasoned and documented.

---

281-301: LGTM!

Clean implementation of env var bumping with proper version detection and error aggregation.

---

317-339: LGTM!

Good defensive handling with the nil check for env.Default and version parsing guard. Both prevent panics and incorrect bumps.

pkg/prowgen/jobbase.go (2)

11-11: LGTM!

Import added for the relocated ProvidesSignalForVersion function.

---

83-85: LGTM!

Clean refactor using the now-public ocplifecycle.ProvidesSignalForVersion function. This enables reuse in the bumper package as discussed in previous reviews.

pkg/branchcuts/bumper/gen-release-jobs-bumper_test.go (4)

44-44: LGTM!

Constructor call updated to include the new FileFinderSignal parameter.

---

205-284: LGTM!

Thorough test coverage for bumpStepEnvVars including OCP_VERSION handling, major version matching, non-version values, multiple vars, and nil input.

---

286-406: LGTM!

Comprehensive tests for bumpTestStepEnvVars covering all scenarios including the nil Default field case (lines 374-393), which was previously flagged as missing coverage.

---

408-501: LGTM!

Good test coverage for bumpBaseImages validating plain version tag bumping, non-version tag preservation, substring version handling, and nil input.

cmd/branchingconfigmanagers/generated-release-gating-jobs/main.go (2)

23-24: LGTM!

New fileFinder field added to options struct.

---

71-79: LGTM!

Clean wiring of the fileFinder option to the bumper constructor.

_{✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.}

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 golangci-lint (2.5.0)

Command failed

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[openshift-ci-robot]

@petr-muller: This pull request references DPTP-4435 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set.

Details

In response to this:

Find all release gating job configs using the same method used for determining which jobs will be consumed by TRT tooling. Code partially adapted from #4854.

When run over base state https://github.com/petr-muller/release/tree/dptp-4435-mock-prebranch-gating-jobs-final-base, it results in petr-muller/release@e8d717f over what the current tool creates in petr-muller/release@c8af926

---

branchcuts: bump versions in envvar values

Code adapted from #4854 by @Prucek

Results in the following changes over the previous commit: petr-muller/release@2822640

---

branchcuts: bump versions in base_images tags
Some configs import base_images from specified version-like tags like ci/tests-private:4.22. When tags are bare versions we can bumpt them too. We cannot bump tags where a version is a substring because cases like rhel-9-golang-1.24-openshift-4.22 cannot be handled by simply bumping the OCP version substring.

Results in the following changes over the previous commit: petr-muller/release@45a900a

---

branchcuts: ignore release gating jobs in openshift-priv

Results in the following changes over the previous commit: petr-muller/release@9c24588

---

branchcuts: ignore release gating jobs in non-variant configs
Nonvariant configs contain configuration that applies on the branch
directly and typically contains presubmits and postsubmits, not release
gating periodics.

It seems the only exception to this rule is this file (which is currently
missed by all other tooling):

ci-operator/config/shiftstack/ci/shiftstack-ci-release-4.22.yaml

Non-variant release-4.22 would typically be maintained by automation,
not manually, and TRT-facing jobs would be defined in a __periodics
file or something similar. If we can rename the config (which may be
non-trivial because job names carry meaning) the config may get picked
up by this tool again.

Results in the following changes over the previous commit: petr-muller/release@faca085

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

[petr-muller]

Is there a good home for this method better than prowgen package? It is technically not prowgen-internal anymore, something closer to ci-op configs would be a better fit. Prowgen would still use it but would import it from the new location.

[Prucek]

I think it could be here: pkg/api/ocplifecycle or in pkg/branchcuts/<something>
WDYT?

[petr-muller]

pkg/api/ocplifecycle sounds good! Let me fix that up.

/hold

[petr-muller]

/cc @Prucek @danilo-gemoli

[Copilot]

Pull request overview

This pull request improves the release gating jobs bumper tool to find and update all relevant release gating job configurations using the same signal-based method used by TRT tooling. The PR enhances version bumping capabilities across multiple configuration fields and adds a new file-finding mechanism.

Changes:

• Exported ProvidesSignalForVersion function in prowgen package to enable reuse by branchcuts tooling
• Added signal-based file finder that identifies configs providing CI signal for a version stream, with filtering for openshift-priv and non-variant configs
• Extended version bumping to include base_images tags, environment variables in test steps, and metadata branch field
• Added comprehensive test coverage for new bumping functions

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File	Description
pkg/prowgen/jobbase.go	Exports ProvidesSignalForVersion function by capitalizing it, enabling reuse by branchcuts tooling
pkg/branchcuts/bumper/gen-release-jobs-bumper.go	Adds signal-based file finder, extends bumping logic for env vars and base_images tags, updates metadata branch field
pkg/branchcuts/bumper/gen-release-jobs-bumper_test.go	Adds tests for new bumping functions (env vars, test step env vars, base images), updates existing tests to use new fileFinder parameter
cmd/branchingconfigmanagers/generated-release-gating-jobs/main.go	Adds file-finder CLI flag to select between regexp and signal-based file finding methods

Comments suppressed due to low confidence (1)

pkg/branchcuts/bumper/gen-release-jobs-bumper.go:149

• The documentation comment lists fields that are bumped but is now incomplete. The function also bumps .base_images.*.tag (line 199), .tests[].multistage.environment (line 255), .tests[].multistage.workflow (line 270), .metadata.variant (line 165), .metadata.branch (line 170), and test intervals (lines 174-179). The documentation should be updated to reflect all the fields that are actually being bumped.

// BumpContent bumps OCP versions in the given ci-operator config
//
// Candidate bumping fields:
// .base_images.*.name
// .releases.*.{release,candidate}.version
// .releases.*.prerelease.version_bounds.{lower,upper}
// .tests[].steps.test[].env[].default

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

pkg/branchcuts/bumper/gen-release-jobs-bumper.go (1)

321-332: Potential nil pointer dereference on env.Default.

env.Default is a *string and can be nil. Dereferencing it on line 324 without a nil check will cause a panic.

🐛 Proposed fix

 	for i := 0; i < len(multistageTest.Environment); i++ {
 		env := multistageTest.Environment[i]
+		if env.Default == nil {
+			continue
+		}
 		// value does not look like a version => nothing to bump
 		mm, err := ocplifecycle.ParseMajorMinor(*env.Default)
 		if err != nil {
 			continue
 		}

🤖 Fix all issues with AI agents

In `@pkg/branchcuts/bumper/gen-release-jobs-bumper.go`:
- Around line 44-49: The filepath.Walk callback passed to filepath.Walk in
gen-release-jobs-bumper.go ignores the incoming err parameter, so filesystem
errors (e.g., permission issues) are silently dropped; update the anonymous
func(path string, info fs.FileInfo, err error) error used with filepath.Walk to
first check if err != nil and return that error (or return filepath.SkipDir when
appropriate) before using getFilesRegexp and appending to files, ensuring Walk
surfaces filesystem errors instead of skipping them silently.

🧹 Nitpick comments (1)

pkg/branchcuts/bumper/gen-release-jobs-bumper_test.go (1)

286-386: Consider adding a test case for nil Default pointer.

The tests cover nil LiteralTestStep but not the case where env.Default is nil while Environment is populated. This would help verify the fix for the nil pointer dereference issue.

💡 Suggested test case

{
    id:    "Handles nil Default in step environment",
    major: 4,
    testStep: cioperatorapi.TestStep{
        LiteralTestStep: &cioperatorapi.LiteralTestStep{
            Environment: []cioperatorapi.StepParameter{
                {Name: "SOME_VAR", Default: nil},
            },
        },
    },
    wantStep: cioperatorapi.TestStep{
        LiteralTestStep: &cioperatorapi.LiteralTestStep{
            Environment: []cioperatorapi.StepParameter{
                {Name: "SOME_VAR", Default: nil},
            },
        },
    },
},

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting

📥 Commits

Reviewing files that changed from the base of the PR and between 69ff8fa and e71a368.

📒 Files selected for processing (4)

• cmd/branchingconfigmanagers/generated-release-gating-jobs/main.go
• pkg/branchcuts/bumper/gen-release-jobs-bumper.go
• pkg/branchcuts/bumper/gen-release-jobs-bumper_test.go
• pkg/prowgen/jobbase.go

🧰 Additional context used

📓 Path-based instructions (1)

**

⚙️ CodeRabbit configuration file

-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.

Files:

• pkg/prowgen/jobbase.go
• cmd/branchingconfigmanagers/generated-release-gating-jobs/main.go
• pkg/branchcuts/bumper/gen-release-jobs-bumper_test.go
• pkg/branchcuts/bumper/gen-release-jobs-bumper.go

🧬 Code graph analysis (3)

pkg/prowgen/jobbase.go (1)

pkg/api/types.go (1)

• ReleaseBuildConfiguration (37-140)

cmd/branchingconfigmanagers/generated-release-gating-jobs/main.go (1)

pkg/branchcuts/bumper/gen-release-jobs-bumper.go (1)

• NewGeneratedReleaseGatingJobsBumper (80-104)

pkg/branchcuts/bumper/gen-release-jobs-bumper_test.go (1)

pkg/branchcuts/bumper/gen-release-jobs-bumper.go (2)

• NewGeneratedReleaseGatingJobsBumper (80-104)
• FileFinderSignal (28-28)

🔇 Additional comments (9)

pkg/prowgen/jobbase.go (1)

22-35: Function export is appropriate for its new cross-package usage.

The function is now consumed by makeGetFilesProvidingSignal in the bumper package. The export is necessary for this use case.

Note: A past review comment suggested moving this to a location closer to ci-op configs since it's no longer prowgen-internal. This is a valid consideration for future refactoring but doesn't block this PR.

pkg/branchcuts/bumper/gen-release-jobs-bumper.go (4)

54-76: Signal-based file finder implementation looks correct.

The filtering logic properly excludes openshift-priv and non-variant configs as specified in the PR objectives. The use of prowgen.ProvidesSignalForVersion for version matching is appropriate.

---

80-104: Constructor changes are well-structured.

The pluggable file-finder strategy is cleanly implemented with proper validation for unknown strategies.

---

185-209: Base image bumping logic is sound.

The approach of only bumping plain version tags (where ParseMajorMinor succeeds) while leaving complex tags like rhel-9-golang-1.24-openshift-4.22 untouched aligns with the PR objectives. Error aggregation ensures all images are attempted.

---

281-301: Environment variable bumping logic is correct.

The function properly handles non-version values by skipping them and correctly identifies version-like values for bumping based on the OCP_VERSION name or matching major version.

pkg/branchcuts/bumper/gen-release-jobs-bumper_test.go (2)

205-284: Good test coverage for bumpStepEnvVars.

Tests cover the key scenarios: OCP_VERSION handling, major version matching, different major versions, non-version values, multiple vars, and nil input.

---

388-482: Comprehensive test coverage for bumpBaseImages.

Tests properly verify plain version tag bumping, substring version handling, cross-major version guards, and nil handling.

cmd/branchingconfigmanagers/generated-release-gating-jobs/main.go (2)

34-34: Flag default aligns with PR objectives.

The default value of "signal" matches the commit message stating the switch to signal-based file finding as the default.

---

71-79: Constructor call correctly passes the new fileFinder parameter.

The error from the constructor will surface invalid fileFinder values, providing adequate validation.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Walk callback ignores filesystem errors.

The err parameter in the Walk callback is not checked. This could silently skip directories or files due to permission errors or other filesystem issues.

🔧 Proposed fix

 	return func() ([]string, error) {
 		files := make([]string, 0)
 		err := filepath.Walk(baseDir, func(path string, info fs.FileInfo, err error) error {
+			if err != nil {
+				return err
+			}
 			if getFilesRegexp.Match([]byte(path)) {
 				files = append(files, path)
 			}
 			return nil
 		})
 		return files, err
 	}

📝 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

	err := filepath.Walk(baseDir, func(path string, info fs.FileInfo, err error) error {
	if getFilesRegexp.Match([]byte(path)) {
	files = append(files, path)
	}
	return nil
	})
	err := filepath.Walk(baseDir, func(path string, info fs.FileInfo, err error) error {
	if err != nil {
	return err
	}
	if getFilesRegexp.Match([]byte(path)) {
	files = append(files, path)
	}
	return nil
	})

🤖 Prompt for AI Agents

In `@pkg/branchcuts/bumper/gen-release-jobs-bumper.go` around lines 44 - 49, The
filepath.Walk callback passed to filepath.Walk in gen-release-jobs-bumper.go
ignores the incoming err parameter, so filesystem errors (e.g., permission
issues) are silently dropped; update the anonymous func(path string, info
fs.FileInfo, err error) error used with filepath.Walk to first check if err !=
nil and return that error (or return filepath.SkipDir when appropriate) before
using getFilesRegexp and appending to files, ensuring Walk surfaces filesystem
errors instead of skipping them silently.

[petr-muller]

This is not new code, just extracted to a new method

[coderabbitai]

@petr-muller Thank you for the clarification! You're right — if this is existing behavior that's just been refactored, it makes sense to keep this PR focused on its primary objective rather than mixing in unrelated fixes.

Acknowledged! 🐰

[coderabbitai]

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

pkg/branchcuts/bumper/gen-release-jobs-bumper.go (1)

322-333: Nil pointer dereference when env.Default is nil.

env.Default is an optional field (*string). Dereferencing it at line 325 without a nil check will cause a panic if any StepParameter has no default value set.

🐛 Proposed fix

 	for i := 0; i < len(multistageTest.Environment); i++ {
 		env := multistageTest.Environment[i]
+		if env.Default == nil {
+			continue
+		}
 		// value does not look like a version => nothing to bump
 		mm, err := ocplifecycle.ParseMajorMinor(*env.Default)
 		if err != nil {
 			continue
 		}

♻️ Duplicate comments (1)

pkg/branchcuts/bumper/gen-release-jobs-bumper.go (1)

45-50: Walk callback ignores filesystem errors.

The err parameter in the Walk callback is not checked. Filesystem errors (permission issues, broken symlinks) will be silently ignored.

🔧 Proposed fix

 	return func() ([]string, error) {
 		files := make([]string, 0)
 		err := filepath.Walk(baseDir, func(path string, info fs.FileInfo, err error) error {
+			if err != nil {
+				return err
+			}
 			if getFilesRegexp.Match([]byte(path)) {
 				files = append(files, path)
 			}
 			return nil
 		})
 		return files, err
 	}

🧹 Nitpick comments (1)

pkg/branchcuts/bumper/gen-release-jobs-bumper_test.go (1)

368-374: Consider adding a test case for nil Default value.

The test covers nil LiteralTestStep but doesn't test StepParameter entries where Default is nil. This would help catch the nil pointer dereference issue in bumpTestStepEnvVars.

💡 Suggested test case

{
	id:    "Handles nil Default in StepParameter",
	major: 4,
	testStep: cioperatorapi.TestStep{
		LiteralTestStep: &cioperatorapi.LiteralTestStep{
			Environment: []cioperatorapi.StepParameter{
				{Name: "REQUIRED_VAR"}, // Default is nil
			},
		},
	},
	wantStep: cioperatorapi.TestStep{
		LiteralTestStep: &cioperatorapi.LiteralTestStep{
			Environment: []cioperatorapi.StepParameter{
				{Name: "REQUIRED_VAR"},
			},
		},
	},
},

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting

📥 Commits

Reviewing files that changed from the base of the PR and between e71a368 and 0424df6.

📒 Files selected for processing (3)

• cmd/branchingconfigmanagers/generated-release-gating-jobs/main.go
• pkg/branchcuts/bumper/gen-release-jobs-bumper.go
• pkg/branchcuts/bumper/gen-release-jobs-bumper_test.go

🧰 Additional context used

📓 Path-based instructions (1)

**

⚙️ CodeRabbit configuration file

-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.

Files:

• pkg/branchcuts/bumper/gen-release-jobs-bumper_test.go
• pkg/branchcuts/bumper/gen-release-jobs-bumper.go
• cmd/branchingconfigmanagers/generated-release-gating-jobs/main.go

🧬 Code graph analysis (2)

pkg/branchcuts/bumper/gen-release-jobs-bumper_test.go (2)

pkg/branchcuts/bumper/gen-release-jobs-bumper.go (2)

• NewGeneratedReleaseGatingJobsBumper (81-105)
• FileFinderSignal (29-29)

pkg/api/types.go (2)

• StepParameter (1123-1130)
• ImageStreamTagReference (499-506)

cmd/branchingconfigmanagers/generated-release-gating-jobs/main.go (1)

pkg/branchcuts/bumper/gen-release-jobs-bumper.go (1)

• NewGeneratedReleaseGatingJobsBumper (81-105)

🔇 Additional comments (4)

pkg/branchcuts/bumper/gen-release-jobs-bumper.go (1)

186-210: LGTM on error aggregation fix.

The bumpBaseImages function now correctly returns aggregated errors via errors.NewAggregate(errs), addressing the previous review feedback.

pkg/branchcuts/bumper/gen-release-jobs-bumper_test.go (1)

204-284: Good test coverage for bumpStepEnvVars.

The tests comprehensively cover multiple scenarios including OCP_VERSION, matching/mismatched major versions, non-version values, multiple variables, and nil input.

cmd/branchingconfigmanagers/generated-release-gating-jobs/main.go (2)

23-34: LGTM on file-finder flag integration.

The new --file-finder flag is cleanly integrated with a sensible default (signal) and clear help text. Validation is appropriately delegated to the NewGeneratedReleaseGatingJobsBumper constructor.

---

71-80: Reconcile function correctly updated.

The NewGeneratedReleaseGatingJobsBumper call properly passes the new fileFinder parameter, maintaining consistent error handling.

_{✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.}

[petr-muller]

/retest-required

[Prucek]

I think it could be here: pkg/api/ocplifecycle or in pkg/branchcuts/<something>
WDYT?

[openshift-ci-robot]

@petr-muller: This pull request references DPTP-4435 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set.

Details

In response to this:

Find all release gating job configs using the same method used for determining which jobs will be consumed by TRT tooling. Code partially adapted from #4854.

When run over base state https://github.com/petr-muller/release/tree/dptp-4435-mock-prebranch-gating-jobs-final-base, it results in petr-muller/release@e8d717f over what the current tool creates in petr-muller/release@c8af926

---

branchcuts: bump versions in envvar values

Code adapted from #4854 by @Prucek

Results in the following changes over the previous commit: petr-muller/release@2822640

---

branchcuts: bump versions in base_images tags
Some configs import base_images from specified version-like tags like ci/tests-private:4.22. When tags are bare versions we can bumpt them too. We cannot bump tags where a version is a substring because cases like rhel-9-golang-1.24-openshift-4.22 cannot be handled by simply bumping the OCP version substring.

Results in the following changes over the previous commit: petr-muller/release@45a900a

---

branchcuts: ignore release gating jobs in openshift-priv

Results in the following changes over the previous commit: petr-muller/release@9c24588

---

branchcuts: ignore release gating jobs in non-variant configs
Nonvariant configs contain configuration that applies on the branch
directly and typically contains presubmits and postsubmits, not release
gating periodics.

It seems the only exception to this rule is this file (which is currently
missed by all other tooling):

ci-operator/config/shiftstack/ci/shiftstack-ci-release-4.22.yaml

Non-variant release-4.22 would typically be maintained by automation,
not manually, and TRT-facing jobs would be defined in a __periodics
file or something similar. If we can rename the config (which may be
non-trivial because job names carry meaning) the config may get picked
up by this tool again.

Results in the following changes over the previous commit: petr-muller/release@faca085

---

refactor: move ProvidesSignalForVersion to ocplifecycle

The method is no longer prowgen-internal, it is now a more general
ci-operator interrogation method that is used both for marking jobs for
TRT consumption (through job-release label) and on the other side for
recognizing which jobs would be marked as such, so they can be handled
by tooling e.g. involved in branchcuts.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

[petr-muller]

/hold cancel

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Comments suppressed due to low confidence (2)

cmd/branchingconfigmanagers/generated-release-gating-jobs/main.go:53

• The file-finder flag value is not validated in gatherOptions(). Invalid values will only be caught when creating the bumper. Consider adding validation here to provide earlier feedback to users. For example, check if the value is one of the valid options: regexp or signal.

func gatherOptions() (*options, error) {
	var errs []error
	o := &options{}
	flag.StringVar(&o.curOCPVersion, "current-release", "", "Current OCP version")
	flag.StringVar(&o.releaseRepoDir, "release-repo", "", "Path to 'openshift/release/ folder")
	flag.IntVar(&o.newIntervalValue, "interval", 168, "New interval to set")
	flag.IntVar(&o.logLevel, "log-level", int(logrus.DebugLevel), "Log level")
	flag.StringVar(&o.fileFinder, "file-finder", "signal", "Method to find files with gating jobs. One of: regexp | signal")
	flag.Parse()

	if _, err := ocplifecycle.ParseMajorMinor(o.curOCPVersion); o.curOCPVersion != "" && err != nil {
		errs = append(errs, fmt.Errorf("error parsing current-release %s", o.curOCPVersion))
	}

	if o.newIntervalValue < 0 {
		errs = append(errs, errors.New("error parsing interval: value is not a positive integer"))
	}

	if o.releaseRepoDir != "" {
		if !path.IsAbs(o.releaseRepoDir) {
			errs = append(errs, errors.New("error parsing release repo path: path has to be absolute"))
		}
	} else {
		errs = append(errs, errors.New("error parsing release repo path: path is mandatory"))
	}

	return o, utilerrors.NewAggregate(errs)

pkg/branchcuts/bumper/gen-release-jobs-bumper.go:149

• The documentation comment for BumpContent is incomplete. It lists candidate bumping fields but is missing several fields that are actually bumped by the implementation: .base_images.*.tag, .tests[].steps.{pre,test,post}[].env[].default, .tests[].multi_stage_test_configuration.environment, .tests[].multi_stage_test_configuration.workflow, .metadata.variant, and .metadata.branch.

// BumpContent bumps OCP versions in the given ci-operator config
//
// Candidate bumping fields:
// .base_images.*.name
// .releases.*.{release,candidate}.version
// .releases.*.prerelease.version_bounds.{lower,upper}
// .tests[].steps.test[].env[].default

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The code dereferences env.Default without checking if it's nil. Since Default is a pointer field that can be nil (as indicated by the omitempty JSON tag), this will cause a panic if an environment variable is defined without a default value. Add a nil check before dereferencing: if env.Default == nil { continue }.

Suggested change

	env := multistageTest.Environment[i]
	env := multistageTest.Environment[i]
	if env.Default == nil {
	continue
	}

[Copilot]

The error parameter passed to the walk function is not checked before processing. If there's an error accessing a file or directory, it should be handled. Add a check: if err != nil { return err } at the beginning of the walk function to properly propagate errors.

[Copilot]

Test coverage is missing for the case where env.Default is nil. This is important because Default is a pointer field that can be nil (as indicated by the omitempty JSON tag in the API definition), and the implementation should handle this case gracefully to prevent panics.

[coderabbitai]

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

pkg/branchcuts/bumper/gen-release-jobs-bumper.go (1)

317-336: Potential nil pointer dereference on env.Default.

Line 324 dereferences env.Default without checking if it's nil. While the function checks that LiteralTestStep and the Environment slice exist, individual StepParameter entries may have a nil Default field (it's optional with omitempty tag).

🐛 Proposed fix

 	for i := 0; i < len(multistageTest.Environment); i++ {
 		env := multistageTest.Environment[i]
+		if env.Default == nil {
+			continue
+		}
 		// value does not look like a version => nothing to bump
 		mm, err := ocplifecycle.ParseMajorMinor(*env.Default)
 		if err != nil {
 			continue
 		}

♻️ Duplicate comments (1)

pkg/branchcuts/bumper/gen-release-jobs-bumper.go (1)

38-52: Pre-existing: Walk callback ignores filesystem errors.

The err parameter is not checked, which could silently skip files on permission errors. This was flagged previously and acknowledged as pre-existing behavior. Consider addressing in a follow-up.

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting

📥 Commits

Reviewing files that changed from the base of the PR and between 0424df6 and c05d952.

📒 Files selected for processing (3)

• pkg/api/ocplifecycle/ocplifecycle.go
• pkg/branchcuts/bumper/gen-release-jobs-bumper.go
• pkg/prowgen/jobbase.go

🚧 Files skipped from review as they are similar to previous changes (1)

• pkg/prowgen/jobbase.go

🧰 Additional context used

📓 Path-based instructions (1)

**

⚙️ CodeRabbit configuration file

-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.

Files:

• pkg/api/ocplifecycle/ocplifecycle.go
• pkg/branchcuts/bumper/gen-release-jobs-bumper.go

🧬 Code graph analysis (2)

pkg/api/ocplifecycle/ocplifecycle.go (1)

pkg/api/types.go (2)

• LatestReleaseName (2865-2865)
• Candidate (364-370)

pkg/branchcuts/bumper/gen-release-jobs-bumper.go (3)

pkg/api/ocplifecycle/ocplifecycle.go (3)

• MajorMinor (199-202)
• ProvidesSignalForVersion (262-267)
• ParseMajorMinor (237-252)

pkg/config/load.go (2)

• OperateOnCIOperatorConfigDir (327-329)
• DataWithInfo (415-418)

pkg/api/types.go (2)

• ReleaseBuildConfiguration (37-140)
• Metadata (161-166)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)

• GitHub Check: Agent
• GitHub Check: CodeQL analysis (go)

🔇 Additional comments (10)

pkg/api/ocplifecycle/ocplifecycle.go (2)

15-16: LGTM!

The new import is correctly added and properly aliased for clarity.

---

253-267: LGTM!

The function is well-documented and correctly implements the signal detection logic. The nil check on release.Candidate prevents potential nil pointer dereference.

pkg/branchcuts/bumper/gen-release-jobs-bumper.go (8)

21-29: LGTM!

The constants are well-organized and clearly named. The file finder strategy constants (FileFinderRegexp, FileFinderSignal) provide a clean API for selecting the discovery method.

---

54-76: LGTM!

The signal-based file finder is well-implemented with clear comments explaining the filtering logic. The integration with ocplifecycle.ProvidesSignalForVersion is correct.

---

80-104: LGTM!

The constructor cleanly implements the strategy pattern with proper validation of the fileFinder parameter.

---

125-141: LGTM!

The function correctly bumps both Variant and Branch fields, ensuring consistency with the metadata updates in BumpContent.

---

143-183: LGTM!

The content bumping workflow is well-structured, handling all relevant configuration fields. The separation between dataWithInfo.Info.Metadata (file path) and config.Metadata (embedded config) justifies bumping in both BumpFilename and BumpContent.

---

185-209: LGTM!

The error aggregation is now correctly returned via errors.NewAggregate(errs). The selective tag bumping (only plain versions like 4.21) is a sensible approach with clear documentation.

---

243-279: LGTM!

The function properly handles environment variables at the test level and correctly guards workflow bumping with a nil check.

---

281-301: LGTM!

The helper correctly identifies version-like environment values and applies bumping logic consistently. Error aggregation is properly implemented.

_{✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.}

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: petr-muller, Prucek

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [Prucek]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[petr-muller]

/test unit

[openshift-ci]

@petr-muller: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/unit	0b8d4f8	link	true	/test unit
ci/prow/images	0b8d4f8	link	true	/test images
ci/prow/breaking-changes	0b8d4f8	link	false	/test breaking-changes

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.