Enhancement: Allow combining `run_if_changed` and `skip_if_only_changed` for more flexible job triggering

[kaovilai]

Summary

Currently, run_if_changed and skip_if_only_changed are mutually exclusive in job configuration, enforced by validation in the validateTriggering function. This limitation prevents users from creating more sophisticated job triggering logic that could benefit from combining both conditions.

Current Implementation

As of commit 419f5e43, the validation in pkg/config/config.go prevents jobs from specifying both fields:

if job.RunIfChanged != "" && job.SkipIfOnlyChanged != "" {
    return fmt.Errorf("job %s declares run_if_changed and skip_if_only_changed, which are mutually exclusive", job.Name)
}

Motivation and Use Case

There are legitimate scenarios where combining both conditions would provide more precise control over job execution:

Example Scenario: A comprehensive test suite that should:

• Run when core application files change (run_if_changed: "^(src/|tests/)")
• BUT skip when only documentation files change (skip_if_only_changed: "^docs/")

Current Workaround Limitations:

• Using only run_if_changed cannot exclude documentation-only changes
• Using only skip_if_only_changed cannot ensure the job runs for specific critical paths
• Complex regex patterns become unwieldy and hard to maintain

Proposed Enhancement

Remove the mutual exclusivity constraint and implement combined logic where:

1. Both conditions must be satisfied for the job to run:

   • At least one changed file matches run_if_changed pattern
   • AND NOT all changed files match skip_if_only_changed pattern

2. Logical precedence:

   • If run_if_changed is specified, at least one file must match it
   • If skip_if_only_changed is specified, not all files should match it
   • Both conditions are evaluated independently and combined with AND logic

Implementation Considerations

The change would involve:

1. Remove validation constraint in validateTriggering function
2. Update job execution logic in RegexpChangeMatcher methods to handle both conditions
3. Maintain backward compatibility - existing configurations with only one field should continue working unchanged
4. Add comprehensive tests for the combined logic scenarios

Benefits

• More granular control over job triggering based on file changes
• Reduced CI resource usage by avoiding unnecessary job runs
• Simplified configuration compared to complex single-regex solutions
• Better developer experience with more intuitive job triggering rules

Backward Compatibility

This change is fully backward compatible as it only removes a restriction. Existing jobs using either run_if_changed or skip_if_only_changed individually will continue to work exactly as before.

Related Work

This enhancement addresses similar needs discussed in issue #151 where users wanted more precise control over when jobs run based on file changes, particularly for skipping CI when only documentation changes are present.

[petr-muller]

There are legitimate scenarios where combining both conditions would provide more precise control over job execution

I am not convinced the above is actually a valid statement. The example scenario does not present a legitimate case because the desired outcome is achieved simply by the presented run_if_changed stanza (anything that only touches ^docs/ would not be triggered because run_if_changed would not match).

Only hypothetical case I can imagine would be to use skip_if to "carve out" a piece of what would otherwise match run_if, something like:

run_if_changed: ^src/
skip_if_only_changed: ^src/.*[.](txt|md)

Which seems remotely useful in niche cases but at a cost of being a footgun because a misconfiguration between the two fields would result in jobs not being triggered suprisingly. The exclusive config enforcement through config check prevents that from happening.

Therefore I'm 👎 on the proposal, I think the unlocked value would be very little at a cost of removing an easy check that eliminates a class of misconfigured jobs from existing.

[petr-muller]

/kind feature
/area plugins

[kaovilai]

A valid usecase for ours is we wanted to exclude ^konflux/.*$ dir which will include yaml files
but the run_if_changed included yaml files which exists in 10 or so dirs.

[kaovilai]

  run_if_changed: .+\.go|go\.(\bmod\b|\bsum\b)|Makefile|Dockerfile|.dockerignore|.*\.ya{0,1}ml|build/.+|.sh
  skip_if_only_changed: docs/

https://github.com/openshift/oadp-operator/tree/oadp-dev/docs (which sometimes have yaml files)
https://github.com/openshift/release/blob/6080e7fa317dbbc88b27bfd7640cbe5ff58c7397/ci-operator/config/openshift/oadp-operator/openshift-oadp-operator-oadp-dev__4.19.yaml#L33

[kaovilai]

Would something like this work?

  ^(?!docs/).+\.go|go\.(\bmod\b|\bsum\b)|Makefile|Dockerfile|.dockerignore|^(?!docs/).*\.ya{0,1}ml|build/.+|.sh

[kaovilai]

Looking at Go's regexp documentation confirms it - Go does NOT support lookahead or lookbehind. The documentation shows all supported
syntax, and notably absent are:

• (?=...) positive lookahead
• (?!...) negative lookahead
• (?<=...) positive lookbehind
• (?<!...) negative lookbehind

This is because Go uses RE2, which guarantees O(n) time complexity by avoiding backtracking.

Solution for excluding docs/ in Prow

Since negative lookahead isn't available, here are options without implementing #502:

Option 1: List specific directories explicitly

^(cmd|internal|pkg|tests|api|config|hack|must-gather|bundle|controllers)/.*|^go\\.(mod|sum)$|^Makefile$|^Dockerfile$|^\\.dockerignore$|^build/.+|\\.sh$

Option 2: Use alternation to match everything except docs

^[^d].*\\.go|^d[^o].*\\.go|^do[^c].*\\.go|^doc[^s].*\\.go|^docs[^/].*\\.go|^(cmd|internal|pkg|tests|api|config)/.*|go\\.(mod|sum)|Makefile|Dockerfile

[BenTheElder]

The run_if_changed feature tends to be misused, when you do something like "trigger on go source files" developers tend to miss other inputs like the go.sum, go.mod, go.work, go.work.sum, makefile, etc., that's where go's internal caching or a system like bazel handles "trigger if needed" more precisely (though they have other challenges)

skip_if_only_changed is a safer appraoch, generally, it's much easier to reason about "docs/ are not an input to the binaries under test" and new files will generally default to triggering (consider when go added modules, workspaces, etc)

I worry about making this feature even more of a footgun and difficult to reason about.