branchcuts: add bumping of release gating jobs from sippy config #4854
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
WalkthroughAdds a Sippy config parser and optional Sippy-based discovery of ci-operator config files; integrates Sippy into the bumper with a new constructor/flags, updates file-discovery branching, and extends bump logic to update Branch metadata and handle nil workflows. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to 📒 Files selected for processing (2)
🧰 Additional context used📓 Path-based instructions (1)**⚙️ CodeRabbit configuration file
Files:
🔇 Additional comments (13)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Prucek The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
pkg/branchcuts/bumper/sippy.go (1)
247-249: Consider logging when jobs lackextra_refs.Jobs without
extra_refsare silently skipped here. Since these are jobs you're actively looking for (in theneededmap), a debug log would help diagnose why certain jobs aren't being resolved.if len(job.ExtraRefs) == 0 { + logrus.Debugf("skipping job %s: no extra_refs", job.Name) continue }pkg/branchcuts/bumper/sippy_test.go (1)
9-148: Consider adding edge case tests.The test covers the happy path well, but consider adding test cases for:
- A periodic job that doesn't exist in any job file (job not found error)
- A periodic job found in job file but the corresponding config file doesn't exist
These would improve coverage of error paths in
JobNameToConfigFile.pkg/branchcuts/bumper/gen-release-jobs-bumper.go (1)
35-67: Minor duplication in constructors.Both constructors duplicate the regex compilation logic. Consider extracting to reduce duplication, though this is optional given the simplicity.
+func newBumperCommon(ocpVer, jobsDir 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, + }, nil +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (3)
pkg/branchcuts/bumper/gen-release-jobs-bumper.go(6 hunks)pkg/branchcuts/bumper/sippy.go(1 hunks)pkg/branchcuts/bumper/sippy_test.go(1 hunks)
🧰 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/sippy_test.gopkg/branchcuts/bumper/gen-release-jobs-bumper.gopkg/branchcuts/bumper/sippy.go
🧬 Code graph analysis (3)
pkg/branchcuts/bumper/sippy_test.go (1)
pkg/branchcuts/bumper/sippy.go (3)
JobNameToConfigFile(69-85)LoadSippyConfig(25-37)ProwJobMetadata(60-65)
pkg/branchcuts/bumper/gen-release-jobs-bumper.go (3)
pkg/api/ocplifecycle/ocplifecycle.go (1)
ParseMajorMinor(235-250)pkg/branchcuts/bumper/sippy.go (1)
GetConfigFilesForReleaseFromSippy(278-301)pkg/api/types.go (2)
Metadata(156-161)MultiStageTestConfiguration(1192-1233)
pkg/branchcuts/bumper/sippy.go (1)
pkg/prowgen/podspec.go (1)
Variant(561-569)
🔇 Additional comments (10)
pkg/branchcuts/bumper/sippy.go (5)
14-22: LGTM!The struct definitions are clean and properly tagged for YAML unmarshalling.
25-57: LGTM!Loading and accessor methods are well-structured with proper error wrapping.
94-157: LGTM!The function correctly walks the directory tree, handles parsing errors gracefully by continuing to the next file, and uses
filepath.SkipAllfor early termination when the job is found.
159-202: LGTM!Good use of caching via
buildJobMetadataCacheto avoid repeated filesystem walks, and proper deduplication of config files.
277-301: LGTM!End-to-end integration correctly combines both informing and blocking jobs before resolving to config files.
pkg/branchcuts/bumper/sippy_test.go (2)
150-201: LGTM!The test covers config loading and accessor methods. The symmetric
GetBlockingJobsForReleaseisn't tested but follows the same pattern.
203-249: LGTM!Good coverage of filename building with and without variants.
pkg/branchcuts/bumper/gen-release-jobs-bumper.go (3)
121-136: LGTM!Adding Branch bumping alongside Variant is consistent with the expanded scope of this PR to handle release gating jobs from Sippy config.
158-167: LGTM!Bumping Branch in
zz_generated_metadatais consistent with the filename bumping logic.
250-254: Good nil check addition.The
Workflowfield is a*stringand can be nil. This guard prevents a potential nil pointer dereference.
| 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 | ||
| } |
There was a problem hiding this comment.
🧩 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 -100Length 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.
| 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.
Prucek
force-pushed
the
bumper-update
branch
from
c3056f0 to
4605cf5
Compare
|
/cc |
| envValue := env[envKey] | ||
| mm, err := ocplifecycle.ParseMajorMinor(envValue) | ||
| if err != nil { | ||
| continue |
There was a problem hiding this comment.
Use the logger and continue, do not swallow the error silently,
There was a problem hiding this comment.
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
|
/test e2e |
|
@Prucek: Overrode contexts on behalf of Prucek: ci/prow/breaking-changes DetailsIn response to this:
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. |
|
@Prucek: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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. |
4 participants
Populating release gating jobs in Pre-branching generated by the
generated-release-gating-jobswas, until now, doing only a subset of all release gating jobs. Utilizing the sippy config, we can get all the release gating jobs and their files and update all of them using the existing bumper.Edit: added env var bumping
Result: openshift/release#71837