prowgen: add sparse checkout for .ci-operator.yaml and Dockerfiles#4990
Open
Prucek wants to merge 1 commit intoopenshift:mainfrom
Open
prowgen: add sparse checkout for .ci-operator.yaml and Dockerfiles#4990Prucek wants to merge 1 commit intoopenshift:mainfrom
Prucek wants to merge 1 commit intoopenshift:mainfrom
Conversation
Contributor
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
Contributor
|
Skipping CI for Draft Pull Request. |
Contributor
|
[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 |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughCompute ordered, deduplicated sparse-checkout file lists from ReleaseBuildConfiguration (build roots and images), include Changes
Sequence Diagram(s)sequenceDiagram
participant R as ReleaseBuildConfiguration
participant B as ProwJobBaseBuilder
participant H as SparseCheckoutHelper
participant D as DecorationConfig
R->>B: provide configSpec (build roots, images)
B->>H: compute sparse-checkout list (detect FromRepository, collect Dockerfile candidates)
H-->>B: return ordered, deduplicated file list
B->>D: set SparseCheckoutFiles (mutate existing DecorationConfig)
B->>D: set SkipCloning based on list empty? (empty => true)
B->>D: when private job -> set OauthTokenSecret field (mutate existing DecorationConfig)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 warning, 2 inconclusive)
✅ Passed checks (9 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
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
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/prowgen/jobbase.go`:
- Around line 24-50: fromRepositorySet and sparseCheckoutFiles currently
dereference configSpec without a nil guard and will panic if called with nil;
add an explicit nil check at the start of fromRepositorySet (return false if
configSpec == nil) and at the start of sparseCheckoutFiles (return empty slice
if configSpec == nil) so callers like sparseCheckoutFiles -> fromRepositorySet
(which references cioperatorapi.CIOperatorInrepoConfigFileName) are safe when
configSpec is nil.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 0f9e4af2-a726-4428-a8d9-fa8266c31820
📒 Files selected for processing (20)
cmd/ci-operator-prowgen/testdata/zz_fixture_postsubmit_TestFromCIOperatorConfigToProwYaml_Custom_test_timeout.yamlcmd/ci-operator-prowgen/testdata/zz_fixture_postsubmit_TestFromCIOperatorConfigToProwYaml_Input_is_YAML_and_it_is_correctly_processed.yamlcmd/ci-operator-prowgen/testdata/zz_fixture_postsubmit_TestFromCIOperatorConfigToProwYaml_Using_a_variant_config__one_test_and_images__one_existing_job._Expect_one_presubmit__pre_post_submit_images_jobs._Existing_job_should_not_be_changed..yamlcmd/ci-operator-prowgen/testdata/zz_fixture_postsubmit_TestFromCIOperatorConfigToProwYaml_one_test_and_images__no_previous_jobs._Expect_test_presubmit__pre_post_submit_images_jobs.yamlcmd/ci-operator-prowgen/testdata/zz_fixture_presubmit_TestFromCIOperatorConfigToProwYaml_Custom_test_timeout.yamlcmd/ci-operator-prowgen/testdata/zz_fixture_presubmit_TestFromCIOperatorConfigToProwYaml_Input_is_YAML_and_it_is_correctly_processed.yamlcmd/ci-operator-prowgen/testdata/zz_fixture_presubmit_TestFromCIOperatorConfigToProwYaml_Using_a_variant_config__one_test_and_images__one_existing_job._Expect_one_presubmit__pre_post_submit_images_jobs._Existing_job_should_not_be_changed..yamlcmd/ci-operator-prowgen/testdata/zz_fixture_presubmit_TestFromCIOperatorConfigToProwYaml_one_test_and_images__no_previous_jobs._Expect_test_presubmit__pre_post_submit_images_jobs.yamlpkg/prowgen/jobbase.gopkg/prowgen/jobbase_test.gopkg/prowgen/prowgen_test.gopkg/prowgen/testdata/zz_fixture_TestGenerateJobs_Promotion_configuration_causes_promote_job_with_unique_targets.yamlpkg/prowgen/testdata/zz_fixture_TestGenerateJobs_promotion_postsubmit_and_periodic_.yamlpkg/prowgen/testdata/zz_fixture_TestGeneratePeriodicForTest_periodic_with_from_repository_build_root__ExtraRefs_has_sparse_checkout_files.yamlpkg/prowgen/testdata/zz_fixture_TestGeneratePeriodicForTest_periodic_with_from_repository_build_root_and_images__ExtraRefs_has_sparse_checkout_files_with_dockerfiles.yamlpkg/prowgen/testdata/zz_fixture_TestNewProwJobBaseBuilderForTest_simple_container_based_test_with_timeout_and_no_decoration.yamlpkg/prowgen/testdata/zz_fixture_TestProwJobBaseBuilder_job_with_from_repository_build_root__has_sparse_checkout_with_.ci_operator.yaml.yamlpkg/prowgen/testdata/zz_fixture_TestProwJobBaseBuilder_job_with_from_repository_build_root_and_images__adds_dockerfiles_to_sparse_checkout.yamlpkg/prowgen/testdata/zz_fixture_TestProwJobBaseBuilder_job_with_image_builds_in_of_openshift_release_main__does_not_have_no_builds__label.yamlpkg/prowgen/testdata/zz_fixture_TestProwJobBaseBuilder_private_job_with_cloning__including_podspec.yaml
✅ Files skipped from review due to trivial changes (9)
- cmd/ci-operator-prowgen/testdata/zz_fixture_postsubmit_TestFromCIOperatorConfigToProwYaml_one_test_and_images__no_previous_jobs._Expect_test_presubmit__pre_post_submit_images_jobs.yaml
- pkg/prowgen/testdata/zz_fixture_TestProwJobBaseBuilder_private_job_with_cloning__including_podspec.yaml
- pkg/prowgen/testdata/zz_fixture_TestProwJobBaseBuilder_job_with_image_builds_in_of_openshift_release_main__does_not_have_no_builds__label.yaml
- pkg/prowgen/testdata/zz_fixture_TestGeneratePeriodicForTest_periodic_with_from_repository_build_root_and_images__ExtraRefs_has_sparse_checkout_files_with_dockerfiles.yaml
- pkg/prowgen/testdata/zz_fixture_TestNewProwJobBaseBuilderForTest_simple_container_based_test_with_timeout_and_no_decoration.yaml
- cmd/ci-operator-prowgen/testdata/zz_fixture_presubmit_TestFromCIOperatorConfigToProwYaml_one_test_and_images__no_previous_jobs._Expect_test_presubmit__pre_post_submit_images_jobs.yaml
- pkg/prowgen/testdata/zz_fixture_TestGenerateJobs_Promotion_configuration_causes_promote_job_with_unique_targets.yaml
- cmd/ci-operator-prowgen/testdata/zz_fixture_presubmit_TestFromCIOperatorConfigToProwYaml_Using_a_variant_config__one_test_and_images__one_existing_job._Expect_one_presubmit__pre_post_submit_images_jobs._Existing_job_should_not_be_changed..yaml
- pkg/prowgen/testdata/zz_fixture_TestGeneratePeriodicForTest_periodic_with_from_repository_build_root__ExtraRefs_has_sparse_checkout_files.yaml
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/prowgen/testdata/zz_fixture_TestProwJobBaseBuilder_job_with_from_repository_build_root__has_sparse_checkout_with_.ci_operator.yaml.yaml
- pkg/prowgen/testdata/zz_fixture_TestProwJobBaseBuilder_job_with_from_repository_build_root_and_images__adds_dockerfiles_to_sparse_checkout.yaml
- pkg/prowgen/testdata/zz_fixture_TestGenerateJobs_promotion_postsubmit_and_periodic_.yaml
Comment on lines
+24
to
+50
| func fromRepositorySet(configSpec *cioperatorapi.ReleaseBuildConfiguration) bool { | ||
| if configSpec.BuildRootImage != nil && configSpec.BuildRootImage.FromRepository { | ||
| return true | ||
| } | ||
| for _, buildRoot := range buildRoots { | ||
| for _, buildRoot := range configSpec.BuildRootImages { | ||
| if buildRoot.FromRepository { | ||
| return false | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| } | ||
|
|
||
| return true | ||
| // sparseCheckoutFiles returns the list of files to include in a sparse checkout | ||
| // when the configuration requires cloning. This includes the in-repo ci-operator | ||
| // configuration file and any Dockerfiles that need to be built. | ||
| // Returns an empty slice if no sparse checkout is needed. | ||
| func sparseCheckoutFiles(configSpec *cioperatorapi.ReleaseBuildConfiguration) []string { | ||
| seen := map[string]bool{} | ||
| var result []string | ||
| add := func(path string) { | ||
| if !seen[path] { | ||
| seen[path] = true | ||
| result = append(result, path) | ||
| } | ||
| } | ||
| if fromRepositorySet(configSpec) { | ||
| add(cioperatorapi.CIOperatorInrepoConfigFileName) |
There was a problem hiding this comment.
Guard nil configSpec before sparse-checkout evaluation.
Line 25 dereferences configSpec without a nil check, and Line 102 calls into this path unconditionally. A nil input will panic before builder initialization completes.
💡 Suggested fix
func fromRepositorySet(configSpec *cioperatorapi.ReleaseBuildConfiguration) bool {
+ if configSpec == nil {
+ return false
+ }
if configSpec.BuildRootImage != nil && configSpec.BuildRootImage.FromRepository {
return true
}
for _, buildRoot := range configSpec.BuildRootImages {
if buildRoot.FromRepository {
return true
}
}
return false
}
func sparseCheckoutFiles(configSpec *cioperatorapi.ReleaseBuildConfiguration) []string {
+ if configSpec == nil {
+ return nil
+ }
seen := map[string]bool{}
var result []stringAlso applies to: 102-103
🧰 Tools
🪛 golangci-lint (2.11.4)
[error] 41-41: : # github.com/openshift/ci-tools/pkg/api/multiarchbuildconfig/v1
pkg/api/multiarchbuildconfig/v1/register.go:41:3: cannot use &MultiArchBuildConfig{} (value of type *MultiArchBuildConfig) as "k8s.io/apimachinery/pkg/runtime".Object value in argument to scheme.AddKnownTypes: *MultiArchBuildConfig does not implement "k8s.io/apimachinery/pkg/runtime".Object (missing method DeepCopyObject)
pkg/api/multiarchbuildconfig/v1/register.go:42:3: cannot use &MultiArchBuildConfigList{} (value of type *MultiArchBuildConfigList) as "k8s.io/apimachinery/pkg/runtime".Object value in argument to scheme.AddKnownTypes: *MultiArchBuildConfigList does not implement "k8s.io/apimachinery/pkg/runtime".Object (missing method DeepCopyObject)
(typecheck)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/prowgen/jobbase.go` around lines 24 - 50, fromRepositorySet and
sparseCheckoutFiles currently dereference configSpec without a nil guard and
will panic if called with nil; add an explicit nil check at the start of
fromRepositorySet (return false if configSpec == nil) and at the start of
sparseCheckoutFiles (return empty slice if configSpec == nil) so callers like
sparseCheckoutFiles -> fromRepositorySet (which references
cioperatorapi.CIOperatorInrepoConfigFileName) are safe when configSpec is nil.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/prowgen/jobbase.go`:
- Around line 102-109: The current logic sets SkipCloning based solely on
sparseCheckoutFiles(configSpec) which causes periodic jobs (which clone via
extra_refs) to lose skip_cloning; update the assignment so periodics preserve
skip_cloning: detect periodic jobs using the job info available in this scope
(e.g., inspect info or the job type on the job struct) and only set
b.base.UtilityConfig.DecorationConfig.SkipCloning = utilpointer.Bool(true) for
non-periodic jobs when len(sparseFiles) == 0, or otherwise do not override an
existing SkipCloning for periodic jobs; keep the other branches (setting
OauthTokenSecret for private jobs and assigning SparseCheckoutFiles) unchanged
and reference sparseCheckoutFiles(configSpec), shouldSkipCloning,
b.base.UtilityConfig.DecorationConfig.SkipCloning and extra_refs when making the
conditional change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 98016f9d-1c86-4e8b-bd16-ab8e1c430104
📒 Files selected for processing (35)
cmd/ci-operator-prowgen/testdata/zz_fixture_postsubmit_TestFromCIOperatorConfigToProwYaml_Custom_test_timeout.yamlcmd/ci-operator-prowgen/testdata/zz_fixture_postsubmit_TestFromCIOperatorConfigToProwYaml_Input_is_YAML_and_it_is_correctly_processed.yamlcmd/ci-operator-prowgen/testdata/zz_fixture_postsubmit_TestFromCIOperatorConfigToProwYaml_Using_a_variant_config__one_test_and_images__one_existing_job._Expect_one_presubmit__pre_post_submit_images_jobs._Existing_job_should_not_be_changed..yamlcmd/ci-operator-prowgen/testdata/zz_fixture_postsubmit_TestFromCIOperatorConfigToProwYaml_one_test_and_images__no_previous_jobs._Expect_test_presubmit__pre_post_submit_images_jobs.yamlcmd/ci-operator-prowgen/testdata/zz_fixture_presubmit_TestFromCIOperatorConfigToProwYaml_Custom_test_timeout.yamlcmd/ci-operator-prowgen/testdata/zz_fixture_presubmit_TestFromCIOperatorConfigToProwYaml_Input_is_YAML_and_it_is_correctly_processed.yamlcmd/ci-operator-prowgen/testdata/zz_fixture_presubmit_TestFromCIOperatorConfigToProwYaml_Using_a_variant_config__one_test_and_images__one_existing_job._Expect_one_presubmit__pre_post_submit_images_jobs._Existing_job_should_not_be_changed..yamlcmd/ci-operator-prowgen/testdata/zz_fixture_presubmit_TestFromCIOperatorConfigToProwYaml_one_test_and_images__no_previous_jobs._Expect_test_presubmit__pre_post_submit_images_jobs.yamlpkg/prowgen/jobbase.gopkg/prowgen/jobbase_test.gopkg/prowgen/prowgen_test.gopkg/prowgen/testdata/zz_fixture_TestGenerateJobs_Promotion_configuration_causes_promote_job_with_unique_targets.yamlpkg/prowgen/testdata/zz_fixture_TestGenerateJobs_promotion_postsubmit_and_periodic_.yamlpkg/prowgen/testdata/zz_fixture_TestGeneratePeriodicForTest_periodic_with_from_repository_build_root__ExtraRefs_has_sparse_checkout_files.yamlpkg/prowgen/testdata/zz_fixture_TestGeneratePeriodicForTest_periodic_with_from_repository_build_root_and_images__ExtraRefs_has_sparse_checkout_files_with_dockerfiles.yamlpkg/prowgen/testdata/zz_fixture_TestNewProwJobBaseBuilderForTest_simple_container_based_test_with_timeout_and_no_decoration.yamlpkg/prowgen/testdata/zz_fixture_TestProwJobBaseBuilder_job_with_from_repository_build_root__has_sparse_checkout_with_.ci_operator.yaml.yamlpkg/prowgen/testdata/zz_fixture_TestProwJobBaseBuilder_job_with_from_repository_build_root_and_images__adds_dockerfiles_to_sparse_checkout.yamlpkg/prowgen/testdata/zz_fixture_TestProwJobBaseBuilder_job_with_image_builds_in_of_openshift_release_main__does_not_have_no_builds__label.yamlpkg/prowgen/testdata/zz_fixture_TestProwJobBaseBuilder_private_job_with_cloning__including_podspec.yamltest/integration/ci-operator-prowgen/output/jobs/private-org/super/private-org-super-master-presubmits.yamltest/integration/ci-operator-prowgen/output/jobs/private/duper/private-duper-master-periodics.yamltest/integration/ci-operator-prowgen/output/jobs/private/duper/private-duper-master-postsubmits.yamltest/integration/ci-operator-prowgen/output/jobs/private/duper/private-duper-master-presubmits.yamltest/integration/ci-operator-prowgen/output/jobs/super/duper/super-duper-master-periodics.yamltest/integration/ci-operator-prowgen/output/jobs/super/duper/super-duper-master-postsubmits.yamltest/integration/ci-operator-prowgen/output/jobs/super/duper/super-duper-master-presubmits.yamltest/integration/ci-operator-prowgen/output/jobs/super/duper/super-duper-master-removed-promotion-presubmits.yamltest/integration/ci-operator-prowgen/output/jobs/super/duper/super-duper-release-3.11-postsubmits.yamltest/integration/ci-operator-prowgen/output/jobs/super/duper/super-duper-release-3.11-presubmits.yamltest/integration/ci-operator-prowgen/output/jobs/super/duper/super-duper-release-4.19-periodics.yamltest/integration/repo-init/expected/ci-operator/jobs/openshift/ci-tools/openshift-ci-tools-master-postsubmits.yamltest/integration/repo-init/expected/ci-operator/jobs/openshift/ci-tools/openshift-ci-tools-master-presubmits.yamltest/integration/repo-init/expected/ci-operator/jobs/openshift/origin/openshift-origin-main-postsubmits.yamltest/integration/repo-init/expected/ci-operator/jobs/openshift/origin/openshift-origin-main-presubmits.yaml
✅ Files skipped from review due to trivial changes (15)
- test/integration/ci-operator-prowgen/output/jobs/private-org/super/private-org-super-master-presubmits.yaml
- pkg/prowgen/testdata/zz_fixture_TestProwJobBaseBuilder_private_job_with_cloning__including_podspec.yaml
- pkg/prowgen/testdata/zz_fixture_TestProwJobBaseBuilder_job_with_image_builds_in_of_openshift_release_main__does_not_have_no_builds__label.yaml
- pkg/prowgen/testdata/zz_fixture_TestGenerateJobs_promotion_postsubmit_and_periodic_.yaml
- test/integration/repo-init/expected/ci-operator/jobs/openshift/ci-tools/openshift-ci-tools-master-postsubmits.yaml
- pkg/prowgen/testdata/zz_fixture_TestGenerateJobs_Promotion_configuration_causes_promote_job_with_unique_targets.yaml
- pkg/prowgen/testdata/zz_fixture_TestNewProwJobBaseBuilderForTest_simple_container_based_test_with_timeout_and_no_decoration.yaml
- cmd/ci-operator-prowgen/testdata/zz_fixture_presubmit_TestFromCIOperatorConfigToProwYaml_Input_is_YAML_and_it_is_correctly_processed.yaml
- test/integration/ci-operator-prowgen/output/jobs/super/duper/super-duper-master-removed-promotion-presubmits.yaml
- pkg/prowgen/testdata/zz_fixture_TestGeneratePeriodicForTest_periodic_with_from_repository_build_root__ExtraRefs_has_sparse_checkout_files.yaml
- cmd/ci-operator-prowgen/testdata/zz_fixture_presubmit_TestFromCIOperatorConfigToProwYaml_one_test_and_images__no_previous_jobs._Expect_test_presubmit__pre_post_submit_images_jobs.yaml
- cmd/ci-operator-prowgen/testdata/zz_fixture_presubmit_TestFromCIOperatorConfigToProwYaml_Using_a_variant_config__one_test_and_images__one_existing_job._Expect_one_presubmit__pre_post_submit_images_jobs._Existing_job_should_not_be_changed..yaml
- pkg/prowgen/testdata/zz_fixture_TestGeneratePeriodicForTest_periodic_with_from_repository_build_root_and_images__ExtraRefs_has_sparse_checkout_files_with_dockerfiles.yaml
- cmd/ci-operator-prowgen/testdata/zz_fixture_presubmit_TestFromCIOperatorConfigToProwYaml_Custom_test_timeout.yaml
- test/integration/ci-operator-prowgen/output/jobs/super/duper/super-duper-master-presubmits.yaml
🚧 Files skipped from review as they are similar to previous changes (5)
- pkg/prowgen/testdata/zz_fixture_TestProwJobBaseBuilder_job_with_from_repository_build_root__has_sparse_checkout_with_.ci_operator.yaml.yaml
- cmd/ci-operator-prowgen/testdata/zz_fixture_postsubmit_TestFromCIOperatorConfigToProwYaml_one_test_and_images__no_previous_jobs._Expect_test_presubmit__pre_post_submit_images_jobs.yaml
- cmd/ci-operator-prowgen/testdata/zz_fixture_postsubmit_TestFromCIOperatorConfigToProwYaml_Custom_test_timeout.yaml
- cmd/ci-operator-prowgen/testdata/zz_fixture_postsubmit_TestFromCIOperatorConfigToProwYaml_Input_is_YAML_and_it_is_correctly_processed.yaml
- pkg/prowgen/testdata/zz_fixture_TestProwJobBaseBuilder_job_with_from_repository_build_root_and_images__adds_dockerfiles_to_sparse_checkout.yaml
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/prowgen/jobbase.go`:
- Around line 40-67: The sparseCheckoutFiles function currently only adds
Dockerfile paths and ignores project-directory builds (see sparseCheckoutFiles,
cioperatorapi.ReleaseBuildConfiguration, Images.Items, DockerfilePath,
ContextDir) as well as BuildRootImage.ProjectImageBuild / BuildRootImages;
update it to include the full build context directory entries (the
image.ContextDir and all files under that context) for images that use COPY/ADD
or have ProjectImageBuild set, and if any BuildRootImage.ProjectImageBuild or
BuildRootImages[*].ProjectImageBuild is present, either add the repository
root/project-directory paths required by those builds to the sparse set or
signal a fallback to a full clone; ensure you still dedupe via the seen map and
preserve the existing behavior for images with DockerfileLiteral or image.Ref.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 1ff937fb-b9e9-4316-9c28-c025a73554d2
📒 Files selected for processing (45)
cmd/ci-operator-prowgen/testdata/zz_fixture_postsubmit_TestFromCIOperatorConfigToProwYaml_Custom_test_timeout.yamlcmd/ci-operator-prowgen/testdata/zz_fixture_postsubmit_TestFromCIOperatorConfigToProwYaml_Input_is_YAML_and_it_is_correctly_processed.yamlcmd/ci-operator-prowgen/testdata/zz_fixture_postsubmit_TestFromCIOperatorConfigToProwYaml_Using_a_variant_config__one_test_and_images__one_existing_job._Expect_one_presubmit__pre_post_submit_images_jobs._Existing_job_should_not_be_changed..yamlcmd/ci-operator-prowgen/testdata/zz_fixture_postsubmit_TestFromCIOperatorConfigToProwYaml_one_test_and_images__no_previous_jobs._Expect_test_presubmit__pre_post_submit_images_jobs.yamlcmd/ci-operator-prowgen/testdata/zz_fixture_presubmit_TestFromCIOperatorConfigToProwYaml_Custom_test_timeout.yamlcmd/ci-operator-prowgen/testdata/zz_fixture_presubmit_TestFromCIOperatorConfigToProwYaml_Input_is_YAML_and_it_is_correctly_processed.yamlcmd/ci-operator-prowgen/testdata/zz_fixture_presubmit_TestFromCIOperatorConfigToProwYaml_Using_a_variant_config__one_test_and_images__one_existing_job._Expect_one_presubmit__pre_post_submit_images_jobs._Existing_job_should_not_be_changed..yamlcmd/ci-operator-prowgen/testdata/zz_fixture_presubmit_TestFromCIOperatorConfigToProwYaml_one_test_and_images__no_previous_jobs._Expect_test_presubmit__pre_post_submit_images_jobs.yamlpkg/prowgen/jobbase.gopkg/prowgen/jobbase_test.gopkg/prowgen/prowgen_test.gopkg/prowgen/testdata/zz_fixture_TestGenerateJobs_Promotion_configuration_causes_promote_job_with_unique_targets.yamlpkg/prowgen/testdata/zz_fixture_TestGenerateJobs_promotion_postsubmit_and_periodic_.yamlpkg/prowgen/testdata/zz_fixture_TestGeneratePeriodicForTest_periodic_with_from_repository_build_root__ExtraRefs_has_sparse_checkout_files.yamlpkg/prowgen/testdata/zz_fixture_TestGeneratePeriodicForTest_periodic_with_from_repository_build_root_and_images__ExtraRefs_has_sparse_checkout_files_with_dockerfiles.yamlpkg/prowgen/testdata/zz_fixture_TestNewProwJobBaseBuilderForTest_simple_container_based_test_with_timeout_and_no_decoration.yamlpkg/prowgen/testdata/zz_fixture_TestProwJobBaseBuilder_job_with_from_repository_build_root__has_sparse_checkout_with_.ci_operator.yaml.yamlpkg/prowgen/testdata/zz_fixture_TestProwJobBaseBuilder_job_with_from_repository_build_root_and_images__adds_dockerfiles_to_sparse_checkout.yamlpkg/prowgen/testdata/zz_fixture_TestProwJobBaseBuilder_job_with_image_builds_in_of_openshift_release_main__does_not_have_no_builds__label.yamlpkg/prowgen/testdata/zz_fixture_TestProwJobBaseBuilder_private_job_with_cloning__including_podspec.yamltest/integration/ci-operator-prowgen/output/jobs/private-org/super/private-org-super-master-presubmits.yamltest/integration/ci-operator-prowgen/output/jobs/private/duper/private-duper-master-periodics.yamltest/integration/ci-operator-prowgen/output/jobs/private/duper/private-duper-master-postsubmits.yamltest/integration/ci-operator-prowgen/output/jobs/private/duper/private-duper-master-presubmits.yamltest/integration/ci-operator-prowgen/output/jobs/super/duper/super-duper-master-periodics.yamltest/integration/ci-operator-prowgen/output/jobs/super/duper/super-duper-master-postsubmits.yamltest/integration/ci-operator-prowgen/output/jobs/super/duper/super-duper-master-presubmits.yamltest/integration/ci-operator-prowgen/output/jobs/super/duper/super-duper-master-removed-promotion-presubmits.yamltest/integration/ci-operator-prowgen/output/jobs/super/duper/super-duper-release-3.11-postsubmits.yamltest/integration/ci-operator-prowgen/output/jobs/super/duper/super-duper-release-3.11-presubmits.yamltest/integration/ci-operator-prowgen/output/jobs/super/duper/super-duper-release-4.19-periodics.yamltest/integration/pj-rehearse/candidate/ci-operator/jobs/super/duper/super-duper-ciop-cfg-change-presubmits.yamltest/integration/pj-rehearse/candidate/ci-operator/jobs/super/duper/super-duper-cluster-profile-presubmits.yamltest/integration/pj-rehearse/candidate/ci-operator/jobs/super/duper/super-duper-master-presubmits.yamltest/integration/pj-rehearse/candidate/ci-operator/jobs/super/trooper/super-trooper-master-periodics.yamltest/integration/pj-rehearse/candidate/ci-operator/jobs/super/trooper/super-trooper-master-presubmits.yamltest/integration/pj-rehearse/master/ci-operator/jobs/super/duper/super-duper-ciop-cfg-change-presubmits.yamltest/integration/pj-rehearse/master/ci-operator/jobs/super/duper/super-duper-cluster-profile-presubmits.yamltest/integration/pj-rehearse/master/ci-operator/jobs/super/duper/super-duper-master-presubmits.yamltest/integration/pj-rehearse/master/ci-operator/jobs/super/trooper/super-trooper-master-periodics.yamltest/integration/pj-rehearse/master/ci-operator/jobs/super/trooper/super-trooper-master-presubmits.yamltest/integration/repo-init/expected/ci-operator/jobs/openshift/ci-tools/openshift-ci-tools-master-postsubmits.yamltest/integration/repo-init/expected/ci-operator/jobs/openshift/ci-tools/openshift-ci-tools-master-presubmits.yamltest/integration/repo-init/expected/ci-operator/jobs/openshift/origin/openshift-origin-main-postsubmits.yamltest/integration/repo-init/expected/ci-operator/jobs/openshift/origin/openshift-origin-main-presubmits.yaml
✅ Files skipped from review due to trivial changes (18)
- test/integration/ci-operator-prowgen/output/jobs/super/duper/super-duper-master-removed-promotion-presubmits.yaml
- pkg/prowgen/testdata/zz_fixture_TestNewProwJobBaseBuilderForTest_simple_container_based_test_with_timeout_and_no_decoration.yaml
- pkg/prowgen/testdata/zz_fixture_TestProwJobBaseBuilder_private_job_with_cloning__including_podspec.yaml
- cmd/ci-operator-prowgen/testdata/zz_fixture_postsubmit_TestFromCIOperatorConfigToProwYaml_Input_is_YAML_and_it_is_correctly_processed.yaml
- test/integration/pj-rehearse/master/ci-operator/jobs/super/duper/super-duper-cluster-profile-presubmits.yaml
- cmd/ci-operator-prowgen/testdata/zz_fixture_presubmit_TestFromCIOperatorConfigToProwYaml_Input_is_YAML_and_it_is_correctly_processed.yaml
- pkg/prowgen/testdata/zz_fixture_TestGenerateJobs_Promotion_configuration_causes_promote_job_with_unique_targets.yaml
- pkg/prowgen/testdata/zz_fixture_TestGeneratePeriodicForTest_periodic_with_from_repository_build_root__ExtraRefs_has_sparse_checkout_files.yaml
- test/integration/repo-init/expected/ci-operator/jobs/openshift/origin/openshift-origin-main-presubmits.yaml
- test/integration/ci-operator-prowgen/output/jobs/private-org/super/private-org-super-master-presubmits.yaml
- pkg/prowgen/testdata/zz_fixture_TestGeneratePeriodicForTest_periodic_with_from_repository_build_root_and_images__ExtraRefs_has_sparse_checkout_files_with_dockerfiles.yaml
- test/integration/ci-operator-prowgen/output/jobs/super/duper/super-duper-release-3.11-presubmits.yaml
- pkg/prowgen/testdata/zz_fixture_TestProwJobBaseBuilder_job_with_image_builds_in_of_openshift_release_main__does_not_have_no_builds__label.yaml
- cmd/ci-operator-prowgen/testdata/zz_fixture_presubmit_TestFromCIOperatorConfigToProwYaml_Custom_test_timeout.yaml
- test/integration/ci-operator-prowgen/output/jobs/super/duper/super-duper-master-periodics.yaml
- cmd/ci-operator-prowgen/testdata/zz_fixture_presubmit_TestFromCIOperatorConfigToProwYaml_Using_a_variant_config__one_test_and_images__one_existing_job._Expect_one_presubmit__pre_post_submit_images_jobs._Existing_job_should_not_be_changed..yaml
- test/integration/ci-operator-prowgen/output/jobs/super/duper/super-duper-master-postsubmits.yaml
- cmd/ci-operator-prowgen/testdata/zz_fixture_presubmit_TestFromCIOperatorConfigToProwYaml_one_test_and_images__no_previous_jobs._Expect_test_presubmit__pre_post_submit_images_jobs.yaml
🚧 Files skipped from review as they are similar to previous changes (15)
- test/integration/ci-operator-prowgen/output/jobs/super/duper/super-duper-release-4.19-periodics.yaml
- pkg/prowgen/testdata/zz_fixture_TestProwJobBaseBuilder_job_with_from_repository_build_root__has_sparse_checkout_with_.ci_operator.yaml.yaml
- test/integration/repo-init/expected/ci-operator/jobs/openshift/origin/openshift-origin-main-postsubmits.yaml
- cmd/ci-operator-prowgen/testdata/zz_fixture_postsubmit_TestFromCIOperatorConfigToProwYaml_Custom_test_timeout.yaml
- pkg/prowgen/testdata/zz_fixture_TestGenerateJobs_promotion_postsubmit_and_periodic_.yaml
- pkg/prowgen/testdata/zz_fixture_TestProwJobBaseBuilder_job_with_from_repository_build_root_and_images__adds_dockerfiles_to_sparse_checkout.yaml
- cmd/ci-operator-prowgen/testdata/zz_fixture_postsubmit_TestFromCIOperatorConfigToProwYaml_one_test_and_images__no_previous_jobs._Expect_test_presubmit__pre_post_submit_images_jobs.yaml
- cmd/ci-operator-prowgen/testdata/zz_fixture_postsubmit_TestFromCIOperatorConfigToProwYaml_Using_a_variant_config__one_test_and_images__one_existing_job._Expect_one_presubmit__pre_post_submit_images_jobs._Existing_job_should_not_be_changed..yaml
- test/integration/ci-operator-prowgen/output/jobs/private/duper/private-duper-master-postsubmits.yaml
- test/integration/repo-init/expected/ci-operator/jobs/openshift/ci-tools/openshift-ci-tools-master-postsubmits.yaml
- test/integration/ci-operator-prowgen/output/jobs/super/duper/super-duper-release-3.11-postsubmits.yaml
- pkg/prowgen/prowgen_test.go
- test/integration/ci-operator-prowgen/output/jobs/private/duper/private-duper-master-periodics.yaml
- test/integration/ci-operator-prowgen/output/jobs/private/duper/private-duper-master-presubmits.yaml
- test/integration/ci-operator-prowgen/output/jobs/super/duper/super-duper-master-presubmits.yaml
Contributor
|
@Prucek: The following tests failed, say
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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
from_repositoryis set,.ci-operator.yamlis included in the sparse checkoutimagesstanza are included (with deduplication), respectingcontext_diranddockerfile_pathdockerfile_literalorrefare skipped (no file to clone)DecorationConfig.SparseCheckoutFileswhich prow applies to the primary ref for presubmits/postsubmits🤖 Generated with Claude Code
Summary by CodeRabbit
Improvements
Tests