Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
[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
WalkthroughThis change introduces sparse checkout functionality to the job base configuration in ProwGen. It adds helper functions to determine which files require sparse checkout, modifies the cloning skip logic to depend on sparse checkout file count, updates DecorationConfig initialization to conditionally set SkipCloning and OauthTokenSecret fields, populates the SparseCheckoutFiles field, and reverses the FromRepository handling logic. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/prowgen/jobbase.go (2)
105-113: Compute sparse files once and reuse the derived skip decision.This block recalculates sparse checkout data multiple times (also used again at Line 131 via
skipCloning(configSpec)). A single computation improves consistency and avoids duplicate work.Proposed simplification
- if skipCloning(configSpec) { + sparseFiles := sparseCheckoutFiles(configSpec) + shouldSkipCloning := len(sparseFiles) == 0 + if shouldSkipCloning { b.base.UtilityConfig.DecorationConfig.SkipCloning = utilpointer.Bool(true) } else if info.Config.Private { b.base.UtilityConfig.DecorationConfig.OauthTokenSecret = &prowv1.OauthTokenSecret{Key: cioperatorapi.OauthTokenSecretKey, Name: cioperatorapi.OauthTokenSecretName} } - sparseFiles := sparseCheckoutFiles(configSpec) b.base.UtilityConfig.DecorationConfig.SparseCheckoutFiles = sparseFiles🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/prowgen/jobbase.go` around lines 105 - 113, Compute sparseFiles once and cache the skip decision so we don't recompute; call sparseCheckoutFiles(configSpec) into sparseFiles and call skipCloning(configSpec) once into a local boolean (e.g. shouldSkip) before the decoration config block, then use shouldSkip to set b.base.UtilityConfig.DecorationConfig.SkipCloning and reuse that same variable wherever skipCloning(configSpec) was used later (instead of calling skipCloning again), keeping the existing OauthTokenSecret and SparseCheckoutFiles assignments intact.
30-37: Avoid mutating input config while checking build-root flags.
buildRoots := configSpec.BuildRootImagesaliases the original map, so writing tobuildRoots[""]mutatesconfigSpec.BuildRootImages. This helper should stay read-only.Proposed refactor (pure check, no map writes)
func fromRepositorySet(configSpec *cioperatorapi.ReleaseBuildConfiguration) bool { - buildRoots := configSpec.BuildRootImages - if buildRoots == nil { - buildRoots = make(map[string]cioperatorapi.BuildRootImageConfiguration) - } - if configSpec.BuildRootImage != nil { - buildRoots[""] = *configSpec.BuildRootImage - } - for _, buildRoot := range buildRoots { + if configSpec.BuildRootImage != nil && configSpec.BuildRootImage.FromRepository { + return true + } + for _, buildRoot := range configSpec.BuildRootImages { if buildRoot.FromRepository { return true } } return false }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/prowgen/jobbase.go` around lines 30 - 37, fromRepositorySet currently aliases and mutates the input map by doing buildRoots := configSpec.BuildRootImages and then writing buildRoots[""]; avoid mutating configSpec. Either (a) treat the check as read-only by testing configSpec.BuildRootImage != nil or checking for the empty key in configSpec.BuildRootImages without writing to it, or (b) create a local copy before any writes (e.g. make a new map, copy entries from configSpec.BuildRootImages, then set the "" key if configSpec.BuildRootImage != nil). Update the logic in fromRepositorySet to use one of these approaches so configSpec.BuildRootImages is never modified.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/prowgen/jobbase.go`:
- Around line 105-113: Compute sparseFiles once and cache the skip decision so
we don't recompute; call sparseCheckoutFiles(configSpec) into sparseFiles and
call skipCloning(configSpec) once into a local boolean (e.g. shouldSkip) before
the decoration config block, then use shouldSkip to set
b.base.UtilityConfig.DecorationConfig.SkipCloning and reuse that same variable
wherever skipCloning(configSpec) was used later (instead of calling skipCloning
again), keeping the existing OauthTokenSecret and SparseCheckoutFiles
assignments intact.
- Around line 30-37: fromRepositorySet currently aliases and mutates the input
map by doing buildRoots := configSpec.BuildRootImages and then writing
buildRoots[""]; avoid mutating configSpec. Either (a) treat the check as
read-only by testing configSpec.BuildRootImage != nil or checking for the empty
key in configSpec.BuildRootImages without writing to it, or (b) create a local
copy before any writes (e.g. make a new map, copy entries from
configSpec.BuildRootImages, then set the "" key if configSpec.BuildRootImage !=
nil). Update the logic in fromRepositorySet to use one of these approaches so
configSpec.BuildRootImages is never modified.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 273c2059-d1e2-4358-8dff-08f771048c4d
📒 Files selected for processing (1)
pkg/prowgen/jobbase.go
2 participants
Summary by CodeRabbit
Release Notes