image-detector: support /image commit directive to force image builds#5105
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
|
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:
WalkthroughThe changes extend Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
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
|
/retest |
|
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. |
|
/lgtm |
|
/retest |
|
/retest |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
pkg/jira/fake.go (1)
45-50: Expose setup hooks for the new fake behaviors.
NewFakestill only lets callers seedFileIssueexpectations. BecausecloseBehaviorandstatusBehaviorare unexported, packages outsidepkg/jiracan't reuse this fake for the expandedIssueFilersurface; the newpkg/slack/events/supportrequest/handler_test.gohad to define another filer instead. Consider constructor options or helper setters for the new behaviors.Also applies to: 125-130
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/jira/fake.go` around lines 45 - 50, NewFake currently only seeds FileIssue expectations and leaves closeBehavior, statusBehavior, unwantedCloseCalls and unwantedStatusCalls unexported so external tests can't reuse this fake for the full IssueFiler surface; expose hooks by adding constructor options or helper setters on the fake (e.g., NewFake(...opts) that accept closeBehavior map[CloseIssueRequest]CloseIssueResponse and statusBehavior map[SetIssueStatusRequest]SetIssueStatusResponse, or methods SetCloseBehavior and SetStatusBehavior) and provide getters or exported slices for unwantedCloseCalls/unwantedStatusCalls so tests can inspect calls; update the fake's NewFake and the fake type to accept or set these behaviors and ensure the names referenced (NewFake, closeBehavior, statusBehavior, unwantedCloseCalls, unwantedStatusCalls, IssueRequest, CloseIssueRequest, SetIssueStatusRequest) are used to locate the changes.pkg/slack/events/supportrequest/lock.go (1)
40-73: Consider acceptingcontext.Contextparameter instead of usingcontext.TODO().Using
context.TODO()throughout the lock operations prevents proper cancellation and timeout propagation from callers. This could cause the handler to hang indefinitely if the Kubernetes API is slow or unresponsive.♻️ Suggested signature change
-func (c *configMapLockClient) Acquire(threadTS string) (bool, error) { +func (c *configMapLockClient) Acquire(ctx context.Context, threadTS string) (bool, error) { key := lockNameForThread(threadTS) for i := 0; i < maxLockRetries; i++ { - lockMap, err := c.getOrCreateLockMap() + lockMap, err := c.getOrCreateLockMap(ctx)Apply the same pattern to
MarkProcessed,Release, andgetOrCreateLockMap.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/slack/events/supportrequest/lock.go` around lines 40 - 73, Change the locking methods to accept a context.Context parameter and propagate it instead of using context.TODO(): update the signatures of Acquire, MarkProcessed, Release and getOrCreateLockMap to accept ctx context.Context, replace any context.TODO() calls (e.g., in c.client.Update and any other k8s client calls) with the passed ctx, and update all call sites to forward the caller's context; ensure error handling and retry logic remain the same and that timeout/cancellation from the caller now properly cancels k8s API requests.
🤖 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/slack/events/supportrequest/lock.go`:
- Around line 75-98: The ConfigMap growth issue stems from MarkProcessed
(configMapLockClient) writing perpetual entries (lockStateProcessed) under keys
from lockNameForThread into threadLockMapName; change the stored value to
include a timestamp (e.g., "processed|<unix_ts>") or a separate timestamp map so
each entry can be aged, and implement a cleanup mechanism: add a
CleanProcessedOlderThan(threshold time.Duration) method that reads
threadLockMapName, removes entries whose timestamp is older than threshold, and
updates the ConfigMap with the same retry/conflict handling used in
MarkProcessed; schedule that cleanup via a periodic goroutine or external
CronJob and ensure all updates handle apierrors.IsConflict with retries.
---
Nitpick comments:
In `@pkg/jira/fake.go`:
- Around line 45-50: NewFake currently only seeds FileIssue expectations and
leaves closeBehavior, statusBehavior, unwantedCloseCalls and unwantedStatusCalls
unexported so external tests can't reuse this fake for the full IssueFiler
surface; expose hooks by adding constructor options or helper setters on the
fake (e.g., NewFake(...opts) that accept closeBehavior
map[CloseIssueRequest]CloseIssueResponse and statusBehavior
map[SetIssueStatusRequest]SetIssueStatusResponse, or methods SetCloseBehavior
and SetStatusBehavior) and provide getters or exported slices for
unwantedCloseCalls/unwantedStatusCalls so tests can inspect calls; update the
fake's NewFake and the fake type to accept or set these behaviors and ensure the
names referenced (NewFake, closeBehavior, statusBehavior, unwantedCloseCalls,
unwantedStatusCalls, IssueRequest, CloseIssueRequest, SetIssueStatusRequest) are
used to locate the changes.
In `@pkg/slack/events/supportrequest/lock.go`:
- Around line 40-73: Change the locking methods to accept a context.Context
parameter and propagate it instead of using context.TODO(): update the
signatures of Acquire, MarkProcessed, Release and getOrCreateLockMap to accept
ctx context.Context, replace any context.TODO() calls (e.g., in c.client.Update
and any other k8s client calls) with the passed ctx, and update all call sites
to forward the caller's context; ensure error handling and retry logic remain
the same and that timeout/cancellation from the caller now properly cancels k8s
API requests.
🪄 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: 2028b837-2743-452f-a66e-2b7ae404bd82
📒 Files selected for processing (12)
cmd/slack-bot/README.mdcmd/slack-bot/main.gopkg/jira/fake.gopkg/jira/issues.gopkg/jira/issues_test.gopkg/slack/events/router/router.gopkg/slack/events/supportrequest/handler.gopkg/slack/events/supportrequest/handler_test.gopkg/slack/events/supportrequest/lock.gopkg/slack/events/supportrequest/lock_test.gopkg/tool-detector/detector.gopkg/tool-detector/detector_test.go
✅ Files skipped from review due to trivial changes (1)
- cmd/slack-bot/README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/tool-detector/detector.go
| // Jira issue is created, so keep this thread lock permanently by | ||
| // converting it to a durable "processed" marker. | ||
| shouldReleaseLock = false | ||
| if err := locker.MarkProcessed(event.ThreadTimeStamp); err != nil { | ||
| return true, err | ||
| } | ||
| if err := filer.SetIssueStatus(issue.Key, jira.StatusInProgress, logger); err != nil { | ||
| logger.WithError(err).WithField("issue", issue.Key).Warn("Failed to transition support request to In Progress") | ||
| } | ||
|
|
||
| issueURL := fmt.Sprintf("%s%s", issuesRedHatBrowseBase, issue.Key) | ||
| _, _, postErr := postMessageWithRetry(client, channelID, slack.MsgOptionText( | ||
| fmt.Sprintf( | ||
| "%s <%s|%s>\nThis ticket was created automatically after this thread exceeded the threshold of %d messages. No user action is needed and conversation can continue in this forum thread.", | ||
| supportRequestPrefix, issueURL, issue.Key, threadMessageThreshold, | ||
| ), | ||
| false, | ||
| ), slack.MsgOptionTS(event.ThreadTimeStamp)) | ||
| if postErr != nil { | ||
| logger.WithError(postErr).Warn("Failed to post support request Jira link in Slack thread") | ||
| } |
There was a problem hiding this comment.
Make the thread→issue mapping durable before exiting this critical section.
After FileIssue() succeeds, there are two failure windows here with bad recovery behavior. If MarkProcessed() fails, the handler returns before posting the Slack marker, so the thread only has a TTL-bound processing lock; once that expires, a later reply can reacquire and file a second Jira. If the Slack post fails after MarkProcessed(), the thread is permanently marked processed but handleReactionAdded() has no reply to scrape for the issue key, so the Slack close flow is orphaned. Persist the issue key atomically with the lock, or otherwise ensure one durable duplicate-suppression/discovery marker exists before returning.
| func (c *configMapLockClient) MarkProcessed(threadTS string) error { | ||
| key := lockNameForThread(threadTS) | ||
| for i := 0; i < maxLockRetries; i++ { | ||
| lockMap := &corev1.ConfigMap{} | ||
| if err := c.client.Get(context.TODO(), ctrlruntimeclient.ObjectKey{Namespace: c.namespace, Name: threadLockMapName}, lockMap); err != nil { | ||
| if apierrors.IsNotFound(err) { | ||
| return nil | ||
| } | ||
| return err | ||
| } | ||
| if lockMap.Data == nil { | ||
| lockMap.Data = map[string]string{} | ||
| } | ||
| lockMap.Data[key] = lockStateProcessed | ||
| if err := c.client.Update(context.TODO(), lockMap); err != nil { | ||
| if apierrors.IsConflict(err) { | ||
| continue | ||
| } | ||
| return err | ||
| } | ||
| return nil | ||
| } | ||
| return fmt.Errorf("failed to mark lock for %s as processed after retries", threadTS) | ||
| } |
There was a problem hiding this comment.
ConfigMap can grow unbounded with "processed" entries.
The MarkProcessed method adds entries that are never cleaned up. Over time, the ConfigMap will accumulate stale "processed" entries, potentially approaching the 1MB etcd limit for ConfigMap data. Consider implementing periodic cleanup of old processed entries or adding a TTL-based eviction mechanism.
Would you like me to help design a cleanup strategy, such as:
- Storing a timestamp with "processed" state and purging entries older than N days
- Running a periodic goroutine that cleans old entries
- Using a separate cleanup job/CronJob
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/slack/events/supportrequest/lock.go` around lines 75 - 98, The ConfigMap
growth issue stems from MarkProcessed (configMapLockClient) writing perpetual
entries (lockStateProcessed) under keys from lockNameForThread into
threadLockMapName; change the stored value to include a timestamp (e.g.,
"processed|<unix_ts>") or a separate timestamp map so each entry can be aged,
and implement a cleanup mechanism: add a CleanProcessedOlderThan(threshold
time.Duration) method that reads threadLockMapName, removes entries whose
timestamp is older than threshold, and updates the ConfigMap with the same
retry/conflict handling used in MarkProcessed; schedule that cleanup via a
periodic goroutine or external CronJob and ensure all updates handle
apierrors.IsConflict with retries.
|
/retest |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danilo-gemoli, jmguzik, 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 |
|
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. |
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>
|
New changes are detected. LGTM label has been removed. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/tool-detector/detector.go (1)
153-182: Optional: dedupe unknown-image warnings to reduce log noise.If multiple commits repeat the same unknown image token, this currently logs repeatedly. Consider warning once per unique unknown token per parse run.
Possible small refactor
func parseForcedImagesFromCommitMessages(messages []string, allowedImageNames sets.Set[string]) (sets.Set[string], bool) { forced := sets.New[string]() + unknownWarned := sets.New[string]() for _, message := range messages { scanner := bufio.NewScanner(strings.NewReader(message)) for scanner.Scan() { line := scanner.Text() @@ if allowedImageNames.Has(imageName) { forced.Insert(imageName) continue } - logrus.WithField("image", imageName).Warn("Ignoring unknown forced image requested via /image commit directive") + if !unknownWarned.Has(imageName) { + logrus.WithField("image", imageName).Warn("Ignoring unknown forced image requested via /image commit directive") + unknownWarned.Insert(imageName) + } } }🤖 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 153 - 182, In parseForcedImagesFromCommitMessages, duplicate "unknown forced image" warnings are noisy; add a local set (e.g., unknownWarned := sets.New[string]()) at the top of the function and, when encountering an imageName that's not in allowedImageNames and not "all", only call logrus.WithField(...).Warn(...) if unknownWarned.Has(imageName) is false, then insert imageName into unknownWarned to suppress further warnings for that token during this parse run; keep existing behavior for inserting allowed names into forced and returning allowedImageNames on "all".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/tool-detector/detector.go`:
- Around line 153-182: In parseForcedImagesFromCommitMessages, duplicate
"unknown forced image" warnings are noisy; add a local set (e.g., unknownWarned
:= sets.New[string]()) at the top of the function and, when encountering an
imageName that's not in allowedImageNames and not "all", only call
logrus.WithField(...).Warn(...) if unknownWarned.Has(imageName) is false, then
insert imageName into unknownWarned to suppress further warnings for that token
during this parse run; keep existing behavior for inserting allowed names into
forced and returning allowedImageNames on "all".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 1fa3d157-4a8e-427d-873b-1b3b6595cc93
📒 Files selected for processing (2)
pkg/tool-detector/detector.gopkg/tool-detector/detector_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/tool-detector/detector_test.go
|
/retest |
|
@jmguzik: 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. |
5 participants
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
Release Notes
/imagedirectives to control which tools and images are selected for builds/image <name>to force a specific tool, or/image allto include all configured images