CNTRLPLANE-3780: feat(prowgen): add pipeline_run_if_dockerfile_changed annotation#5300
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@celebdor: This pull request references CNTRLPLANE-3780 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. 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 openshift-eng/jira-lifecycle-plugin repository. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (9)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (8)
📝 WalkthroughWalkthroughThis PR adds a Dockerfile-aware pipeline trigger to test step configuration, propagates it into presubmit generation, and extends validation and tests for exclusivity and path checks. ChangesDockerfile-aware pipeline trigger
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 15 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: celebdor The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![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/api/types.go (1)
795-804: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winConsider guarding
Pathagainst traversal.
DockerfileEntry.Pathis a user-supplied relative path with no documented constraint against../segments. Since the value is later serialized into a job annotation and consumed by an external controller (per PR description), validation closer to the schema (e.g., inpkg/validation) should reject paths that escape the repo root, similar to canonicalization checks recommended for path-like fields.As per path instructions, "Path traversal: canonicalize paths, reject ../" for
**/*.{py,js,ts,go,rs,java,rb,php,kt,swift,cs}.🤖 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/api/types.go` around lines 795 - 804, Guard DockerfileEntry.Path against path traversal by adding validation near the schema/validation layer instead of trusting the raw field. Update the DockerfileEntry handling in pkg/api/types.go and the corresponding validation path in pkg/validation so user-supplied paths are canonicalized and any value containing ../ or otherwise escaping the repo root is rejected before serialization into the job annotation. Use DockerfileEntry and Path as the main symbols to locate the field and its validation.Source: Path instructions
🤖 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/prowgen/prowgen.go`:
- Around line 312-319: In the prowgen annotation setup, stop ignoring the
json.Marshal result for opts.pipelineRunIfDockerfileChanged and handle any
serialization failure explicitly. Update the block that writes
base.Annotations["pipeline_run_if_dockerfile_changed"] so it checks the marshal
error and returns it wrapped with fmt.Errorf, rather than silently continuing
with empty or invalid annotation data. Use the surrounding prowgen generation
logic and the pipelineOpt path to locate the fix.
---
Nitpick comments:
In `@pkg/api/types.go`:
- Around line 795-804: Guard DockerfileEntry.Path against path traversal by
adding validation near the schema/validation layer instead of trusting the raw
field. Update the DockerfileEntry handling in pkg/api/types.go and the
corresponding validation path in pkg/validation so user-supplied paths are
canonicalized and any value containing ../ or otherwise escaping the repo root
is rejected before serialization into the job annotation. Use DockerfileEntry
and Path as the main symbols to locate the field and its validation.
🪄 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: 2cd67e85-3c8f-42a1-9588-ca261d0e4863
⛔ Files ignored due to path filters (1)
pkg/webreg/zz_generated.ci_operator_reference.gois excluded by!**/zz_generated*
📒 Files selected for processing (9)
pkg/api/types.gopkg/prowgen/prowgen.gopkg/prowgen/prowgen_test.gopkg/prowgen/testdata/zz_fixture_TestGeneratePresubmitForTest_presubmit_with_always_run_but_pipeline_run_if_dockerfile_changed_set.yamlpkg/prowgen/testdata/zz_fixture_TestGeneratePresubmitForTest_presubmit_with_always_run_false_and_pipeline_run_if_dockerfile_changed.yamlpkg/validation/config.gopkg/validation/config_test.gopkg/validation/test.gopkg/validation/test_test.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift/release(manual)openshift/ci-docs(manual)openshift/release-controller(manual)openshift/ci-chat-bot(manual)
| if len(opts.pipelineRunIfDockerfileChanged) > 0 { | ||
| if base.Annotations == nil { | ||
| base.Annotations = make(map[string]string) | ||
| } | ||
| data, _ := json.Marshal(opts.pipelineRunIfDockerfileChanged) | ||
| base.Annotations["pipeline_run_if_dockerfile_changed"] = string(data) | ||
| pipelineOpt = true | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Don't discard the json.Marshal error.
data, _ := json.Marshal(opts.pipelineRunIfDockerfileChanged) swallows the error entirely. DockerfileEntry currently only holds strings, so this can't fail today, but silently ignoring it hides any future serialization break (e.g., if the type gains a non-marshalable field) — the annotation would just come out empty/wrong with no diagnostic.
As per path instructions, "Wrap errors with fmt.Errorf(...)"/error handling requirements for **/*.go files.
🐛 Proposed fix
if len(opts.pipelineRunIfDockerfileChanged) > 0 {
if base.Annotations == nil {
base.Annotations = make(map[string]string)
}
- data, _ := json.Marshal(opts.pipelineRunIfDockerfileChanged)
- base.Annotations["pipeline_run_if_dockerfile_changed"] = string(data)
- pipelineOpt = true
+ data, err := json.Marshal(opts.pipelineRunIfDockerfileChanged)
+ if err != nil {
+ logrus.WithError(err).Error("failed to marshal pipeline_run_if_dockerfile_changed")
+ } else {
+ base.Annotations["pipeline_run_if_dockerfile_changed"] = string(data)
+ pipelineOpt = 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.
| if len(opts.pipelineRunIfDockerfileChanged) > 0 { | |
| if base.Annotations == nil { | |
| base.Annotations = make(map[string]string) | |
| } | |
| data, _ := json.Marshal(opts.pipelineRunIfDockerfileChanged) | |
| base.Annotations["pipeline_run_if_dockerfile_changed"] = string(data) | |
| pipelineOpt = true | |
| } | |
| if len(opts.pipelineRunIfDockerfileChanged) > 0 { | |
| if base.Annotations == nil { | |
| base.Annotations = make(map[string]string) | |
| } | |
| data, err := json.Marshal(opts.pipelineRunIfDockerfileChanged) | |
| if err != nil { | |
| logrus.WithError(err).Error("failed to marshal pipeline_run_if_dockerfile_changed") | |
| } else { | |
| base.Annotations["pipeline_run_if_dockerfile_changed"] = string(data) | |
| pipelineOpt = 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 `@pkg/prowgen/prowgen.go` around lines 312 - 319, In the prowgen annotation
setup, stop ignoring the json.Marshal result for
opts.pipelineRunIfDockerfileChanged and handle any serialization failure
explicitly. Update the block that writes
base.Annotations["pipeline_run_if_dockerfile_changed"] so it checks the marshal
error and returns it wrapped with fmt.Errorf, rather than silently continuing
with empty or invalid annotation data. Use the surrounding prowgen generation
logic and the pipelineOpt path to locate the fix.
Source: Path instructions
celebdor
force-pushed
the
CNTRLPLANE-3780-pipeline-run-if-dockerfile-changed
branch
from
9372c42 to
669ef54
Compare
CNTRLPLANE-3780 Add a new `pipeline_run_if_dockerfile_changed` field to TestStepConfiguration that allows tests to be conditionally run based on whether files feeding into specified Dockerfiles have changed. This is the first part of OCPSTRAT-3060 (dependency-aware CI job filtering). The new field is mutually exclusive with the existing run_if_changed, skip_if_only_changed, pipeline_run_if_changed, and pipeline_skip_if_only_changed fields. The prowgen serializes the field as a JSON annotation on the generated presubmit job, which the pipeline-controller (in ci-tools-standalone) will consume. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
celebdor
force-pushed
the
CNTRLPLANE-3780-pipeline-run-if-dockerfile-changed
branch
from
669ef54 to
2ecc245
Compare
|
/retest |
|
@celebdor: 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 might be interesting but we |
openshift-ci
Bot
added
the
do-not-merge/hold
|
/hold |
3 participants
Summary
DockerfileEntrytype andPipelineRunIfDockerfileChangedfield toTestStepConfigurationpipeline_run_if_dockerfile_changedannotation on presubmit jobsrun_if_changed,skip_if_only_changed,pipeline_run_if_changed, andpipeline_skip_if_only_changedfieldsDockerfileEntry.Pathis non-emptyThis is the schema/prowgen half of CNTRLPLANE-3780 (first story under OCPSTRAT-3060). The pipeline-controller half that evaluates the annotation lives in openshift/ci-tools-standalone.
Test plan
go test ./pkg/api/...passesgo test ./pkg/prowgen/...passes (2 new test cases + fixtures)go test ./pkg/validation/...passes (2 new test cases, updated error messages)make ci-operator-referenceregeneratedzz_generated.ci_operator_reference.go🤖 Generated with Claude Code
Added Dockerfile-aware change filtering to the CI config schema and presubmit job generation.
For CI users/operators, tests can now set
pipeline_run_if_dockerfile_changedonTestStepConfigurationto have presubmit jobs carry apipeline_run_if_dockerfile_changedJSON annotation. prowgen serializes this into the generated presubmit job annotations, enabling downstream controller logic (inopenshift/ci-tools-standalone) to decide whether the pipeline “second stage” should run based on Dockerfile-referenced container image inputs.The API introduces a new
DockerfileEntrytype (pathplus optionalgo_binary_targets) and documents that Dockerfile impacts are resolved by parsingCOPY/ADDinstructions;go_binary_targetssupports Go-dependency-aware filtering for DockerfileCOPY-allpatterns.Validation was updated to:
pipeline_run_if_dockerfile_changedand the existing change-filter fields (run_if_changed,skip_if_only_changed,pipeline_run_if_changed,pipeline_skip_if_only_changed)DockerfileEntry.pathto be non-emptyci-operator reference docs and prowgen tests/fixtures were regenerated to cover the new annotation behavior.