fix(prowgen): disable sparse checkout when build_if_affected is enabled#5228
Conversation
The build_if_affected feature uses the Go packages loader to determine which cmd tools are affected by code changes, but sparse checkout only checks out Dockerfiles. This caused packages.Load to fail with "directory not found" errors, silently falling back to building all images and defeating the purpose of build_if_affected. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
📝 WalkthroughWalkthroughThe PR extends sparse checkout disabling behavior to include ChangesSparse checkout disabled for BuildIfAffected
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 16 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (16 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/prowgen/jobbase.go (1)
100-100: ⚡ Quick winAdd inline comment explaining BuildIfAffected sparse checkout dependency.
The relationship between
BuildIfAffectedand sparse checkout is non-obvious. Add a brief comment explaining thatBuildIfAffectedrequires the full Go source tree forpackages.Loadanalysis, not just Dockerfiles.📝 Suggested comment
+ // BuildIfAffected requires full Go source tree for packages.Load, not just Dockerfiles disableSparseCheckout := (configSpec.Prowgen != nil && configSpec.Prowgen.DisableSparseCheckout) || configSpec.Images.BuildIfAffectedAs per coding guidelines, "Comment important exported functions with their purpose, parameters, and return values. Do not add inline comments explaining what code does; only why when non-obvious."
🤖 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` at line 100, The logic setting disableSparseCheckout combines Prowgen.DisableSparseCheckout and Images.BuildIfAffected but lacks an inline explanation; add a brief inline comment above the disableSparseCheckout assignment that explains why Images.BuildIfAffected forces sparse checkout to be disabled — specifically: BuildIfAffected requires the full Go source tree (not just Dockerfiles) for packages.Load analysis, so sparse checkout must be disabled when configSpec.Images.BuildIfAffected is true; reference the disableSparseCheckout variable, configSpec.Images.BuildIfAffected, and packages.Load in the comment.
🤖 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/jobbase.go`:
- Line 100: The logic setting disableSparseCheckout combines
Prowgen.DisableSparseCheckout and Images.BuildIfAffected but lacks an inline
explanation; add a brief inline comment above the disableSparseCheckout
assignment that explains why Images.BuildIfAffected forces sparse checkout to be
disabled — specifically: BuildIfAffected requires the full Go source tree (not
just Dockerfiles) for packages.Load analysis, so sparse checkout must be
disabled when configSpec.Images.BuildIfAffected is true; reference the
disableSparseCheckout variable, configSpec.Images.BuildIfAffected, and
packages.Load in the comment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 62d90041-1c22-46bb-9c65-ef52bfc2e269
📒 Files selected for processing (5)
pkg/prowgen/jobbase.gopkg/prowgen/jobbase_test.gopkg/prowgen/testdata/zz_fixture_TestProwJobBaseBuilder_job_with_images_and_build_if_affected_disables_sparse_checkout.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.yaml
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deepsm007, droslean, 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 |
|
/test e2e |
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
|
/override ci/prow/e2e |
|
@Prucek: Overrode contexts on behalf of Prucek: ci/prow/e2e 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: The following test 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. |
ae2ca0d
into
openshift:main
Summary
build_if_affectedrequires the full Go source tree to runpackages.Loadfor tool detectionpackages.Loadto fail with "directory not found" errors, silently falling back to building all images — defeating the purpose ofbuild_if_affectedimages.build_if_affectedis true🤖 Generated with Claude Code