image-detector: support /image commit directive to force image builds#5105
image-detector: support /image commit directive to force image builds#5105jmguzik wants to merge 1 commit intoopenshift:mainfrom
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAffectedTools now reads commit messages between baseRef..HEAD for Changes
Sequence DiagramsequenceDiagram
participant Caller as Caller
participant Detector as AffectedTools()
participant Git as "git log"
participant Parser as parseForcedImagesFromCommitMessages()
participant Config as Config
Caller->>Detector: AffectedTools(baseRef..HEAD)
Detector->>Git: git log --format=%B%x00 baseRef..HEAD
Git-->>Detector: commit messages (NUL-separated)
Detector->>Parser: parse messages + allowed image names
Parser->>Config: validate tokens vs configured images
Config-->>Parser: allowed / unknown results
Parser-->>Detector: forced set + all flag
alt all == true
Detector->>Config: getAllImageNames()
Config-->>Detector: all image names
end
Detector->>Detector: union forced images into affected set
Detector-->>Caller: final affected images (with forcedByCommitMessage info)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 warning, 2 inconclusive)
✅ Passed checks (7 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jmguzik 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 |
openshift-ci
bot
added
the
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: 2
🧹 Nitpick comments (1)
pkg/tool-detector/detector_test.go (1)
865-875: Add oneAffectedTools()-level test for the new control flow.Lines 867-873 only exercise the parser. They won't catch regressions in the
AffectedTools()branches that merge forced images, short-circuit on/image all, or hit the empty-diff path. A small git-backed test there would lock down the behavior that actually drives image skipping.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/tool-detector/detector_test.go` around lines 865 - 875, Add a new test in detector_test.go that exercises the higher-level AffectedTools() control flow (not just parseForcedImagesFromCommitMessages) by creating a small git-backed repo state and commits that include the forced-image patterns; call AffectedTools() (the function under test) and assert the three important branches: (1) merging of forced images into the result, (2) short-circuit behavior when a commit contains "/image all", and (3) the empty-diff path when no changes should affect tools. Use the existing tests table style and reference parseForcedImagesFromCommitMessages and AffectedTools by name so the test fails if those branches regress. Ensure the test sets up commits/messages and allowed list to trigger each branch and validates the returned forced-images map and the all-flag or empty result accordingly.
🤖 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/tool-detector/detector.go`:
- Around line 142-151: The git range in
Detector.getForcedImagesFromCommitMessages uses the symmetric difference range
"baseRef...HEAD" which can include commits from base when it's not an ancestor;
change the git log invocation to use a HEAD-only range such as "baseRef..HEAD"
(or compute merge-base(baseRef, HEAD) and use "mergeBase..HEAD") so only commits
reachable from HEAD are scanned for /image directives; update the exec.Command
call that builds the "git log" args in getForcedImagesFromCommitMessages
accordingly and audit any other uses of "...HEAD" in this package (e.g., other
functions that call git log or git diff) to the same pattern.
---
Nitpick comments:
In `@pkg/tool-detector/detector_test.go`:
- Around line 865-875: Add a new test in detector_test.go that exercises the
higher-level AffectedTools() control flow (not just
parseForcedImagesFromCommitMessages) by creating a small git-backed repo state
and commits that include the forced-image patterns; call AffectedTools() (the
function under test) and assert the three important branches: (1) merging of
forced images into the result, (2) short-circuit behavior when a commit contains
"/image all", and (3) the empty-diff path when no changes should affect tools.
Use the existing tests table style and reference
parseForcedImagesFromCommitMessages and AffectedTools by name so the test fails
if those branches regress. Ensure the test sets up commits/messages and allowed
list to trigger each branch and validates the returned forced-images map and the
all-flag or empty result accordingly.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 9acfe622-7b48-4518-8562-0abb52a2f7f4
📒 Files selected for processing (2)
pkg/tool-detector/detector.gopkg/tool-detector/detector_test.go
|
Scheduling tests matching the |
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
1 similar comment
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
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 `@pkg/tool-detector/detector.go`:
- Around line 89-91: The early returns fail to include forcedImages when
expanding binary-inputs; ensure the binary-input expansion call
getAffectedToolsByBinaryInputs is seeded with forcedImages so images forced via
"/image <tool>" propagate through inputs["bin"]. Specifically, where
affectedByImageChanges.Union(getAffectedToolsByBinaryInputs(affectedByImageChanges))
is computed (and in the other two similar return paths), call
getAffectedToolsByBinaryInputs with the union of affectedByImageChanges and
forcedImages (e.g.,
getAffectedToolsByBinaryInputs(affectedByImageChanges.Union(forcedImages))) and
then union the result with forcedImages before returning so forcedImages are
part of the binary-input expansion in all three return branches.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 72b03a4f-df69-46e0-9fc0-09a8fb5aca72
📒 Files selected for processing (2)
pkg/tool-detector/detector.gopkg/tool-detector/detector_test.go
✅ Files skipped from review due to trivial changes (1)
- pkg/tool-detector/detector_test.go
When build_if_affected is enabled, users can now add /image directives in commit messages to force-build specific images regardless of code change detection. The directive must start at the beginning of a line. Supported syntax: - /image <name1> <name2> ... - force specific images - /image all - force all configured images Image names are validated against images.items[].to in ci-operator config. Unknown names are warned and ignored. Forced images are unioned with auto-detected affected images. Signed-off-by: Jakub Guzik <jguzik@redhat.com>
|
/retest |
|
Scheduling tests matching the |
|
@jmguzik: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
1 similar comment
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
1 participant
When build_if_affected is enabled, users can now add /image directives in commit messages to force-build specific images regardless of code change detection. The directive must start at the beginning of a line.
Supported syntax:
Image names are validated against images.items[].to in ci-operator config. Unknown names are warned and ignored. Forced images are unioned with auto-detected affected images.
Summary by CodeRabbit
New Features
/imagedirectives are applied before change-based detection;/image allforces all configured images and requested image names are merged into the detected set.Tests
/imagedirectives across multiple commits, CRLF handling, ignored unknown names, and/image all.