Skip to content

CNTRLPLANE-3780: feat(prowgen): add pipeline_run_if_dockerfile_changed annotation #5300

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
celebdor wants to merge 1 commit into
base: main
Choose a base branch
from
Open

CNTRLPLANE-3780: feat(prowgen): add pipeline_run_if_dockerfile_changed annotation #5300

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions pkg/api/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -792,6 +792,16 @@ const (
ClusterHive Cluster = "hosted-mgmt"
)

// DockerfileEntry specifies a Dockerfile whose build output determines
// whether a test should run.
type DockerfileEntry struct {
// Path is the path to the Dockerfile relative to the repo root.
Path string `json:"path"`
// GoBinaryTargets is an optional list of Go build targets (e.g., "./cmd/foo").
// When set, enables go-dependency-aware filtering for COPY-all Dockerfiles.
GoBinaryTargets []string `json:"go_binary_targets,omitempty"`
}

// TestStepConfiguration describes a step that runs a
// command in one of the previously built images and then
// gathers artifacts from that step.
Expand Down Expand Up @@ -893,6 +903,12 @@ type TestStepConfiguration struct {
// stage of the pipeline run if all changed files match that regex.
PipelineSkipIfOnlyChanged string `json:"pipeline_skip_if_only_changed,omitempty"`

// PipelineRunIfDockerfileChanged determines whether to run a test based on
// whether the container image built by the specified Dockerfile(s) would change.
// Instead of hand-maintained regex, it resolves which files matter by parsing
// Dockerfile COPY/ADD instructions.
PipelineRunIfDockerfileChanged []DockerfileEntry `json:"pipeline_run_if_dockerfile_changed,omitempty"`

// Timeout overrides maximum prowjob duration
Timeout *prowv1.Duration `json:"timeout,omitempty"`

Expand Down
27 changes: 27 additions & 0 deletions pkg/api/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

35 changes: 23 additions & 12 deletions pkg/prowgen/prowgen.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package prowgen

import (
"encoding/json"
"fmt"
"hash/fnv"

Expand Down Expand Up @@ -226,6 +227,7 @@ func handlePresubmit(g *prowJobBaseBuilder, element cioperatorapi.TestStepConfig
presubmit := generatePresubmitForTest(g, name, info, func(options *generatePresubmitOptions) {
options.pipelineRunIfChanged = element.PipelineRunIfChanged
options.pipelineSkipIfOnlyChanged = element.PipelineSkipIfOnlyChanged
options.pipelineRunIfDockerfileChanged = element.PipelineRunIfDockerfileChanged
options.Capabilities = element.Capabilities
options.runIfChanged = element.RunIfChanged
options.skipIfOnlyChanged = element.SkipIfOnlyChanged
Expand All @@ -244,21 +246,22 @@ func handlePresubmit(g *prowJobBaseBuilder, element cioperatorapi.TestStepConfig
}

type generatePresubmitOptions struct {
pipelineRunIfChanged string
pipelineSkipIfOnlyChanged string
Capabilities []string
runIfChanged string
skipIfOnlyChanged string
skipBranches []string
defaultDisable bool
optional bool
disableRehearsal bool
maxConcurrency int
slackReporterConfig *cioperatorapi.SlackReporterConfig
pipelineRunIfChanged string
pipelineSkipIfOnlyChanged string
pipelineRunIfDockerfileChanged []cioperatorapi.DockerfileEntry
Capabilities []string
runIfChanged string
skipIfOnlyChanged string
skipBranches []string
defaultDisable bool
optional bool
disableRehearsal bool
maxConcurrency int
slackReporterConfig *cioperatorapi.SlackReporterConfig
}

func (opts *generatePresubmitOptions) shouldAlwaysRun() bool {
return opts.runIfChanged == "" && opts.skipIfOnlyChanged == "" && !opts.defaultDisable && opts.pipelineRunIfChanged == "" && opts.pipelineSkipIfOnlyChanged == ""
return opts.runIfChanged == "" && opts.skipIfOnlyChanged == "" && !opts.defaultDisable && opts.pipelineRunIfChanged == "" && opts.pipelineSkipIfOnlyChanged == "" && len(opts.pipelineRunIfDockerfileChanged) == 0
}

type generatePresubmitOption func(options *generatePresubmitOptions)
Expand Down Expand Up @@ -306,6 +309,14 @@ func generatePresubmitForTest(jobBaseBuilder *prowJobBaseBuilder, name string, i
base.Annotations["pipeline_skip_if_only_changed"] = opts.pipelineSkipIfOnlyChanged
pipelineOpt = true
}
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
}
Comment on lines +312 to +319

@coderabbitai coderabbitai Bot Jul 8, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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.

Suggested change
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

triggerCommand := prowconfig.DefaultTriggerFor(shortName)
if opts.defaultDisable && opts.runIfChanged == "" && opts.skipIfOnlyChanged == "" && !opts.optional && !pipelineOpt {
triggerCommand = fmt.Sprintf(`(?m)^/test( | .* )(%s|%s),?($|\s.*)`, shortName, "remaining-required")
Expand Down
22 changes: 22 additions & 0 deletions pkg/prowgen/prowgen_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,28 @@ func TestGeneratePresubmitForTest(t *testing.T) {
options.pipelineSkipIfOnlyChanged = "^docs/"
},
},
{
description: "presubmit with always_run=false and pipeline_run_if_dockerfile_changed",
test: ciop.TestStepConfiguration{As: "testname"},
repoInfo: &ciop.Metadata{Org: "org", Repo: "repo", Branch: "branch"},
generateOption: func(options *generatePresubmitOptions) {
options.defaultDisable = false
options.pipelineRunIfDockerfileChanged = []ciop.DockerfileEntry{
{Path: "Dockerfile"},
}
},
},
{
description: "presubmit with always_run but pipeline_run_if_dockerfile_changed set",
test: ciop.TestStepConfiguration{As: "testname"},
repoInfo: &ciop.Metadata{Org: "org", Repo: "repo", Branch: "branch"},
generateOption: func(options *generatePresubmitOptions) {
options.defaultDisable = true
options.pipelineRunIfDockerfileChanged = []ciop.DockerfileEntry{
{Path: "Dockerfile", GoBinaryTargets: []string{"./cmd/server"}},
}
},
},
{
description: "presubmit with always_run but optional true",
test: ciop.TestStepConfiguration{As: "testname"},
Expand Down
16 changes: 16 additions & 0 deletions ...resubmitForTest_presubmit_with_always_run_but_pipeline_run_if_dockerfile_changed_set.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
agent: kubernetes
always_run: false
annotations:
pipeline_run_if_dockerfile_changed: '[{"path":"Dockerfile","go_binary_targets":["./cmd/server"]}]'
branches:
- ^branch$
- ^branch-
context: ci/prow/testname
decorate: true
decoration_config:
skip_cloning: true
labels:
pj-rehearse.openshift.io/can-be-rehearsed: "true"
name: pull-ci-org-repo-branch-testname
rerun_command: /test testname
trigger: (?m)^/test( | .* )testname,?($|\s.*)
16 changes: 16 additions & 0 deletions ...submitForTest_presubmit_with_always_run_false_and_pipeline_run_if_dockerfile_changed.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
agent: kubernetes
always_run: false
annotations:
pipeline_run_if_dockerfile_changed: '[{"path":"Dockerfile"}]'
branches:
- ^branch$
- ^branch-
context: ci/prow/testname
decorate: true
decoration_config:
skip_cloning: true
labels:
pj-rehearse.openshift.io/can-be-rehearsed: "true"
name: pull-ci-org-repo-branch-testname
rerun_command: /test testname
trigger: (?m)^/test( | .* )testname,?($|\s.*)
14 changes: 11 additions & 3 deletions pkg/validation/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -314,22 +314,30 @@ func validateBuildRootImageStreamTag(ctx *configContext, buildRoot api.ImageStre
return validationErrors
}

func validateRunIfChangedExclusivity(runIfChanged, skipIfOnlyChanged, pipelineRunIfChanged, pipelineSkipIfOnlyChanged string) error {
func validateRunIfChangedExclusivity(runIfChanged, skipIfOnlyChanged, pipelineRunIfChanged, pipelineSkipIfOnlyChanged string, pipelineRunIfDockerfileChanged []api.DockerfileEntry) error {
set := 0
for _, f := range []string{runIfChanged, skipIfOnlyChanged, pipelineRunIfChanged, pipelineSkipIfOnlyChanged} {
if f != "" {
set++
}
}
if len(pipelineRunIfDockerfileChanged) > 0 {
set++
}
if set > 1 {
return fmt.Errorf("`run_if_changed`, `skip_if_only_changed`, `pipeline_run_if_changed`, and `pipeline_skip_if_only_changed` are mutually exclusive")
return fmt.Errorf("`run_if_changed`, `skip_if_only_changed`, `pipeline_run_if_changed`, `pipeline_skip_if_only_changed`, and `pipeline_run_if_dockerfile_changed` are mutually exclusive")
}
for i, entry := range pipelineRunIfDockerfileChanged {
if entry.Path == "" {
return fmt.Errorf("`pipeline_run_if_dockerfile_changed[%d].path` must be set", i)
}
}
return nil
}

func validateImageConfiguration(ctx *configContext, images api.ImageConfiguration) []error {
var validationErrors []error
if err := validateRunIfChangedExclusivity(images.RunIfChanged, images.SkipIfOnlyChanged, images.PipelineRunIfChanged, images.PipelineSkipIfOnlyChanged); err != nil {
if err := validateRunIfChangedExclusivity(images.RunIfChanged, images.SkipIfOnlyChanged, images.PipelineRunIfChanged, images.PipelineSkipIfOnlyChanged, nil); err != nil {
validationErrors = append(validationErrors, ctx.errorf("%s", err))
}
validationErrors = append(validationErrors, ValidateImages(ctx, images.Items)...)
Expand Down
6 changes: 3 additions & 3 deletions pkg/validation/config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -525,7 +525,7 @@ func TestValidateImageConfiguration(t *testing.T) {
SkipIfOnlyChanged: `^docs/`,
},
output: []error{
errors.New("images: `run_if_changed`, `skip_if_only_changed`, `pipeline_run_if_changed`, and `pipeline_skip_if_only_changed` are mutually exclusive"),
errors.New("images: `run_if_changed`, `skip_if_only_changed`, `pipeline_run_if_changed`, `pipeline_skip_if_only_changed`, and `pipeline_run_if_dockerfile_changed` are mutually exclusive"),
},
}, {
name: "pipeline_run_if_changed and pipeline_skip_if_only_changed are mutually exclusive",
Expand All @@ -534,7 +534,7 @@ func TestValidateImageConfiguration(t *testing.T) {
PipelineSkipIfOnlyChanged: `^docs/`,
},
output: []error{
errors.New("images: `run_if_changed`, `skip_if_only_changed`, `pipeline_run_if_changed`, and `pipeline_skip_if_only_changed` are mutually exclusive"),
errors.New("images: `run_if_changed`, `skip_if_only_changed`, `pipeline_run_if_changed`, `pipeline_skip_if_only_changed`, and `pipeline_run_if_dockerfile_changed` are mutually exclusive"),
},
}, {
name: "run_if_changed with pipeline_skip_if_only_changed are mutually exclusive",
Expand All @@ -543,7 +543,7 @@ func TestValidateImageConfiguration(t *testing.T) {
PipelineSkipIfOnlyChanged: `^docs/`,
},
output: []error{
errors.New("images: `run_if_changed`, `skip_if_only_changed`, `pipeline_run_if_changed`, and `pipeline_skip_if_only_changed` are mutually exclusive"),
errors.New("images: `run_if_changed`, `skip_if_only_changed`, `pipeline_run_if_changed`, `pipeline_skip_if_only_changed`, and `pipeline_run_if_dockerfile_changed` are mutually exclusive"),
},
}, {
name: "run_if_changed alone is valid",
Expand Down
2 changes: 1 addition & 1 deletion pkg/validation/test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,7 @@ func (v *Validator) validateTestStepConfiguration(
if (test.Cron != nil || test.Interval != nil || test.MinimumInterval != nil) && !test.Presubmit && (test.RunIfChanged != "" || test.SkipIfOnlyChanged != "" || test.Optional) {
validationErrors = append(validationErrors, fmt.Errorf("%s: `cron`/`interval`/`minimum_interval` are mutually exclusive with `run_if_changed`/`skip_if_only_changed`/`optional`", fieldRootN))
}
if err := validateRunIfChangedExclusivity(test.RunIfChanged, test.SkipIfOnlyChanged, test.PipelineRunIfChanged, test.PipelineSkipIfOnlyChanged); err != nil {
if err := validateRunIfChangedExclusivity(test.RunIfChanged, test.SkipIfOnlyChanged, test.PipelineRunIfChanged, test.PipelineSkipIfOnlyChanged, test.PipelineRunIfDockerfileChanged); err != nil {
validationErrors = append(validationErrors, fmt.Errorf("%s: %w", fieldRootN, err))
}

Expand Down
23 changes: 22 additions & 1 deletion pkg/validation/test_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -850,7 +850,28 @@ func TestValidateTests(t *testing.T) {
SkipIfOnlyChanged: "^OTHER_README.md$",
ContainerTestConfiguration: &api.ContainerTestConfiguration{From: "ignored"},
}},
expectedError: errors.New("tests[0]: `run_if_changed`, `skip_if_only_changed`, `pipeline_run_if_changed`, and `pipeline_skip_if_only_changed` are mutually exclusive"),
expectedError: errors.New("tests[0]: `run_if_changed`, `skip_if_only_changed`, `pipeline_run_if_changed`, `pipeline_skip_if_only_changed`, and `pipeline_run_if_dockerfile_changed` are mutually exclusive"),
},
{
id: "pipeline_run_if_dockerfile_changed with run_if_changed are mutually exclusive",
tests: []api.TestStepConfiguration{{
As: "unit",
Commands: "commands",
RunIfChanged: "^README.md$",
PipelineRunIfDockerfileChanged: []api.DockerfileEntry{{Path: "Dockerfile"}},
ContainerTestConfiguration: &api.ContainerTestConfiguration{From: "ignored"},
}},
expectedError: errors.New("tests[0]: `run_if_changed`, `skip_if_only_changed`, `pipeline_run_if_changed`, `pipeline_skip_if_only_changed`, and `pipeline_run_if_dockerfile_changed` are mutually exclusive"),
},
{
id: "pipeline_run_if_dockerfile_changed with empty path",
tests: []api.TestStepConfiguration{{
As: "unit",
Commands: "commands",
PipelineRunIfDockerfileChanged: []api.DockerfileEntry{{Path: ""}},
ContainerTestConfiguration: &api.ContainerTestConfiguration{From: "ignored"},
}},
expectedError: errors.New("tests[0]: `pipeline_run_if_dockerfile_changed[0].path` must be set"),
},
{
id: "secrets used on multi-stage tests",
Expand Down
22 changes: 22 additions & 0 deletions pkg/webreg/zz_generated.ci_operator_reference.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1090,6 +1090,17 @@ const ciOperatorReferenceYaml = "# The list of base images describe\n" +
" # PipelineRunIfChanged is a regex that will result in the test only running in second\n" +
" # stage of the pipeline run if something that matches it was changed.\n" +
" pipeline_run_if_changed: ' '\n" +
" # PipelineRunIfDockerfileChanged determines whether to run a test based on\n" +
" # whether the container image built by the specified Dockerfile(s) would change.\n" +
" # Instead of hand-maintained regex, it resolves which files matter by parsing\n" +
" # Dockerfile COPY/ADD instructions.\n" +
" pipeline_run_if_dockerfile_changed:\n" +
" - # GoBinaryTargets is an optional list of Go build targets (e.g., \"./cmd/foo\").\n" +
" # When set, enables go-dependency-aware filtering for COPY-all Dockerfiles.\n" +
" go_binary_targets:\n" +
" - \"\"\n" +
" # Path is the path to the Dockerfile relative to the repo root.\n" +
" path: ' '\n" +
" # PipelineSkipIfOnlyChanged is a regex that will result in the test being skipped in second\n" +
" # stage of the pipeline run if all changed files match that regex.\n" +
" pipeline_skip_if_only_changed: ' '\n" +
Expand Down Expand Up @@ -2071,6 +2082,17 @@ const ciOperatorReferenceYaml = "# The list of base images describe\n" +
" # PipelineRunIfChanged is a regex that will result in the test only running in second\n" +
" # stage of the pipeline run if something that matches it was changed.\n" +
" pipeline_run_if_changed: ' '\n" +
" # PipelineRunIfDockerfileChanged determines whether to run a test based on\n" +
" # whether the container image built by the specified Dockerfile(s) would change.\n" +
" # Instead of hand-maintained regex, it resolves which files matter by parsing\n" +
" # Dockerfile COPY/ADD instructions.\n" +
" pipeline_run_if_dockerfile_changed:\n" +
" - # GoBinaryTargets is an optional list of Go build targets (e.g., \"./cmd/foo\").\n" +
" # When set, enables go-dependency-aware filtering for COPY-all Dockerfiles.\n" +
" go_binary_targets:\n" +
" - \"\"\n" +
" # Path is the path to the Dockerfile relative to the repo root.\n" +
" path: ' '\n" +
" # PipelineSkipIfOnlyChanged is a regex that will result in the test being skipped in second\n" +
" # stage of the pipeline run if all changed files match that regex.\n" +
" pipeline_skip_if_only_changed: ' '\n" +
Expand Down