Utilize clonerefs sparse checkout#5161
Open
Prucek wants to merge 4 commits intoopenshift:mainfrom
Open
Conversation
Contributor
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
|
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:
WalkthroughDerives sparse-checkout file lists from ReleaseBuildConfiguration, sets DecorationConfig.SparseCheckoutFiles (or SkipCloning when empty), propagates sparse lists into generated Refs, clears sparse lists for rehearsal/build-time, and updates many YAML fixtures/outputs to use sparse_checkout_files instead of skip_cloning. ChangesSparse-checkout migration
sequenceDiagram
participant CIO as ci-operator config
participant PG as prowgen (pkg/prowgen)
participant REFS as Refs (prowv1.Refs / ExtraRefs)
participant BUILD as clonerefs / createBuild
participant REHEARSE as rehearse maker
CIO->>PG: input ReleaseBuildConfiguration
PG->>PG: compute sparseFiles via sparseCheckoutFiles()
PG->>REFS: attach ref.SparseCheckoutFiles when generating ExtraRefs
REFS->>BUILD: refs passed into createBuild()
note right of BUILD: createBuild clears refs[*].SparseCheckoutFiles before clonerefs encoding
PG->>REHEARSE: when making rehearsal presubmit, clear DecorationConfig.SparseCheckoutFiles
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
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 |
openshift-ci
Bot
added
the
approved
Prucek
changed the title
Prucek
changed the title
Prucek
changed the title
Member
Author
|
/label tide/merge-method-squash |
openshift-ci
Bot
added
the
tide/merge-method-squash
Prucek
force-pushed
the
sparse-checkout-fixes
branch
from
4bc1753 to
58a15fb
Compare
![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 (1)
pkg/prowgen/jobbase.go (1)
95-104: Verify theopenshift/releaseconsumer before merging.This changes prowgen output from
skip_cloningtosparse_checkout_files. The linkedopenshift/releasefindings still show validation/generation code paths keyed onskip_cloning, so please runmake jobsand the release-side validators there to make sure this schema change does not break the downstream repo.As per coding guidelines, "
pkg/prowgen/**: Changes here require runningmake jobsin the openshift/release repo to verify no unexpected diffs appear".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/prowgen/jobbase.go` around lines 95 - 104, This change replaces setting DecorationConfig.SkipCloning with DecorationConfig.SparseCheckoutFiles; before merging, run the downstream verification: pull the openshift/release repo, run make jobs and the release-side validators to ensure no unexpected diffs or validation errors; if failures occur, either preserve backward compatibility by still setting b.base.UtilityConfig.DecorationConfig.SkipCloning when sparseCheckoutFiles is empty (in the same block that calls sparseCheckoutFiles) or update the release-side generation/validation to accept SparseCheckoutFiles instead of SkipCloning (check the places that consume SkipCloning in release validators and job generation).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@cmd/ci-operator-prowgen/testdata/zz_fixture_presubmit_TestFromCIOperatorConfigToProwYaml_Input_is_YAML_and_it_is_correctly_processed.yaml`:
- Around line 66-67: The fixture sets image-style sparse checkout for a
non-image unit presubmit: remove or change the sparse_checkout_files entry that
lists "Dockerfile" for the job with "--target=unit" so the unit presubmit is not
limited to image files; specifically locate the YAML block that contains
sparse_checkout_files and the job metadata referencing "--target=unit" and
either delete the sparse_checkout_files key or replace it with a general
checkout (or an appropriate non-image file list) so the fixture matches the
"image-build jobs only" intent.
---
Nitpick comments:
In `@pkg/prowgen/jobbase.go`:
- Around line 95-104: This change replaces setting DecorationConfig.SkipCloning
with DecorationConfig.SparseCheckoutFiles; before merging, run the downstream
verification: pull the openshift/release repo, run make jobs and the
release-side validators to ensure no unexpected diffs or validation errors; if
failures occur, either preserve backward compatibility by still setting
b.base.UtilityConfig.DecorationConfig.SkipCloning when sparseCheckoutFiles is
empty (in the same block that calls sparseCheckoutFiles) or update the
release-side generation/validation to accept SparseCheckoutFiles instead of
SkipCloning (check the places that consume SkipCloning in release validators and
job generation).
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 78c043d6-fdce-416d-a3dc-28ccfbb28c15
📒 Files selected for processing (17)
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/prowgen.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_TestNewProwJobBaseBuilderForTest_simple_container_based_test_with_timeout_and_no_decoration.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.yamlpkg/rehearse/jobs.gopkg/steps/source.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/prowgen/prowgen.go
- pkg/prowgen/testdata/zz_fixture_TestGenerateJobs_Promotion_configuration_causes_promote_job_with_unique_targets.yaml
- cmd/ci-operator-prowgen/testdata/zz_fixture_presubmit_TestFromCIOperatorConfigToProwYaml_Custom_test_timeout.yaml
Comment on lines
+66
to
+67
| sparse_checkout_files: | ||
| - Dockerfile |
There was a problem hiding this comment.
Non-image unit presubmit should not be sparse-checked out to only Dockerfile.
At Line 66-Line 67, this fixture sets image-style sparse checkout on a --target=unit job, which conflicts with the PR objective (“image-build jobs only”) and can encode incorrect expected output.
Proposed fixture correction
decoration_config:
- sparse_checkout_files:
- - Dockerfile
+ skip_cloning: true📝 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
| sparse_checkout_files: | |
| - Dockerfile | |
| skip_cloning: true |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@cmd/ci-operator-prowgen/testdata/zz_fixture_presubmit_TestFromCIOperatorConfigToProwYaml_Input_is_YAML_and_it_is_correctly_processed.yaml`
around lines 66 - 67, The fixture sets image-style sparse checkout for a
non-image unit presubmit: remove or change the sparse_checkout_files entry that
lists "Dockerfile" for the job with "--target=unit" so the unit presubmit is not
limited to image files; specifically locate the YAML block that contains
sparse_checkout_files and the job metadata referencing "--target=unit" and
either delete the sparse_checkout_files key or replace it with a general
checkout (or an appropriate non-image file list) so the fixture matches the
"image-build jobs only" intent.
Member
Author
|
/test e2e |
openshift-ci
Bot
added
the
do-not-merge/hold
Prucek
force-pushed
the
sparse-checkout-fixes
branch
from
58a15fb to
51bef27
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/prowgen/prowgen.go (1)
40-222: Operational:make jobsinopenshift/releasewill change thousands of job files — validate downstream scripts first.From the linked
openshift/releaserepo, these scripts appear to hardcode or validateskip_cloning: trueand may break when prowgen starts emittingsparse_checkout_filesinstead:
hack/validate_slack_report_template.go— contains"skip_cloning: true"expectationhack/validate-yaml-indentation.py— validatesskip_cloningkey type asmappinghack/generate-periodic-build-origin-release-image-jobs.sh— emitsskip_cloning: trueConfirm those scripts are updated before or alongside running
make jobsinopenshift/release.As per coding guidelines: "Changes here require running
make jobsin the openshift/release repo to verify no unexpected diffs appear."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/prowgen/prowgen.go` around lines 40 - 222, GenerateJobs may start emitting sparse_checkout_files where callers downstream expect skip_cloning: true; before changing job output (via GenerateJobs, GeneratePeriodicForTest, generatePresubmitForTest, generatePostsubmitForTest) update and validate the downstream openshift/release validation/generation scripts that hardcode or type-check skip_cloning (e.g., the scripts referenced in the review) so they accept sparse_checkout_files or the new format, then run make jobs in openshift/release and confirm no unexpected diffs; ensure tests/CI that parse presubmit/postsubmit/periodic YAMLs are updated to accept the new key and add a migration or compatibility layer in GenerateJobs if immediate downstream changes aren’t possible.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pkg/prowgen/prowgen.go`:
- Around line 40-222: GenerateJobs may start emitting sparse_checkout_files
where callers downstream expect skip_cloning: true; before changing job output
(via GenerateJobs, GeneratePeriodicForTest, generatePresubmitForTest,
generatePostsubmitForTest) update and validate the downstream openshift/release
validation/generation scripts that hardcode or type-check skip_cloning (e.g.,
the scripts referenced in the review) so they accept sparse_checkout_files or
the new format, then run make jobs in openshift/release and confirm no
unexpected diffs; ensure tests/CI that parse presubmit/postsubmit/periodic YAMLs
are updated to accept the new key and add a migration or compatibility layer in
GenerateJobs if immediate downstream changes aren’t possible.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 0373aed4-f9e4-4288-acc7-566b065db4b9
📒 Files selected for processing (22)
pkg/prowgen/prowgen.gotest/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.yaml
✅ Files skipped from review due to trivial changes (1)
- test/integration/pj-rehearse/candidate/ci-operator/jobs/super/duper/super-duper-master-presubmits.yaml
Replace skipCloning() with sparseCheckoutFiles() that computes the minimal set of files needed for image builds: .ci-operator.yaml (if from_repository is set) plus all Dockerfile paths from image configs. When sparse checkout files are available, set them on DecorationConfig instead of skipping cloning entirely. This allows prow to checkout only the files needed for image builds, significantly reducing clone time for large repositories. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Prucek
force-pushed
the
sparse-checkout-fixes
branch
from
51bef27 to
4301e26
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/prowgen/jobbase.go (1)
37-59:sets.Listensures deterministic output — consistent with prowgen requirementsThe sparse file list is built with
sets.New[string]()and emitted viasets.List(files), which returns a sorted slice. Same input always produces the same orderedSparseCheckoutFileslist.As per coding guidelines (
pkg/prowgen/**): "prowgen output must be deterministic — same input must always produce identical YAML."Reminder: after merging, run
make jobsin theopenshift/releaserepo to confirm no unexpected diffs appear in the generated job YAML. Per the linked-repos findings,openshift/releasestill has manyskip_cloning: trueentries in generated job files that will need to be updated.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/prowgen/jobbase.go` around lines 37 - 59, sparseCheckoutFiles must return a deterministic, sorted list — keep using sets.New[string]() to collect entries and return sets.List(files) (from the sparseCheckoutFiles function) so the output is consistently ordered; if any change replaced sets.List(files) with a non-deterministic conversion, restore it and, if helpful, add an inline comment near sparseCheckoutFiles mentioning determinism and the use of sets.List(files) (referencing cioperatorapi.CIOperatorInrepoConfigFileName and the configSpec *cioperatorapi.ReleaseBuildConfiguration) to prevent accidental future regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/rehearse/jobs.go`:
- Around line 169-171: The periodic-to-presubmit conversion leaves
DecorationConfig.SparseCheckoutFiles set on the rehearsal presubmit because
makeRehearsalPresubmit is called with repo="" and the later branch that clears
SparseCheckoutFiles (in makeRehearsalPresubmit) is never hit; to fix, after
calling ConvertPeriodicsToPresubmits/makeRehearsalPresubmit (in
ConvertPeriodicsToPresubmits), explicitly set the produced presubmit's
DecorationConfig.SparseCheckoutFiles = nil (but do not modify
ExtraRefs[0].SparseCheckoutFiles) so the rehearsal primary ref no longer carries
the periodic's sparse-checkout list. Ensure you target the presubmit object
returned by ConvertPeriodicsToPresubmits/makeRehearsalPresubmit and only clear
DecorationConfig.SparseCheckoutFiles there.
---
Nitpick comments:
In `@pkg/prowgen/jobbase.go`:
- Around line 37-59: sparseCheckoutFiles must return a deterministic, sorted
list — keep using sets.New[string]() to collect entries and return
sets.List(files) (from the sparseCheckoutFiles function) so the output is
consistently ordered; if any change replaced sets.List(files) with a
non-deterministic conversion, restore it and, if helpful, add an inline comment
near sparseCheckoutFiles mentioning determinism and the use of sets.List(files)
(referencing cioperatorapi.CIOperatorInrepoConfigFileName and the configSpec
*cioperatorapi.ReleaseBuildConfiguration) to prevent accidental future
regressions.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 1bcfebdf-4bca-4918-8ec4-ff3bb752a6e7
📒 Files selected for processing (38)
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/prowgen.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_TestNewProwJobBaseBuilderForTest_simple_container_based_test_with_timeout_and_no_decoration.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.yamlpkg/rehearse/jobs.gopkg/steps/source.gotest/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.yaml
✅ Files skipped from review due to trivial changes (19)
- pkg/prowgen/testdata/zz_fixture_TestProwJobBaseBuilder_private_job_with_cloning__including_podspec.yaml
- pkg/prowgen/testdata/zz_fixture_TestNewProwJobBaseBuilderForTest_simple_container_based_test_with_timeout_and_no_decoration.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_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
- pkg/steps/source.go
- 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/pj-rehearse/master/ci-operator/jobs/super/duper/super-duper-master-presubmits.yaml
- test/integration/pj-rehearse/master/ci-operator/jobs/super/duper/super-duper-ciop-cfg-change-presubmits.yaml
- cmd/ci-operator-prowgen/testdata/zz_fixture_postsubmit_TestFromCIOperatorConfigToProwYaml_Custom_test_timeout.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_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
- 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
- test/integration/ci-operator-prowgen/output/jobs/super/duper/super-duper-release-3.11-presubmits.yaml
- test/integration/ci-operator-prowgen/output/jobs/super/duper/super-duper-master-postsubmits.yaml
- test/integration/ci-operator-prowgen/output/jobs/super/duper/super-duper-master-periodics.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 (3)
- test/integration/pj-rehearse/candidate/ci-operator/jobs/super/duper/super-duper-cluster-profile-presubmits.yaml
- test/integration/pj-rehearse/master/ci-operator/jobs/super/duper/super-duper-cluster-profile-presubmits.yaml
- test/integration/ci-operator-prowgen/output/jobs/private/duper/private-duper-master-postsubmits.yaml
When a rehearsal job's primary ref (openshift/release) differs from the target repo, CompletePrimaryRefs propagates SparseCheckoutFiles from DecorationConfig to the extra ref (the original repo). However, the DecorationConfig.SparseCheckoutFiles remains set and prow applies it to the primary ref too, causing the release repo to be sparse-checked out — which breaks the clone with "unrelated histories" errors. Clear SparseCheckoutFiles from DecorationConfig after setting up the extra ref, since the files are already on the extra ref via CompletePrimaryRefs. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
When sparse checkout is active, clonerefs inside the src image build only checks out Dockerfiles, leaving the repo without source code. This breaks binary_build_commands (e.g. make all) and test steps that need the full source tree. Clear SparseCheckoutFiles from all refs before passing them to clonerefs in createBuild(), ensuring the src image always gets a full clone regardless of job-level sparse checkout settings. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Periodic jobs use extra_refs[0] as their primary ref instead of a top-level Refs field. DecorationConfig.SparseCheckoutFiles was not being propagated to this ref, so periodic jobs never used sparse checkout even when configured. Copy SparseCheckoutFiles from DecorationConfig to the periodic's extra_refs[0] before appending it. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Prucek
force-pushed
the
sparse-checkout-fixes
branch
from
4301e26 to
bbf5ac8
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
test/integration/pj-rehearse/candidate/ci-operator/jobs/super/duper/super-duper-ciop-cfg-change-presubmits.yaml (1)
43-45: Heads-up: openshift/release still expectsskip_cloningin several places.From the linked-repository findings, openshift/release contains validation scripts (
hack/validate_slack_report_template.go,hack/validate-yaml-indentation.py,hack/generate-periodic-build-origin-release-image-jobs.sh) and a large number of job YAMLs that still referenceskip_cloning. Once prowgen starts emittingsparse_checkout_filesin generated job specs, those validation scripts and pre-generated job files in openshift/release will need to be updated in lockstep to avoid validation failures and mismatches. This fixture change itself is fine, but the cross-repo coordination work is worth tracking explicitly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/integration/pj-rehearse/candidate/ci-operator/jobs/super/duper/super-duper-ciop-cfg-change-presubmits.yaml` around lines 43 - 45, The change replaces decoration_config: skip_cloning with sparse_checkout_files (see decoration_config and sparse_checkout_files in this YAML), but openshift/release still expects skip_cloning in many places; update the cross-repo consumers to avoid validation failures by coordinating changes: update validation scripts (hack/validate_slack_report_template.go, hack/validate-yaml-indentation.py, hack/generate-periodic-build-origin-release-image-jobs.sh) and any pre-generated job YAMLs in openshift/release to accept or migrate skip_cloning -> sparse_checkout_files (or add backward-compatible handling), and open a follow-up issue/PR in openshift/release to make the changes in lockstep with this fixture change.pkg/prowgen/prowgen.go (1)
450-469: Coordinateopenshift/releasevalidation scripts before rollout.The linked
openshift/releaserepository contains validation scripts and generation tooling that still referenceskip_cloning:
hack/validate_slack_report_template.go:87hack/validate-yaml-indentation.py:156hack/generate-periodic-build-origin-release-image-jobs.sh:18Once this generator ships, prowgen will emit
sparse_checkout_files(instead ofskip_cloning: true) for all image jobs. The validation scripts above will need to accommodate both the old format (for handcrafted jobs) and the new format (for generated jobs), and the generated job YAMLs stored inopenshift/releasewill need to be regenerated with the updated prowgen. Runningmake jobs(or equivalent) inopenshift/releaseafter merging this PR will be required.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/prowgen/prowgen.go` around lines 450 - 469, The change causes prowgen to emit sparse_checkout_files (via ref.SparseCheckoutFiles and base.UtilityConfig.DecorationConfig.SparseCheckoutFiles) instead of the old skip_cloning field, so update any validation and generation logic that checks for skip_cloning to accept both formats: when reading job specs, treat skip_cloning: true as equivalent to a non-empty sparse_checkout_files list and when validating/generated YAML (in tools that will consume prowgen output) ensure they accept sparse_checkout_files; also update any generator scripts that synthesize image jobs (the logic that previously wrote skip_cloning) to write sparse_checkout_files and re-run the jobs generation (make jobs) so openshift/release YAMLs are regenerated with the new field.test/integration/pj-rehearse/candidate/ci-operator/jobs/super/trooper/super-trooper-master-periodics.yaml (1)
52-53: Coordinate rollout withopenshift/releasevalidation scripts.Given linked-repo findings still showing
skip_cloning-centric checks, please confirm companion updates are merged there before broad job regeneration to avoid config/validator drift.Also applies to: 58-59
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/integration/pj-rehearse/candidate/ci-operator/jobs/super/trooper/super-trooper-master-periodics.yaml` around lines 52 - 53, This job adds sparse_checkout_files: - Dockerfile but the repo-wide validation in openshift/release still expects skip_cloning-centric checks; before regenerating or promoting these periodics, coordinate and ensure the companion validation changes in openshift/release are merged (update the validator rules that reference skip_cloning), verify the CI in openshift/release accepts the new sparse_checkout_files layout, and only then proceed with broad job regeneration so the new sparse_checkout_files (and Dockerfile usage) won't be rejected by the validator.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/integration/pj-rehearse/expected.yaml`:
- Around line 486-487: Remove the sparse_checkout_files entry from the rehearsed
job's primary ref block (where spec.refs is the openshift/release primary ref)
in the expected YAML so that sparse_checkout_files appears only under
extra_refs[0]; update the other duplicated occurrence as well so both places no
longer list sparse_checkout_files in the primary ref and it remains only on the
target repo in extra_refs[0].
---
Nitpick comments:
In `@pkg/prowgen/prowgen.go`:
- Around line 450-469: The change causes prowgen to emit sparse_checkout_files
(via ref.SparseCheckoutFiles and
base.UtilityConfig.DecorationConfig.SparseCheckoutFiles) instead of the old
skip_cloning field, so update any validation and generation logic that checks
for skip_cloning to accept both formats: when reading job specs, treat
skip_cloning: true as equivalent to a non-empty sparse_checkout_files list and
when validating/generated YAML (in tools that will consume prowgen output)
ensure they accept sparse_checkout_files; also update any generator scripts that
synthesize image jobs (the logic that previously wrote skip_cloning) to write
sparse_checkout_files and re-run the jobs generation (make jobs) so
openshift/release YAMLs are regenerated with the new field.
In
`@test/integration/pj-rehearse/candidate/ci-operator/jobs/super/duper/super-duper-ciop-cfg-change-presubmits.yaml`:
- Around line 43-45: The change replaces decoration_config: skip_cloning with
sparse_checkout_files (see decoration_config and sparse_checkout_files in this
YAML), but openshift/release still expects skip_cloning in many places; update
the cross-repo consumers to avoid validation failures by coordinating changes:
update validation scripts (hack/validate_slack_report_template.go,
hack/validate-yaml-indentation.py,
hack/generate-periodic-build-origin-release-image-jobs.sh) and any pre-generated
job YAMLs in openshift/release to accept or migrate skip_cloning ->
sparse_checkout_files (or add backward-compatible handling), and open a
follow-up issue/PR in openshift/release to make the changes in lockstep with
this fixture change.
In
`@test/integration/pj-rehearse/candidate/ci-operator/jobs/super/trooper/super-trooper-master-periodics.yaml`:
- Around line 52-53: This job adds sparse_checkout_files: - Dockerfile but the
repo-wide validation in openshift/release still expects skip_cloning-centric
checks; before regenerating or promoting these periodics, coordinate and ensure
the companion validation changes in openshift/release are merged (update the
validator rules that reference skip_cloning), verify the CI in openshift/release
accepts the new sparse_checkout_files layout, and only then proceed with broad
job regeneration so the new sparse_checkout_files (and Dockerfile usage) won't
be rejected by the validator.
🪄 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: e43030ab-e7a7-49cb-88b0-4808cbf9f84a
📒 Files selected for processing (25)
pkg/prowgen/prowgen.gopkg/rehearse/jobs.gopkg/steps/source.gotest/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/expected.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.yaml
✅ Files skipped from review due to trivial changes (7)
- test/integration/pj-rehearse/candidate/ci-operator/jobs/super/duper/super-duper-master-presubmits.yaml
- test/integration/ci-operator-prowgen/output/jobs/private-org/super/private-org-super-master-presubmits.yaml
- test/integration/ci-operator-prowgen/output/jobs/super/duper/super-duper-master-removed-promotion-presubmits.yaml
- test/integration/ci-operator-prowgen/output/jobs/super/duper/super-duper-release-3.11-postsubmits.yaml
- test/integration/ci-operator-prowgen/output/jobs/super/duper/super-duper-release-3.11-presubmits.yaml
- test/integration/ci-operator-prowgen/output/jobs/super/duper/super-duper-release-4.19-periodics.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 (6)
- pkg/rehearse/jobs.go
- test/integration/pj-rehearse/candidate/ci-operator/jobs/super/duper/super-duper-cluster-profile-presubmits.yaml
- test/integration/pj-rehearse/master/ci-operator/jobs/super/duper/super-duper-ciop-cfg-change-presubmits.yaml
- test/integration/ci-operator-prowgen/output/jobs/private/duper/private-duper-master-postsubmits.yaml
- test/integration/ci-operator-prowgen/output/jobs/private/duper/private-duper-master-periodics.yaml
- test/integration/pj-rehearse/master/ci-operator/jobs/super/trooper/super-trooper-master-presubmits.yaml
Comment on lines
+486
to
+487
| sparse_checkout_files: | ||
| - Dockerfile |
There was a problem hiding this comment.
Remove sparse checkout from rehearsal primary refs in this expected output.
For this rehearsed job, spec.refs is the openshift/release primary ref. Expecting sparse_checkout_files there reintroduces the leak this PR is meant to prevent; sparse checkout should stay on the target repo in extra_refs[0] only.
Suggested expected-output adjustment
grace_period: 15s
- sparse_checkout_files:
- - Dockerfile
timeout: 4h0m0s
@@
repo: release
- sparse_checkout_files:
- - DockerfileAlso applies to: 573-574
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/integration/pj-rehearse/expected.yaml` around lines 486 - 487, Remove
the sparse_checkout_files entry from the rehearsed job's primary ref block
(where spec.refs is the openshift/release primary ref) in the expected YAML so
that sparse_checkout_files appears only under extra_refs[0]; update the other
duplicated occurrence as well so both places no longer list
sparse_checkout_files in the primary ref and it remains only on the target repo
in extra_refs[0].
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
1 participant
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
.ci-operator.yaml+ Dockerfile pathsDecorationConfig.SparseCheckoutFilesleaked to the release repo primary ref, causing "unrelated histories" errorsbinary_build_commandsSparseCheckoutFileswas not propagated fromDecorationConfigtoextra_refs[0]Context
The original sparse checkout feature (353a4a0) was reverted in #5145 due to multiple bugs.
Upstream prow PR kubernetes-sigs/prow#639 added
SparseCheckoutFilessupporton
Refs, which is consumed by clonerefs to limit the working tree.How it works
sparseCheckoutFiles()computes the file list fromReleaseBuildConfiguration:.ci-operator.yamliffrom_repositoryis setdockerfile_pathentries (respectingcontext_dir, skippingdockerfile_literaland externalref)DecorationConfig.SparseCheckoutFilesinstead ofskip_cloning: trueskip_cloning: true(no change)pkg/steps/source.go) always clears sparse checkout to get a full cloneDecorationConfigwhen the primary ref differs from the target repoDecorationConfigtoextra_refs[0]Other job types verified safe
ExtraRefsare replaced with fresh refs;PeriodicSpec()doesn't callCompletePrimaryRefsExtraRefsare rebuilt;CompletePrimaryRefsonly applies to the primary refTesting
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes