feat(prowgen): propagate run_if_changed/skip_if_only_changed for images jobs#5009
Open
Prucek wants to merge 1 commit intoopenshift:mainfrom
Open
feat(prowgen): propagate run_if_changed/skip_if_only_changed for images jobs#5009Prucek 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 |
|
Important Review skippedAuto reviews are limited based on label configuration. 🚫 Review skipped — only excluded labels are configured. (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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:
WalkthroughReplace the flat Images slice on ReleaseBuildConfiguration with a new exported ImageConfiguration (holds Items plus image-level change-detection flags and BuildIfAffected); update all call sites, validation, defaults, deepcopy, CLI/registry logic, prow job generation/merging, and tests to use Images.Items and to propagate the new image-change fields. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
Contributor
|
Skipping CI for Draft Pull Request. |
openshift-ci
bot
added
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/prowgen/prowgen_test.go (1)
814-852: Add aImagesPipelineRunIfChangedregression case.This table now covers 3 of the 4 new image change-detection fields, but
ImagesPipelineRunIfChangedis still unexercised. A wiring typo inGenerateJobswould currently pass unnoticed because the pipeline-only coverage is limited toImagesPipelineSkipIfOnlyChanged.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/prowgen/prowgen_test.go` around lines 814 - 852, The test table in prowgen_test.go misses a case for ImagesPipelineRunIfChanged; add a new test entry (similar to the existing "images job with pipeline_skip_if_only_changed propagated to presubmit only" case) that sets ReleaseBuildConfiguration.ImagesPipelineRunIfChanged to a regex and uses the same ProwgenInfo metadata, so GenerateJobs is exercised for the pipeline-only "run if changed" field; ensure the test asserts that this field is propagated only to presubmit jobs (matching how ImagesPipelineSkipIfOnlyChanged is validated) to catch wiring typos in GenerateJobs.
🤖 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/jobconfig/files.go`:
- Around line 491-500: The compatibility block for "-images" jobs only copies
RunIfChanged/SkipIfOnlyChanged and AlwaysRun; update it to also preserve any
manual pipeline annotations (pipeline_run_if_changed and
pipeline_skip_if_only_changed) from old to merged when merged.RunIfChanged and
merged.SkipIfOnlyChanged are empty — i.e., if old has those annotation keys,
copy them into merged's Annotations map (creating it if nil) alongside copying
RunIfChanged/SkipIfOnlyChanged and setting AlwaysRun=false; also add a
merge/unit test exercising the migration path where ImagesPipeline* are empty in
the new config but the old job contains pipeline_run_if_changed /
pipeline_skip_if_only_changed to assert the merged job preserves those
annotations.
---
Nitpick comments:
In `@pkg/prowgen/prowgen_test.go`:
- Around line 814-852: The test table in prowgen_test.go misses a case for
ImagesPipelineRunIfChanged; add a new test entry (similar to the existing
"images job with pipeline_skip_if_only_changed propagated to presubmit only"
case) that sets ReleaseBuildConfiguration.ImagesPipelineRunIfChanged to a regex
and uses the same ProwgenInfo metadata, so GenerateJobs is exercised for the
pipeline-only "run if changed" field; ensure the test asserts that this field is
propagated only to presubmit jobs (matching how ImagesPipelineSkipIfOnlyChanged
is validated) to catch wiring typos in GenerateJobs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3f8cbbad-276a-41cd-b8a5-e2b93192af72
📒 Files selected for processing (8)
pkg/api/types.gopkg/jobconfig/files.gopkg/jobconfig/files_test.gopkg/prowgen/prowgen.gopkg/prowgen/prowgen_test.gopkg/prowgen/testdata/zz_fixture_TestGenerateJobs_images_job_with_pipeline_skip_if_only_changed_propagated_to_presubmit_only.yamlpkg/prowgen/testdata/zz_fixture_TestGenerateJobs_images_job_with_run_if_changed_propagated_to_presubmit_and_postsubmit.yamlpkg/prowgen/testdata/zz_fixture_TestGenerateJobs_images_job_with_skip_if_only_changed_propagated_to_presubmit_and_postsubmit.yaml
pkg/jobconfig/files.go
Outdated
Comment on lines
+491
to
+500
| // For images jobs: if prowgen didn't derive run/skip filters from the ci-operator config | ||
| // (i.e. the new job has no filters), preserve any manually-set filters from the existing | ||
| // job file. Once all repos set images_run_if_changed / images_skip_if_only_changed in | ||
| // their ci-operator config this workaround can be removed. | ||
| if strings.HasSuffix(merged.Name, "-images") { | ||
| if old.RunIfChanged != "" || old.SkipIfOnlyChanged != "" { | ||
| merged.RunIfChanged = old.RunIfChanged | ||
| merged.SkipIfOnlyChanged = old.SkipIfOnlyChanged | ||
| merged.AlwaysRun = false | ||
| if merged.RunIfChanged == "" && merged.SkipIfOnlyChanged == "" { | ||
| if old.RunIfChanged != "" || old.SkipIfOnlyChanged != "" { | ||
| merged.RunIfChanged = old.RunIfChanged | ||
| merged.SkipIfOnlyChanged = old.SkipIfOnlyChanged | ||
| merged.AlwaysRun = false |
There was a problem hiding this comment.
Preserve manual pipeline_*_if_changed annotations for existing images jobs.
This compatibility block only carries forward RunIfChanged / SkipIfOnlyChanged. Existing repos that still rely on manual pipeline_run_if_changed or pipeline_skip_if_only_changed annotations on the generated *-images presubmit will lose those second-stage filters on the next merge when the new ci-operator config leaves ImagesPipeline* empty. That changes job triggering behavior during rollout.
Suggested direction
if strings.HasSuffix(merged.Name, "-images") {
if merged.RunIfChanged == "" && merged.SkipIfOnlyChanged == "" {
if old.RunIfChanged != "" || old.SkipIfOnlyChanged != "" {
merged.RunIfChanged = old.RunIfChanged
merged.SkipIfOnlyChanged = old.SkipIfOnlyChanged
merged.AlwaysRun = false
}
}
+ if merged.Annotations["pipeline_run_if_changed"] == "" && merged.Annotations["pipeline_skip_if_only_changed"] == "" {
+ if old.Annotations["pipeline_run_if_changed"] != "" || old.Annotations["pipeline_skip_if_only_changed"] != "" {
+ if merged.Annotations == nil {
+ merged.Annotations = map[string]string{}
+ }
+ merged.Annotations["pipeline_run_if_changed"] = old.Annotations["pipeline_run_if_changed"]
+ merged.Annotations["pipeline_skip_if_only_changed"] = old.Annotations["pipeline_skip_if_only_changed"]
+ merged.AlwaysRun = false
+ }
+ }
}Please add a merge test for this migration path as well.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/jobconfig/files.go` around lines 491 - 500, The compatibility block for
"-images" jobs only copies RunIfChanged/SkipIfOnlyChanged and AlwaysRun; update
it to also preserve any manual pipeline annotations (pipeline_run_if_changed and
pipeline_skip_if_only_changed) from old to merged when merged.RunIfChanged and
merged.SkipIfOnlyChanged are empty — i.e., if old has those annotation keys,
copy them into merged's Annotations map (creating it if nil) alongside copying
RunIfChanged/SkipIfOnlyChanged and setting AlwaysRun=false; also add a
merge/unit test exercising the migration path where ImagesPipeline* are empty in
the new config but the old job contains pipeline_run_if_changed /
pipeline_skip_if_only_changed to assert the merged job preserves those
annotations.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
pkg/tool-detector/detector.go (1)
375-383:⚠️ Potential issue | 🟠 MajorDon't ignore repo-root images in
build_if_affected.The early empty-
ContextDirskip means images built from the repo root never become affected when onlyDockerfile/dockerfile_pathchanges. Withbuild_if_affectedenabled, that can incorrectly skip a required image build after its Dockerfile changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/tool-detector/detector.go` around lines 375 - 383, The loop currently skips images with an empty image.ContextDir which ignores repo-root images for build_if_affected; remove the early continue and treat an empty ContextDir as the repo root by initializing contextDir := image.ContextDir and only append a trailing slash when contextDir != "" (i.e. if contextDir != "" && !strings.HasSuffix(contextDir, "/") { contextDir += "/" }); keep the rest of the affected-path logic unchanged so Dockerfile/dockerfile_path changes in the repo root correctly mark those images as affected.pkg/prowgen/jobbase.go (1)
45-50:⚠️ Potential issue | 🟡 MinorBroaden the
no-buildspredicate to cover multi-ref build inputs.This still ignores
BuildRootImagesand the*_listbuild-command variants, soopenshift/release@mainconfigs that build via multi-ref inputs can still be mislabeled asci-operator.openshift.io/no-builds.Suggested fix
- if len(c.Images.Items) == 0 && c.BuildRootImage == nil && c.RpmBuildCommands == "" && c.TestBinaryBuildCommands == "" && c.BinaryBuildCommands == "" { + hasBuildRoot := c.BuildRootImage != nil || len(c.BuildRootImages) > 0 + hasBinaryBuild := c.BinaryBuildCommands != "" || len(c.BinaryBuildCommandsList) > 0 + hasTestBinaryBuild := c.TestBinaryBuildCommands != "" || len(c.TestBinaryBuildCommandsList) > 0 + hasRPMBuild := c.RpmBuildCommands != "" || len(c.RpmBuildCommandsList) > 0 + if len(c.Images.Items) == 0 && !hasBuildRoot && !hasRPMBuild && !hasTestBinaryBuild && !hasBinaryBuild { return true }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/prowgen/jobbase.go` around lines 45 - 50, The predicate that marks configs as "no-builds" only checks c.Images.Items, c.BuildRootImage and the singular command fields; update it to also consider multi-ref and plural fields so multi-ref build inputs aren't missed. Modify the conditional around c.Images.Items, c.BuildRootImage and the command checks to additionally test that len(c.BuildRootImages) == 0 and that the list variants (e.g. c.RpmBuildCommandsList, c.TestBinaryBuildCommandsList, c.BinaryBuildCommandsList) are either nil or empty; keep the existing string checks (c.RpmBuildCommands, c.TestBinaryBuildCommands, c.BinaryBuildCommands) and return true only when all images, build-root(s) and both singular and list command fields are empty/absent. Ensure you reference and update the same predicate in pkg/prowgen/jobbase.go where those symbols are used.pkg/api/types.go (1)
107-112:⚠️ Potential issue | 🟠 MajorThe advertised
images_*inputs won't bind to this schema.This refactor only adds
run_if_changed/skip_if_only_changed/ ... underimages, and turns the legacyimages: [...]list intoimages: {items: [...]}. A config that follows the PR objective and sets top-levelimages_run_if_changed,images_skip_if_only_changed, etc. will still leave these values unset, so this is a broader user-facing schema migration rather than the additive field change the feature describes.Also applies to: 2791-2817
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/api/types.go` around lines 107 - 112, The schema change broke binding for legacy and top-level image inputs: update the ImageConfiguration handling so it accepts both the old images: [...] array and the new images: {items: [...] } object, and also accept the advertised top-level images_* fields by adding top-level struct fields and copying them into Images if present. Concretely, modify the type ImageConfiguration (and the root config type that contains the Images ImageConfiguration field) to implement UnmarshalJSON to detect and parse either an array of image items or an object with items, and add top-level fields (e.g. ImagesRunIfChanged, ImagesSkipIfOnlyChanged, ImagesSkipIfOnlyChanged, etc.) with json tags matching the advertised inputs and populate Images.RunIfChanged / Images.SkipIfOnlyChanged from those top-level fields during unmarshalling or validation so both legacy and new configs bind correctly.pkg/registry/server/server.go (1)
368-380:⚠️ Potential issue | 🟠 MajorPreserve
ImageConfigurationmetadata during merge, not onlyItems.Line 380 appends image items, but image-level fields (
BuildIfAffected, run/skip regexes) are never merged, so merged configs can silently lose those settings.💡 Suggested fix
+ // Preserve image-level behavior flags from merged configs. + // BuildIfAffected should be true if any merged config enables it. + mergedConfig.Images.BuildIfAffected = mergedConfig.Images.BuildIfAffected || config.Images.BuildIfAffected + if mergedConfig.Images.RunIfChanged == "" { + mergedConfig.Images.RunIfChanged = config.Images.RunIfChanged + } + if mergedConfig.Images.SkipIfOnlyChanged == "" { + mergedConfig.Images.SkipIfOnlyChanged = config.Images.SkipIfOnlyChanged + } + if mergedConfig.Images.PipelineRunIfChanged == "" { + mergedConfig.Images.PipelineRunIfChanged = config.Images.PipelineRunIfChanged + } + if mergedConfig.Images.PipelineSkipIfOnlyChanged == "" { + mergedConfig.Images.PipelineSkipIfOnlyChanged = config.Images.PipelineSkipIfOnlyChanged + } + for i := range config.Images.Items { image := config.Images.Items[i] if image.From != "" { image.From = api.PipelineImageStreamTagReference(fmt.Sprintf("%s-%s", image.From, ref)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/registry/server/server.go` around lines 368 - 380, The merge loop only appends image items but loses image-level metadata (e.g., BuildIfAffected, Run/Skip regexes) from each ImageConfiguration; update the loop that iterates over config.Images.Items (and assigns to image := config.Images.Items[i]) to copy/merge the parent ImageConfiguration fields into the per-item image before appending to mergedConfig.Images.Items: explicitly transfer fields like BuildIfAffected, RunIfAffected/SkipIfAffected regexes, any image-level annotations or strategy settings, and preserve the original image.Name/From/To/Inputs modifications already present in the block so mergedConfig.Images.Items contains both the item and its metadata.
🧹 Nitpick comments (1)
pkg/prowgen/prowgen_test.go (1)
814-858: Add apipeline_run_if_changedpropagation test case as well.
PipelineRunIfChangedis part of the newImageConfigurationtrigger surface, but this table currently validates only the skip variant for pipeline mode.➕ Suggested test addition
+ { + id: "images job with pipeline_run_if_changed propagated to presubmit only", + config: &ciop.ReleaseBuildConfiguration{ + Images: ciop.ImageConfiguration{ + Items: []ciop.ProjectDirectoryImageBuildStepConfiguration{{}}, + PipelineRunIfChanged: `^(Dockerfile|src/)`, + }, + PromotionConfiguration: &ciop.PromotionConfiguration{}, + }, + repoInfo: &ProwgenInfo{Metadata: ciop.Metadata{ + Org: "organization", + Repo: "repository", + Branch: "branch", + }}, + },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/prowgen/prowgen_test.go` around lines 814 - 858, The test table in prowgen_test.go is missing a case for PipelineRunIfChanged; add a new test case entry mirroring the "images job with pipeline_skip_if_only_changed propagated to presubmit only" case but set ImageConfiguration.PipelineRunIfChanged to a regexp (e.g. `^(Dockerfile|src/)`) and keep PromotionConfiguration present and ProwgenInfo metadata the same; ensure the new case id is descriptive (e.g. "images job with pipeline_run_if_changed propagated to presubmit only") so the test validates that PipelineRunIfChanged is propagated to presubmits.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/repo-init/api.go`:
- Around line 470-471: The ContainerImages branch currently calls
validation.ValidateImages(context.AddField("images"), generated.Images.Items)
but discards its returned error slice and only validates generated.Images.Items;
change this to capture the returned []error (or validation result) from
validation.ValidateImages and propagate or aggregate those errors into the
endpoint response (e.g., append to the main validation error list or return a
4xx), and additionally iterate over generated.Images.Items to validate the new
per-image run/skip regex fields (or call a helper like
validation.ValidateImageRegex(image) for each item) so regex-specific validation
errors are included; update the ContainerImages case to collect and handle both
the ValidateImages result and the per-image run/skip regex validation results.
---
Outside diff comments:
In `@pkg/api/types.go`:
- Around line 107-112: The schema change broke binding for legacy and top-level
image inputs: update the ImageConfiguration handling so it accepts both the old
images: [...] array and the new images: {items: [...] } object, and also accept
the advertised top-level images_* fields by adding top-level struct fields and
copying them into Images if present. Concretely, modify the type
ImageConfiguration (and the root config type that contains the Images
ImageConfiguration field) to implement UnmarshalJSON to detect and parse either
an array of image items or an object with items, and add top-level fields (e.g.
ImagesRunIfChanged, ImagesSkipIfOnlyChanged, ImagesSkipIfOnlyChanged, etc.) with
json tags matching the advertised inputs and populate Images.RunIfChanged /
Images.SkipIfOnlyChanged from those top-level fields during unmarshalling or
validation so both legacy and new configs bind correctly.
In `@pkg/prowgen/jobbase.go`:
- Around line 45-50: The predicate that marks configs as "no-builds" only checks
c.Images.Items, c.BuildRootImage and the singular command fields; update it to
also consider multi-ref and plural fields so multi-ref build inputs aren't
missed. Modify the conditional around c.Images.Items, c.BuildRootImage and the
command checks to additionally test that len(c.BuildRootImages) == 0 and that
the list variants (e.g. c.RpmBuildCommandsList, c.TestBinaryBuildCommandsList,
c.BinaryBuildCommandsList) are either nil or empty; keep the existing string
checks (c.RpmBuildCommands, c.TestBinaryBuildCommands, c.BinaryBuildCommands)
and return true only when all images, build-root(s) and both singular and list
command fields are empty/absent. Ensure you reference and update the same
predicate in pkg/prowgen/jobbase.go where those symbols are used.
In `@pkg/registry/server/server.go`:
- Around line 368-380: The merge loop only appends image items but loses
image-level metadata (e.g., BuildIfAffected, Run/Skip regexes) from each
ImageConfiguration; update the loop that iterates over config.Images.Items (and
assigns to image := config.Images.Items[i]) to copy/merge the parent
ImageConfiguration fields into the per-item image before appending to
mergedConfig.Images.Items: explicitly transfer fields like BuildIfAffected,
RunIfAffected/SkipIfAffected regexes, any image-level annotations or strategy
settings, and preserve the original image.Name/From/To/Inputs modifications
already present in the block so mergedConfig.Images.Items contains both the item
and its metadata.
In `@pkg/tool-detector/detector.go`:
- Around line 375-383: The loop currently skips images with an empty
image.ContextDir which ignores repo-root images for build_if_affected; remove
the early continue and treat an empty ContextDir as the repo root by
initializing contextDir := image.ContextDir and only append a trailing slash
when contextDir != "" (i.e. if contextDir != "" &&
!strings.HasSuffix(contextDir, "/") { contextDir += "/" }); keep the rest of the
affected-path logic unchanged so Dockerfile/dockerfile_path changes in the repo
root correctly mark those images as affected.
---
Nitpick comments:
In `@pkg/prowgen/prowgen_test.go`:
- Around line 814-858: The test table in prowgen_test.go is missing a case for
PipelineRunIfChanged; add a new test case entry mirroring the "images job with
pipeline_skip_if_only_changed propagated to presubmit only" case but set
ImageConfiguration.PipelineRunIfChanged to a regexp (e.g. `^(Dockerfile|src/)`)
and keep PromotionConfiguration present and ProwgenInfo metadata the same;
ensure the new case id is descriptive (e.g. "images job with
pipeline_run_if_changed propagated to presubmit only") so the test validates
that PipelineRunIfChanged is propagated to presubmits.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6d56bdb0-853b-404c-b2fc-550ffa5ba6b3
⛔ Files ignored due to path filters (2)
pkg/api/zz_generated.deepcopy.gois excluded by!**/zz_generated*pkg/webreg/zz_generated.ci_operator_reference.gois excluded by!**/zz_generated*
📒 Files selected for processing (33)
cmd/ci-operator/main.gocmd/ci-operator/main_test.gocmd/registry-replacer/main.gocmd/registry-replacer/main_test.gocmd/repo-init/api.gocmd/repo-init/main.gopkg/api/config_test.gopkg/api/promotion.gopkg/api/types.gopkg/api/types_test.gopkg/defaults/defaults.gopkg/defaults/defaults_test.gopkg/image-graph-generator/operator.gopkg/jobconfig/files.gopkg/jobconfig/files_test.gopkg/promotion/promotion.gopkg/promotion/promotion_test.gopkg/prowgen/jobbase.gopkg/prowgen/jobbase_test.gopkg/prowgen/prowgen.gopkg/prowgen/prowgen_test.gopkg/prowgen/testdata/zz_fixture_TestGenerateJobs_images_job_with_pipeline_skip_if_only_changed_propagated_to_presubmit_only.yamlpkg/prowgen/testdata/zz_fixture_TestGenerateJobs_images_job_with_run_if_changed_propagated_to_presubmit_and_postsubmit.yamlpkg/prowgen/testdata/zz_fixture_TestGenerateJobs_images_job_with_skip_if_only_changed_propagated_to_presubmit_and_postsubmit.yamlpkg/registry/server/server.gopkg/steps/multi_stage/multi_stage_test.gopkg/steps/release/promote.gopkg/steps/release/promote_test.gopkg/tool-detector/detector.gopkg/tool-detector/detector_test.gopkg/validation/config.gopkg/validation/config_test.gopkg/validation/graph_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/jobconfig/files.go
- pkg/prowgen/testdata/zz_fixture_TestGenerateJobs_images_job_with_pipeline_skip_if_only_changed_propagated_to_presubmit_only.yaml
- pkg/jobconfig/files_test.go
Comment on lines
470
to
+471
| case ContainerImages: | ||
| validation.ValidateImages(context.AddField("images"), generated.Images) | ||
| validation.ValidateImages(context.AddField("images"), generated.Images.Items) |
There was a problem hiding this comment.
CONTAINER_IMAGES validation currently discards every error.
ValidateImages returns a slice, but the result is dropped here, so this endpoint reports success even for invalid image definitions. This path also only validates generated.Images.Items, so the new image-level run/skip regex fields need explicit validation as well.
Suggested fix
- validation.ValidateImages(context.AddField("images"), generated.Images.Items)
+ validationErrors = append(validationErrors, validation.ValidateImages(context.AddField("images"), generated.Images.Items)...)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/repo-init/api.go` around lines 470 - 471, The ContainerImages branch
currently calls validation.ValidateImages(context.AddField("images"),
generated.Images.Items) but discards its returned error slice and only validates
generated.Images.Items; change this to capture the returned []error (or
validation result) from validation.ValidateImages and propagate or aggregate
those errors into the endpoint response (e.g., append to the main validation
error list or return a 4xx), and additionally iterate over
generated.Images.Items to validate the new per-image run/skip regex fields (or
call a helper like validation.ValidateImageRegex(image) for each item) so
regex-specific validation errors are included; update the ContainerImages case
to collect and handle both the ValidateImages result and the per-image run/skip
regex validation results.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/prowgen/prowgen_test.go (1)
814-859: Add the missingImages.PipelineRunIfChangedcase here.This block now covers
RunIfChanged,SkipIfOnlyChanged, andPipelineSkipIfOnlyChanged, but not the other presubmit-only image matcher. The lower-level option tests won't catch a wiring regression insideGenerateJobs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/prowgen/prowgen_test.go` around lines 814 - 859, Add a test case covering Images.PipelineRunIfChanged so GenerateJobs' wiring for the presubmit-only image matcher is exercised: create a new table entry (similar to the existing PipelineSkipIfOnlyChanged case) that sets config: &ciop.ReleaseBuildConfiguration{Images: ciop.ImageConfiguration{Items: []ciop.ProjectDirectoryImageBuildStepConfiguration{{}}, PipelineRunIfChanged: `^(Dockerfile|src/)`}, PromotionConfiguration: &ciop.PromotionConfiguration{}}, and the same repoInfo metadata and id indicating it should be "propagated to presubmit only"; this mirrors the RunIfChanged/SkipIfOnlyChanged entries and ensures GenerateJobs handles Images.PipelineRunIfChanged correctly.cmd/registry-replacer/main.go (1)
601-627: Add one prune-path regression test for wrapper-level image fields.This update correctly rewrites only
Images.Items, but the current tests only populateItems. A future refactor that overwrites the wholeImageConfigurationand dropsRunIfChanged/SkipIfOnlyChanged/BuildIfAffectedwould still pass.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/registry-replacer/main.go` around lines 601 - 627, Add a regression test that ensures pruning only rewrites config.Images.Items and does not drop wrapper-level image fields (RunIfChanged, SkipIfOnlyChanged, BuildIfAffected) — create an Images.Items entry with those wrapper fields set plus Inputs/As that should be pruned, run the same prune-path logic that produces prunedImages (the code around config.Images.Items, image.Inputs and prunedImages), and assert after pruning that the Items slice is updated but each surviving ImageConfiguration still retains its RunIfChanged/SkipIfOnlyChanged/BuildIfAffected values; this will guard against future refactors that replace entire ImageConfiguration objects instead of only updating Items.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cmd/registry-replacer/main.go`:
- Around line 601-627: Add a regression test that ensures pruning only rewrites
config.Images.Items and does not drop wrapper-level image fields (RunIfChanged,
SkipIfOnlyChanged, BuildIfAffected) — create an Images.Items entry with those
wrapper fields set plus Inputs/As that should be pruned, run the same prune-path
logic that produces prunedImages (the code around config.Images.Items,
image.Inputs and prunedImages), and assert after pruning that the Items slice is
updated but each surviving ImageConfiguration still retains its
RunIfChanged/SkipIfOnlyChanged/BuildIfAffected values; this will guard against
future refactors that replace entire ImageConfiguration objects instead of only
updating Items.
In `@pkg/prowgen/prowgen_test.go`:
- Around line 814-859: Add a test case covering Images.PipelineRunIfChanged so
GenerateJobs' wiring for the presubmit-only image matcher is exercised: create a
new table entry (similar to the existing PipelineSkipIfOnlyChanged case) that
sets config: &ciop.ReleaseBuildConfiguration{Images:
ciop.ImageConfiguration{Items:
[]ciop.ProjectDirectoryImageBuildStepConfiguration{{}}, PipelineRunIfChanged:
`^(Dockerfile|src/)`}, PromotionConfiguration: &ciop.PromotionConfiguration{}},
and the same repoInfo metadata and id indicating it should be "propagated to
presubmit only"; this mirrors the RunIfChanged/SkipIfOnlyChanged entries and
ensures GenerateJobs handles Images.PipelineRunIfChanged correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c323a992-a4fe-4dc9-9c7e-5b7435a0629f
⛔ Files ignored due to path filters (2)
pkg/api/zz_generated.deepcopy.gois excluded by!**/zz_generated*pkg/webreg/zz_generated.ci_operator_reference.gois excluded by!**/zz_generated*
📒 Files selected for processing (34)
cmd/ci-operator/main.gocmd/ci-operator/main_test.gocmd/registry-replacer/main.gocmd/registry-replacer/main_test.gocmd/repo-init/api.gocmd/repo-init/main.gopkg/api/config_test.gopkg/api/promotion.gopkg/api/types.gopkg/api/types_test.gopkg/defaults/defaults.gopkg/defaults/defaults_test.gopkg/image-graph-generator/operator.gopkg/jobconfig/files.gopkg/jobconfig/files_test.gopkg/promotion/promotion.gopkg/promotion/promotion_test.gopkg/prowgen/jobbase.gopkg/prowgen/jobbase_test.gopkg/prowgen/prowgen.gopkg/prowgen/prowgen_test.gopkg/prowgen/testdata/zz_fixture_TestGenerateJobs_images_job_with_pipeline_skip_if_only_changed_propagated_to_presubmit_only.yamlpkg/prowgen/testdata/zz_fixture_TestGenerateJobs_images_job_with_run_if_changed_propagated_to_presubmit_and_postsubmit.yamlpkg/prowgen/testdata/zz_fixture_TestGenerateJobs_images_job_with_skip_if_only_changed_propagated_to_presubmit_and_postsubmit.yamlpkg/registry/server/server.gopkg/steps/multi_stage/multi_stage_test.gopkg/steps/release/promote.gopkg/steps/release/promote_test.gopkg/tool-detector/detector.gopkg/tool-detector/detector_test.gopkg/validation/config.gopkg/validation/config_test.gopkg/validation/graph_test.gopkg/validation/test.go
🚧 Files skipped from review as they are similar to previous changes (9)
- pkg/promotion/promotion.go
- pkg/prowgen/testdata/zz_fixture_TestGenerateJobs_images_job_with_pipeline_skip_if_only_changed_propagated_to_presubmit_only.yaml
- pkg/registry/server/server.go
- cmd/repo-init/api.go
- pkg/promotion/promotion_test.go
- pkg/jobconfig/files_test.go
- pkg/api/promotion.go
- cmd/repo-init/main.go
- pkg/validation/config.go
Member
Author
|
/retest |
1 similar comment
Member
Author
|
/retest |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pkg/jobconfig/files.go (1)
491-500:⚠️ Potential issue | 🟠 MajorPreserve manual
pipeline_*_if_changedannotations during-imagesjob migration.
pkg/api/types.go:2792-2816addsPipelineRunIfChanged/PipelineSkipIfOnlyChanged, but Line 496 only backfillsRunIfChanged/SkipIfOnlyChanged. Existing-imagespresubmits that still carry manual pipeline annotations will lose their second-stage filters on merge until those fields are migrated into ci-operator config.Suggested fix
if strings.HasSuffix(merged.Name, "-images") { if merged.RunIfChanged == "" && merged.SkipIfOnlyChanged == "" { if old.RunIfChanged != "" || old.SkipIfOnlyChanged != "" { merged.RunIfChanged = old.RunIfChanged merged.SkipIfOnlyChanged = old.SkipIfOnlyChanged merged.AlwaysRun = false } } + if merged.Annotations["pipeline_run_if_changed"] == "" && merged.Annotations["pipeline_skip_if_only_changed"] == "" { + if old.Annotations["pipeline_run_if_changed"] != "" || old.Annotations["pipeline_skip_if_only_changed"] != "" { + if merged.Annotations == nil { + merged.Annotations = map[string]string{} + } + merged.Annotations["pipeline_run_if_changed"] = old.Annotations["pipeline_run_if_changed"] + merged.Annotations["pipeline_skip_if_only_changed"] = old.Annotations["pipeline_skip_if_only_changed"] + merged.AlwaysRun = false + } + } }Please add a merge test for this migration path as well.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/jobconfig/files.go` around lines 491 - 500, The migration for "-images" jobs only backfills RunIfChanged/SkipIfOnlyChanged (seen in the merged object handling where merged.Name is checked) but ignores the newer PipelineRunIfChanged/PipelineSkipIfOnlyChanged fields, so manual pipeline_*_if_changed annotations are dropped; update the logic that runs when strings.HasSuffix(merged.Name, "-images") to also copy old.PipelineRunIfChanged and old.PipelineSkipIfOnlyChanged into merged.PipelineRunIfChanged and merged.PipelineSkipIfOnlyChanged (in addition to RunIfChanged/SkipIfOnlyChanged and AlwaysRun=false) when the merged pipeline fields are empty and the old ones are non-empty, and add a unit test exercising this migration path to ensure both RunIfChanged/SkipIfOnlyChanged and PipelineRunIfChanged/PipelineSkipIfOnlyChanged are preserved from old to merged.
🧹 Nitpick comments (2)
pkg/jobconfig/files_test.go (1)
431-465: Mirror these matcher-merge cases inTestMergePostsubmits.
pkg/prowgen/prowgen.goLines 154-157 now apply the sameRegexpChangeMatcherfields to image postsubmits. Adding preserve/override cases there too would protect the regenerate/merge path on both job types, not just presubmits.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/jobconfig/files_test.go` around lines 431 - 465, Add the same two RegexpChangeMatcher merge test cases currently in TestMergePresubmits into TestMergePostsubmits: (1) preserve an old manual SkipIfOnlyChanged when the new postsubmit has no filter, and (2) prefer the prowgen-generated SkipIfOnlyChanged over an old manual value. Ensure the new tests use the same struct shapes and expected prowconfig.Presubmit/Presubmit-like objects (matching the existing cases referencing RegexpChangeMatcher and SkipIfOnlyChanged) so the postsubmit merge behavior mirrors the presubmit behavior implemented in prowgen.go where RegexpChangeMatcher fields are applied to image postsubmits.cmd/repo-init/main.go (1)
680-680: Carry the full image wrapper through repo-init.This only forwards
Items, sorepo-init --configstill can't represent the new image-level fields (run_if_changed,skip_if_only_changed,pipeline_*,build_if_affected). Usingapi.ImageConfigurationend-to-end here would keep the initializer aligned with the public config shape while still accepting the legacy array form.Possible cleanup
type initConfig struct { Org string `json:"org"` Repo string `json:"repo"` Branch string `json:"branch"` BaseImages map[string]api.ImageStreamTagReference `json:"base_images"` - Images []api.ProjectDirectoryImageBuildStepConfiguration `json:"images"` + Images api.ImageConfiguration `json:"images"` CanonicalGoRepository string `json:"canonical_go_repository"` Promotes bool `json:"promotes"` PromotesWithOpenShift bool `json:"promotes_with_openshift"` NeedsBase bool `json:"needs_base"` NeedsOS bool `json:"needs_os"` @@ - generated.Configuration.Images.Items = config.Images + generated.Configuration.Images = config.Images🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/repo-init/main.go` at line 680, The initializer currently assigns only config.Images into generated.Configuration.Images.Items, losing image-level metadata; update the flow to carry the full api.ImageConfiguration wrapper end-to-end instead of just the Items slice: change the initializer to accept/produce api.ImageConfiguration (use api.ImageConfiguration for the config parsing and for generated.Configuration.Images), map legacy plain-slice input into the wrapper when needed (so --config still accepts old array form), and ensure any code paths referencing generated.Configuration.Images.Items are updated to use the wrapper fields (e.g., run_if_changed, skip_if_only_changed, pipeline_*, build_if_affected) so those image-level fields are preserved through repo-init.
🤖 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/webreg/zz_generated.ci_operator_reference.go`:
- Around line 248-250: The generated doc incorrectly states skip_if_only_changed
applies to postsubmits; update the generator/template that produces
pkg/webreg/zz_generated.ci_operator_reference.go so the comment for
"skip_if_only_changed" explicitly documents it as presubmit-only (matching the
Go type StopIfOnlyChanged/SkipIfOnlyChanged defined in pkg/api/types.go and the
field documented in lines ~2792-2816), or adjust the generator logic to detect
that SkipIfOnlyChanged is defined only for auto-generated-image presubmits and
emit text stating "presubmit-only" instead of implying postsubmit support;
ensure the unique symbol names SkipIfOnlyChanged (pkg/api/types.go) and the
generated comment block in zz_generated.ci_operator_reference.go are aligned.
---
Duplicate comments:
In `@pkg/jobconfig/files.go`:
- Around line 491-500: The migration for "-images" jobs only backfills
RunIfChanged/SkipIfOnlyChanged (seen in the merged object handling where
merged.Name is checked) but ignores the newer
PipelineRunIfChanged/PipelineSkipIfOnlyChanged fields, so manual
pipeline_*_if_changed annotations are dropped; update the logic that runs when
strings.HasSuffix(merged.Name, "-images") to also copy old.PipelineRunIfChanged
and old.PipelineSkipIfOnlyChanged into merged.PipelineRunIfChanged and
merged.PipelineSkipIfOnlyChanged (in addition to RunIfChanged/SkipIfOnlyChanged
and AlwaysRun=false) when the merged pipeline fields are empty and the old ones
are non-empty, and add a unit test exercising this migration path to ensure both
RunIfChanged/SkipIfOnlyChanged and
PipelineRunIfChanged/PipelineSkipIfOnlyChanged are preserved from old to merged.
---
Nitpick comments:
In `@cmd/repo-init/main.go`:
- Line 680: The initializer currently assigns only config.Images into
generated.Configuration.Images.Items, losing image-level metadata; update the
flow to carry the full api.ImageConfiguration wrapper end-to-end instead of just
the Items slice: change the initializer to accept/produce api.ImageConfiguration
(use api.ImageConfiguration for the config parsing and for
generated.Configuration.Images), map legacy plain-slice input into the wrapper
when needed (so --config still accepts old array form), and ensure any code
paths referencing generated.Configuration.Images.Items are updated to use the
wrapper fields (e.g., run_if_changed, skip_if_only_changed, pipeline_*,
build_if_affected) so those image-level fields are preserved through repo-init.
In `@pkg/jobconfig/files_test.go`:
- Around line 431-465: Add the same two RegexpChangeMatcher merge test cases
currently in TestMergePresubmits into TestMergePostsubmits: (1) preserve an old
manual SkipIfOnlyChanged when the new postsubmit has no filter, and (2) prefer
the prowgen-generated SkipIfOnlyChanged over an old manual value. Ensure the new
tests use the same struct shapes and expected
prowconfig.Presubmit/Presubmit-like objects (matching the existing cases
referencing RegexpChangeMatcher and SkipIfOnlyChanged) so the postsubmit merge
behavior mirrors the presubmit behavior implemented in prowgen.go where
RegexpChangeMatcher fields are applied to image postsubmits.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a8aad83c-8dd1-4b65-8955-a6deb37a141a
📒 Files selected for processing (39)
cmd/ci-operator/main.gocmd/ci-operator/main_test.gocmd/registry-replacer/main.gocmd/registry-replacer/main_test.gocmd/repo-init/api.gocmd/repo-init/main.gopkg/api/config_test.gopkg/api/promotion.gopkg/api/types.gopkg/api/types_test.gopkg/api/zz_generated.deepcopy.gopkg/defaults/defaults.gopkg/defaults/defaults_test.gopkg/image-graph-generator/operator.gopkg/jobconfig/files.gopkg/jobconfig/files_test.gopkg/promotion/promotion.gopkg/promotion/promotion_test.gopkg/prowgen/jobbase.gopkg/prowgen/jobbase_test.gopkg/prowgen/prowgen.gopkg/prowgen/prowgen_test.gopkg/prowgen/testdata/zz_fixture_TestGenerateJobs_images_job_with_pipeline_skip_if_only_changed_propagated_to_presubmit_only.yamlpkg/prowgen/testdata/zz_fixture_TestGenerateJobs_images_job_with_run_if_changed_propagated_to_presubmit_and_postsubmit.yamlpkg/prowgen/testdata/zz_fixture_TestGenerateJobs_images_job_with_skip_if_only_changed_propagated_to_presubmit_and_postsubmit.yamlpkg/registry/server/server.gopkg/steps/multi_stage/multi_stage_test.gopkg/steps/release/promote.gopkg/steps/release/promote_test.gopkg/tool-detector/detector.gopkg/tool-detector/detector_test.gopkg/validation/config.gopkg/validation/config_test.gopkg/validation/graph_test.gopkg/validation/test.gopkg/webreg/zz_generated.ci_operator_reference.gotest/integration/ci-operator-prowgen/input/config/super/duper/super-duper-master__images-changed.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
🚧 Files skipped from review as they are similar to previous changes (13)
- pkg/steps/release/promote.go
- pkg/validation/graph_test.go
- pkg/prowgen/jobbase.go
- cmd/ci-operator/main.go
- pkg/prowgen/prowgen_test.go
- pkg/image-graph-generator/operator.go
- pkg/defaults/defaults.go
- pkg/tool-detector/detector.go
- pkg/prowgen/testdata/zz_fixture_TestGenerateJobs_images_job_with_run_if_changed_propagated_to_presubmit_and_postsubmit.yaml
- cmd/registry-replacer/main.go
- pkg/prowgen/testdata/zz_fixture_TestGenerateJobs_images_job_with_skip_if_only_changed_propagated_to_presubmit_and_postsubmit.yaml
- cmd/repo-init/api.go
- pkg/validation/test.go
Comment on lines
+248
to
+250
| " # SkipIfOnlyChanged is a regex that will cause the auto-generated images\n" + | ||
| " # presubmit and postsubmit to be skipped if all changed files match the regex.\n" + | ||
| " skip_if_only_changed: ' '\n" + |
There was a problem hiding this comment.
Document skip_if_only_changed as presubmit-only.
Lines 248-250 say this applies to postsubmits, but pkg/api/types.go:2792-2816 defines SkipIfOnlyChanged only for auto-generated images presubmits. Please fix the source that generates this reference too, or the web docs will advertise unsupported postsubmit behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/webreg/zz_generated.ci_operator_reference.go` around lines 248 - 250, The
generated doc incorrectly states skip_if_only_changed applies to postsubmits;
update the generator/template that produces
pkg/webreg/zz_generated.ci_operator_reference.go so the comment for
"skip_if_only_changed" explicitly documents it as presubmit-only (matching the
Go type StopIfOnlyChanged/SkipIfOnlyChanged defined in pkg/api/types.go and the
field documented in lines ~2792-2816), or adjust the generator logic to detect
that SkipIfOnlyChanged is defined only for auto-generated-image presubmits and
emit text stating "presubmit-only" instead of implying postsubmit support;
ensure the unique symbol names SkipIfOnlyChanged (pkg/api/types.go) and the
generated comment block in zz_generated.ci_operator_reference.go are aligned.
Member
Author
|
/test lint |
Member
Author
|
/test e2e |
Contributor
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
droslean
reviewed
Mar 17, 2026
pkg/api/types.go
Outdated
|
|
||
| // MarshalJSON outputs a plain array when only Items is set (legacy format), | ||
| // otherwise outputs the full struct. | ||
| func (ic ImageConfiguration) MarshalJSON() ([]byte, error) { |
Member
There was a problem hiding this comment.
I don't think this is a proper way to keep backward compatibility. You should update the configuration changes to the release after this pr is merged. And also update the documentation.
| // BuildImagesIfAffected means images are only built if their corresponding cmd tools are affected by code changes. | ||
| // When enabled, the tool-detector package analyzes git changes to determine which images should be built. | ||
| // The image name (To field) should match the cmd tool name for this to work correctly. | ||
| BuildImagesIfAffected bool `json:"build_images_if_affected,omitempty"` |
jmguzik
reviewed
Mar 18, 2026
pkg/jobconfig/files.go
Outdated
| merged.RunIfChanged = old.RunIfChanged | ||
| merged.SkipIfOnlyChanged = old.SkipIfOnlyChanged | ||
| merged.AlwaysRun = false | ||
| if merged.RunIfChanged == "" && merged.SkipIfOnlyChanged == "" { |
Contributor
There was a problem hiding this comment.
will this section still be needed?
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 `@cmd/ci-operator/main.go`:
- Around line 1237-1238: Update the outdated comment that references the old
top-level key to reflect the new nested field structure: replace the obsolete
"build_images_if_affected" wording with something like "// build_if_affected is
enabled (under the images section)" immediately above the conditional that
checks config == nil || jobSpec == nil || !config.Images.BuildIfAffected so the
comment matches the actual field config.Images.BuildIfAffected used by the code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5319765c-2848-4c42-9e9e-0191c94d4408
📒 Files selected for processing (134)
cmd/ci-operator/main.gocmd/ci-operator/main_test.gocmd/private-org-peribolos-sync/testdata/ci-operator/config/org1/repo1/org1-repo1-main.yamlcmd/private-org-peribolos-sync/testdata/ci-operator/config/org1/repo2/org1-repo2-main.yamlcmd/private-org-peribolos-sync/testdata/ci-operator/config/org2/repo3/org2-repo3-main.yamlcmd/registry-replacer/main.gocmd/registry-replacer/main_test.gocmd/registry-replacer/testdata/zz_fixture_TestReplacer_Build_APIs_replacement_is_executed_first.yamlcmd/registry-replacer/testdata/zz_fixture_TestReplacer_ContextDir_is_respected.yamlcmd/registry-replacer/testdata/zz_fixture_TestReplacer_Context_dir_gets_fixed_up.yamlcmd/registry-replacer/testdata/zz_fixture_TestReplacer_Default_to_dockerfile.yamlcmd/registry-replacer/testdata/zz_fixture_TestReplacer_Dockerfile_gets_fixed_up.yamlcmd/registry-replacer/testdata/zz_fixture_TestReplacer_Existing_base_image_is_not_overwritten.yamlcmd/registry-replacer/testdata/zz_fixture_TestReplacer_Replaces_Copy_from.yamlcmd/registry-replacer/testdata/zz_fixture_TestReplacer_Replaces_with_tag.yamlcmd/registry-replacer/testdata/zz_fixture_TestReplacer_Replaces_without_tag.yamlcmd/registry-replacer/testdata/zz_fixture_TestReplacer_Unused_base_images_are_pruned.yamlcmd/registry-replacer/testdata/zz_fixture_TestReplacer_Use_dockerfile_literal_if_present.yamlcmd/repo-init/api.gocmd/repo-init/main.gohack/lint.shpkg/api/config_test.gopkg/api/promotion.gopkg/api/types.gopkg/api/types_test.gopkg/api/zz_generated.deepcopy.gopkg/config/testdata/cm/config/..2019_11_15_19_57_20.547184898/super-duper-master.yamlpkg/config/testdata/cm/config/..2019_11_15_19_57_20.547184898/super-duper-release-4.9.yamlpkg/config/testdata/tree/config/super/duper/super-duper-master.yamlpkg/config/testdata/tree/config/super/duper/super-duper-release-4.9.yamlpkg/defaults/defaults.gopkg/defaults/defaults_test.gopkg/image-graph-generator/operator.gopkg/jobconfig/files.gopkg/jobconfig/files_test.gopkg/promotion/promotion.gopkg/promotion/promotion_test.gopkg/prowgen/jobbase.gopkg/prowgen/jobbase_test.gopkg/prowgen/prowgen.gopkg/prowgen/prowgen_test.gopkg/prowgen/testdata/zz_fixture_TestGenerateJobs_images_job_with_pipeline_skip_if_only_changed_propagated_to_presubmit_only.yamlpkg/prowgen/testdata/zz_fixture_TestGenerateJobs_images_job_with_run_if_changed_propagated_to_presubmit_and_postsubmit.yamlpkg/prowgen/testdata/zz_fixture_TestGenerateJobs_images_job_with_skip_if_only_changed_propagated_to_presubmit_and_postsubmit.yamlpkg/registry/server/server.gopkg/registry/server/testdata/zz_fixture_TestConfig_getting_config_works.yamlpkg/registry/server/testdata/zz_fixture_TestConfigserved_config.yamlpkg/steps/multi_stage/multi_stage_test.gopkg/steps/release/promote.gopkg/steps/release/promote_test.gopkg/tool-detector/detector.gopkg/tool-detector/detector_test.gopkg/validation/config.gopkg/validation/config_test.gopkg/validation/graph_test.gopkg/validation/test.gopkg/validation/test_test.gopkg/webreg/zz_generated.ci_operator_reference.gotest/e2e/multi-stage/configs/master/test-another-master.yamltest/integration/ci-operator-config-mirror/input-to-clean/foo/bar/foo-bar-master.yamltest/integration/ci-operator-config-mirror/input-to-clean/foo/barry-white/foo-barry-white-master.yamltest/integration/ci-operator-config-mirror/input-to-clean/foo/barry-white/foo-barry-white-official.yamltest/integration/ci-operator-config-mirror/input-to-clean/foo/barry-white/foo-barry-white-official__releases.yamltest/integration/ci-operator-config-mirror/input-to-clean/super-priv/barry-white/super-priv-barry-white-master.yamltest/integration/ci-operator-config-mirror/input-to-clean/super-priv/barry-white/super-priv-barry-white-official.yamltest/integration/ci-operator-config-mirror/input-to-clean/super-priv/deprecated-repo/super-priv-deprecated-repo-master.yamltest/integration/ci-operator-config-mirror/input-to-clean/super-priv/duper/super-priv-duper-master.yamltest/integration/ci-operator-config-mirror/input-to-clean/super-priv/trooper/super-priv-trooper-deprecated.yamltest/integration/ci-operator-config-mirror/input-to-clean/super-priv/trooper/super-priv-trooper-master.yamltest/integration/ci-operator-config-mirror/input-to-clean/super/duper/super-duper-master.yamltest/integration/ci-operator-config-mirror/input-to-clean/super/looper/super-looper-master.yamltest/integration/ci-operator-config-mirror/input-to-clean/super/trooper/super-trooper-master.yamltest/integration/ci-operator-config-mirror/input/foo/bar/foo-bar-master.yamltest/integration/ci-operator-config-mirror/input/foo/barry-white/foo-barry-white-master.yamltest/integration/ci-operator-config-mirror/input/foo/barry-white/foo-barry-white-official.yamltest/integration/ci-operator-config-mirror/input/foo/barry-white/foo-barry-white-official__releases.yamltest/integration/ci-operator-config-mirror/input/super/duper/super-duper-master.yamltest/integration/ci-operator-config-mirror/input/super/looper/super-looper-master.yamltest/integration/ci-operator-config-mirror/input/super/trooper/super-trooper-master.yamltest/integration/ci-operator-config-mirror/output-only-super/foo/bar/foo-bar-master.yamltest/integration/ci-operator-config-mirror/output-only-super/foo/barry-white/foo-barry-white-master.yamltest/integration/ci-operator-config-mirror/output-only-super/foo/barry-white/foo-barry-white-official.yamltest/integration/ci-operator-config-mirror/output-only-super/foo/barry-white/foo-barry-white-official__releases.yamltest/integration/ci-operator-config-mirror/output-only-super/super-priv/barry-white/super-priv-barry-white-master.yamltest/integration/ci-operator-config-mirror/output-only-super/super-priv/barry-white/super-priv-barry-white-official.yamltest/integration/ci-operator-config-mirror/output-only-super/super-priv/barry-white/super-priv-barry-white-official__releases.yamltest/integration/ci-operator-config-mirror/output-only-super/super-priv/duper/super-priv-duper-master.yamltest/integration/ci-operator-config-mirror/output-only-super/super-priv/looper/super-priv-looper-master.yamltest/integration/ci-operator-config-mirror/output-only-super/super-priv/trooper/super-priv-trooper-master.yamltest/integration/ci-operator-config-mirror/output-only-super/super/duper/super-duper-master.yamltest/integration/ci-operator-config-mirror/output-only-super/super/looper/super-looper-master.yamltest/integration/ci-operator-config-mirror/output-only-super/super/trooper/super-trooper-master.yamltest/integration/ci-operator-config-mirror/output/foo/bar/foo-bar-master.yamltest/integration/ci-operator-config-mirror/output/foo/barry-white/foo-barry-white-master.yamltest/integration/ci-operator-config-mirror/output/foo/barry-white/foo-barry-white-official.yamltest/integration/ci-operator-config-mirror/output/foo/barry-white/foo-barry-white-official__releases.yamltest/integration/ci-operator-config-mirror/output/super-priv/barry-white/super-priv-barry-white-master.yamltest/integration/ci-operator-config-mirror/output/super-priv/barry-white/super-priv-barry-white-official.yamltest/integration/ci-operator-config-mirror/output/super-priv/barry-white/super-priv-barry-white-official__releases.yamltest/integration/ci-operator-config-mirror/output/super-priv/duper/super-priv-duper-master.yamltest/integration/ci-operator-config-mirror/output/super-priv/looper/super-priv-looper-master.yamltest/integration/ci-operator-config-mirror/output/super-priv/trooper/super-priv-trooper-master.yamltest/integration/ci-operator-config-mirror/output/super/duper/super-duper-master.yamltest/integration/ci-operator-config-mirror/output/super/looper/super-looper-master.yamltest/integration/ci-operator-config-mirror/output/super/trooper/super-trooper-master.yamltest/integration/ci-operator-configresolver/configs/master/openshift-console-master.yamltest/integration/ci-operator-configresolver/configs/release-4.2/openshift-installer-release-4.2.yamltest/integration/ci-operator-configresolver/expected/openshift-installer-console-merged-release-4.2-injected.jsontest/integration/ci-operator-configresolver/expected/openshift-installer-console-merged-release-4.2-installer-as-base.jsontest/integration/ci-operator-configresolver/expected/openshift-installer-release-4.2-injected.jsontest/integration/ci-operator-configresolver/expected/openshift-installer-release-4.2.jsontest/integration/ci-operator-prowgen/input/config/private/duper/private-duper-master.yamltest/integration/ci-operator-prowgen/input/config/super/duper/super-duper-master-removed-promotion.yamltest/integration/ci-operator-prowgen/input/config/super/duper/super-duper-master.yamltest/integration/ci-operator-prowgen/input/config/super/duper/super-duper-master__images-build-affected.yamltest/integration/ci-operator-prowgen/input/config/super/duper/super-duper-master__images-changed.yamltest/integration/ci-operator-prowgen/input/config/super/duper/super-duper-master__images-pipeline-changed.yamltest/integration/ci-operator-prowgen/input/config/super/duper/super-duper-master__images-skip-docs.yamltest/integration/ci-operator-prowgen/input/config/super/duper/super-duper-master__variant.yamltest/integration/ci-operator-prowgen/input/config/super/duper/super-duper-release-3.11.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/pj-rehearse/candidate/ci-operator/config/super/duper/super-duper-ciop-cfg-change.yamltest/integration/pj-rehearse/candidate/ci-operator/config/super/duper/super-duper-cluster-profile.yamltest/integration/pj-rehearse/candidate/ci-operator/config/super/duper/super-duper-master.yamltest/integration/pj-rehearse/candidate/ci-operator/config/super/trooper/super-trooper-master.yamltest/integration/pj-rehearse/master/ci-operator/config/super/duper/super-duper-ciop-cfg-change.yamltest/integration/pj-rehearse/master/ci-operator/config/super/duper/super-duper-cluster-profile.yamltest/integration/pj-rehearse/master/ci-operator/config/super/duper/super-duper-master.yamltest/integration/pj-rehearse/master/ci-operator/config/super/trooper/super-trooper-master.yamltest/integration/repo-init/expected/ci-operator/config/openshift/ci-tools/openshift-ci-tools-master.yamltest/integration/repo-init/expected/ci-operator/config/openshift/origin/openshift-origin-main.yamltest/integration/repo-init/input/ci-operator/config/openshift/ci-tools/openshift-ci-tools-master.yamltest/integration/repo-init/input/ci-operator/config/openshift/origin/openshift-origin-main.yaml
💤 Files with no reviewable changes (1)
- pkg/jobconfig/files.go
✅ Files skipped from review due to trivial changes (65)
- cmd/private-org-peribolos-sync/testdata/ci-operator/config/org1/repo1/org1-repo1-main.yaml
- pkg/config/testdata/cm/config/..2019_11_15_19_57_20.547184898/super-duper-release-4.9.yaml
- cmd/registry-replacer/testdata/zz_fixture_TestReplacer_ContextDir_is_respected.yaml
- cmd/registry-replacer/testdata/zz_fixture_TestReplacer_Context_dir_gets_fixed_up.yaml
- cmd/private-org-peribolos-sync/testdata/ci-operator/config/org1/repo2/org1-repo2-main.yaml
- test/integration/ci-operator-config-mirror/input-to-clean/foo/barry-white/foo-barry-white-master.yaml
- test/integration/ci-operator-config-mirror/output-only-super/super-priv/barry-white/super-priv-barry-white-official.yaml
- pkg/config/testdata/tree/config/super/duper/super-duper-master.yaml
- cmd/private-org-peribolos-sync/testdata/ci-operator/config/org2/repo3/org2-repo3-main.yaml
- test/integration/pj-rehearse/master/ci-operator/config/super/duper/super-duper-master.yaml
- test/integration/ci-operator-config-mirror/input-to-clean/super/looper/super-looper-master.yaml
- test/integration/ci-operator-config-mirror/output-only-super/super-priv/looper/super-priv-looper-master.yaml
- test/integration/ci-operator-config-mirror/input-to-clean/super-priv/trooper/super-priv-trooper-master.yaml
- cmd/registry-replacer/testdata/zz_fixture_TestReplacer_Dockerfile_gets_fixed_up.yaml
- test/integration/ci-operator-config-mirror/input/foo/barry-white/foo-barry-white-master.yaml
- test/integration/ci-operator-config-mirror/input-to-clean/foo/barry-white/foo-barry-white-official.yaml
- hack/lint.sh
- test/integration/ci-operator-config-mirror/input/foo/barry-white/foo-barry-white-official__releases.yaml
- test/integration/ci-operator-config-mirror/output-only-super/super-priv/duper/super-priv-duper-master.yaml
- test/integration/ci-operator-config-mirror/output-only-super/foo/barry-white/foo-barry-white-official__releases.yaml
- test/integration/ci-operator-config-mirror/output/foo/barry-white/foo-barry-white-master.yaml
- test/integration/ci-operator-config-mirror/output/super-priv/looper/super-priv-looper-master.yaml
- test/integration/ci-operator-config-mirror/input/foo/bar/foo-bar-master.yaml
- test/integration/ci-operator-config-mirror/output-only-super/super/duper/super-duper-master.yaml
- pkg/image-graph-generator/operator.go
- pkg/config/testdata/cm/config/..2019_11_15_19_57_20.547184898/super-duper-master.yaml
- test/integration/ci-operator-config-mirror/input-to-clean/super/duper/super-duper-master.yaml
- test/integration/ci-operator-prowgen/input/config/super/duper/super-duper-master-removed-promotion.yaml
- test/integration/ci-operator-config-mirror/output/foo/barry-white/foo-barry-white-official.yaml
- test/integration/ci-operator-configresolver/configs/release-4.2/openshift-installer-release-4.2.yaml
- test/integration/ci-operator-prowgen/input/config/super/duper/super-duper-master.yaml
- test/integration/ci-operator-config-mirror/input-to-clean/super-priv/duper/super-priv-duper-master.yaml
- test/integration/ci-operator-config-mirror/output-only-super/foo/barry-white/foo-barry-white-master.yaml
- test/integration/ci-operator-config-mirror/output/foo/barry-white/foo-barry-white-official__releases.yaml
- test/integration/ci-operator-configresolver/configs/master/openshift-console-master.yaml
- test/integration/ci-operator-config-mirror/input/super/trooper/super-trooper-master.yaml
- test/integration/ci-operator-config-mirror/output/super-priv/barry-white/super-priv-barry-white-master.yaml
- test/integration/pj-rehearse/master/ci-operator/config/super/duper/super-duper-cluster-profile.yaml
- test/integration/ci-operator-config-mirror/output-only-super/super/looper/super-looper-master.yaml
- test/integration/pj-rehearse/candidate/ci-operator/config/super/duper/super-duper-cluster-profile.yaml
- test/integration/ci-operator-config-mirror/input-to-clean/super-priv/barry-white/super-priv-barry-white-official.yaml
- test/integration/pj-rehearse/candidate/ci-operator/config/super/duper/super-duper-master.yaml
- test/integration/ci-operator-config-mirror/output/super-priv/barry-white/super-priv-barry-white-official.yaml
- test/integration/pj-rehearse/candidate/ci-operator/config/super/trooper/super-trooper-master.yaml
- test/integration/repo-init/expected/ci-operator/config/openshift/origin/openshift-origin-main.yaml
- test/integration/ci-operator-config-mirror/output/super/trooper/super-trooper-master.yaml
- pkg/prowgen/testdata/zz_fixture_TestGenerateJobs_images_job_with_skip_if_only_changed_propagated_to_presubmit_and_postsubmit.yaml
- test/integration/ci-operator-config-mirror/input-to-clean/foo/bar/foo-bar-master.yaml
- test/integration/ci-operator-config-mirror/input-to-clean/super-priv/trooper/super-priv-trooper-deprecated.yaml
- test/integration/ci-operator-config-mirror/output-only-super/super-priv/trooper/super-priv-trooper-master.yaml
- pkg/prowgen/testdata/zz_fixture_TestGenerateJobs_images_job_with_pipeline_skip_if_only_changed_propagated_to_presubmit_only.yaml
- test/integration/ci-operator-config-mirror/output-only-super/super/trooper/super-trooper-master.yaml
- cmd/ci-operator/main_test.go
- test/integration/ci-operator-prowgen/input/config/super/duper/super-duper-master__images-pipeline-changed.yaml
- test/integration/ci-operator-prowgen/input/config/super/duper/super-duper-master__images-skip-docs.yaml
- test/integration/ci-operator-prowgen/input/config/super/duper/super-duper-master__images-changed.yaml
- test/integration/repo-init/input/ci-operator/config/openshift/ci-tools/openshift-ci-tools-master.yaml
- pkg/validation/config_test.go
- test/integration/ci-operator-prowgen/input/config/super/duper/super-duper-master__images-build-affected.yaml
- test/integration/pj-rehearse/candidate/ci-operator/config/super/duper/super-duper-ciop-cfg-change.yaml
- test/integration/ci-operator-config-mirror/input/super/looper/super-looper-master.yaml
- pkg/prowgen/testdata/zz_fixture_TestGenerateJobs_images_job_with_run_if_changed_propagated_to_presubmit_and_postsubmit.yaml
- pkg/defaults/defaults.go
- test/integration/pj-rehearse/master/ci-operator/config/super/trooper/super-trooper-master.yaml
- cmd/registry-replacer/testdata/zz_fixture_TestReplacer_Replaces_Copy_from.yaml
🚧 Files skipped from review as they are similar to previous changes (16)
- pkg/steps/release/promote.go
- pkg/tool-detector/detector.go
- pkg/api/promotion.go
- pkg/promotion/promotion.go
- cmd/repo-init/api.go
- pkg/prowgen/jobbase_test.go
- pkg/jobconfig/files_test.go
- pkg/validation/graph_test.go
- pkg/prowgen/jobbase.go
- pkg/steps/release/promote_test.go
- pkg/prowgen/prowgen_test.go
- test/integration/ci-operator-prowgen/output/jobs/super/duper/super-duper-master-postsubmits.yaml
- pkg/steps/multi_stage/multi_stage_test.go
- pkg/validation/test.go
- pkg/registry/server/server.go
- cmd/registry-replacer/main.go
Comment on lines
+1237
to
1238
| if config == nil || jobSpec == nil || !config.Images.BuildIfAffected { | ||
| return nil |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== remaining legacy field references ==="
rg -n -C1 'build_images_if_affected|BuildImagesIfAffected'
echo
echo "=== current nested field references ==="
rg -n -C1 'build_if_affected|BuildIfAffected'Repository: openshift/ci-tools
Length of output: 3203
Update the comment at line 1235 to reflect the new nested field structure.
The migration to config.Images.BuildIfAffected is complete. The old top-level build_images_if_affected key is not used anywhere in the codebase, all test data and example configs have been updated to the new nested structure, and the runtime logic correctly accesses the field from its new location. However, the comment at line 1235 still references the obsolete field name. Update it to: // build_if_affected is enabled (under the images section) or similar to match the actual structure.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/ci-operator/main.go` around lines 1237 - 1238, Update the outdated
comment that references the old top-level key to reflect the new nested field
structure: replace the obsolete "build_images_if_affected" wording with
something like "// build_if_affected is enabled (under the images section)"
immediately above the conditional that checks config == nil || jobSpec == nil ||
!config.Images.BuildIfAffected so the comment matches the actual field
config.Images.BuildIfAffected used by the code.
Member
|
/lgtm |
openshift-ci
bot
added
the
do-not-merge/hold
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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 |
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
4 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.
Uh oh!
There was an error while loading. Please reload this page.