WIP: feat(slack-bot): automate support-request Jira workflow#5111
WIP: feat(slack-bot): automate support-request Jira workflow#5111jmguzik wants to merge 1 commit intoopenshift:mainfrom
Conversation
Add support-request handling for long forum threads by creating DPTP Jira tickets, posting thread guidance, and closing linked tickets from :closed: reactions with replica-safe locking and transition-aware Jira updates. Signed-off-by: Jakub Guzik <jguzik@redhat.com>
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
[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
WalkthroughThis pull request introduces a new "support-request" feature for the Slack bot. The implementation includes Jira issue lifecycle management (closing/status updates), distributed locking via Kubernetes ConfigMaps for cross-replica coordination, a new Slack event handler that monitors threads and automatically files Jira issues when message thresholds are exceeded, and related CLI configuration options. Changes
Sequence Diagram(s)sequenceDiagram
actor Slack as Slack User
participant SB as Slack Bot
participant LC as Lock Client
participant Jira as Jira
participant KV as K8s ConfigMap
Slack->>SB: Message in thread (exceeds threshold)
SB->>SB: Check thread eligibility
SB->>LC: Acquire lock for thread
LC->>KV: Read/write lock state
KV-->>LC: Lock acquired
LC-->>SB: Acquired
SB->>Jira: FileIssue (with thread link)
Jira-->>SB: Issue created
SB->>Slack: Post Jira link in thread
Slack-->>SB: ACK
SB->>LC: MarkProcessed
LC->>KV: Set processed marker
KV-->>LC: OK
LC-->>SB: Marked
Slack->>SB: Reaction :closed: on root message
SB->>SB: Extract Jira key from thread
SB->>Jira: CloseIssue(key, resolution)
Jira-->>SB: Issue closed
SB->>Slack: Post close confirmation
Slack-->>SB: ACK
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes 🚥 Pre-merge checks | ✅ 9 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (9 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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 (2)
pkg/slack/events/supportrequest/lock.go (1)
75-98: Consider documenting the cleanup strategy for processed entries.The
processedstate is permanent and entries are never removed, which could cause the ConfigMap to grow unbounded over time as more threads are processed. Consider adding a comment about the expected growth rate and whether periodic cleanup (e.g., a separate job) is planned.🤖 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, MarkProcessed currently writes permanent entries (lockStateProcessed) into the ConfigMap (threadLockMapName) via configMapLockClient.MarkProcessed which can cause unbounded growth; add a concise comment above the MarkProcessed method (or near the constants lockStateProcessed/threadLockMapName) documenting that processed entries are retained, the expected growth rate, and the intended cleanup strategy (e.g., periodic GC job, TTL pruning, or compaction) and, if applicable, mention where that job will live or how retention is bounded so reviewers know this is intentional.pkg/jira/issues.go (1)
115-139: Case-sensitive comparison may miss valid transitions.
SetIssueStatususes case-sensitive comparison (==) whileclosingTransitionPriorityusesstrings.EqualFold. Consider using case-insensitive matching here for consistency and robustness against varying Jira workflow configurations.♻️ Proposed fix for case-insensitive matching
var transitionID string for _, transition := range transitions { - if transition.To.Name == status || transition.Name == status { + if strings.EqualFold(transition.To.Name, status) || strings.EqualFold(transition.Name, status) { transitionID = transition.ID break } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/jira/issues.go` around lines 115 - 139, SetIssueStatus currently compares transition names using case-sensitive == which can miss valid transitions; change the comparisons in SetIssueStatus to use case-insensitive matching (strings.EqualFold) when comparing transition.To.Name and transition.Name to the provided status, and add the strings import if not already present so the function consistently matches closingTransitionPriority's behavior.
🤖 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/jira/fake.go`:
- Around line 45-50: NewFake currently only accepts
map[IssueRequest]IssueResponse and the internal maps closeBehavior and
statusBehavior are unexported, so external callers cannot configure CloseIssue
or SetIssueStatus behavior; update the API by either extending NewFake to accept
additional parameters (map[CloseIssueRequest]CloseIssueResponse and
map[SetIssueStatusRequest]SetIssueStatusResponse) or add exported setter methods
on the Fake type (e.g.,
SetCloseBehavior(map[CloseIssueRequest]CloseIssueResponse) and
SetStatusBehavior(map[SetIssueStatusRequest]SetIssueStatusResponse)) that
populate the internal closeBehavior and statusBehavior fields so Fake.CloseIssue
and Fake.SetIssueStatus can be configured from outside pkg/jira.
In `@pkg/slack/events/supportrequest/handler.go`:
- Around line 154-174: The thread is marked processed too early
(locker.MarkProcessed with event.ThreadTimeStamp), making subsequent actions
best-effort; either persist the thread→issue mapping before calling
locker.MarkProcessed or move the MarkProcessed call to after the durable
operations complete. Specifically update the logic around locker.MarkProcessed,
filer.SetIssueStatus(issue.Key, ...), and the postMessageWithRetry(...) that
posts supportRequestPrefix so that the thread→issue mapping (used by
handleReactionAdded) is written durably first or MarkProcessed is invoked only
after SetIssueStatus and the Slack post both succeed (or after you persist the
mapping elsewhere) to guarantee the transition and link-back are reliable.
---
Nitpick comments:
In `@pkg/jira/issues.go`:
- Around line 115-139: SetIssueStatus currently compares transition names using
case-sensitive == which can miss valid transitions; change the comparisons in
SetIssueStatus to use case-insensitive matching (strings.EqualFold) when
comparing transition.To.Name and transition.Name to the provided status, and add
the strings import if not already present so the function consistently matches
closingTransitionPriority's behavior.
In `@pkg/slack/events/supportrequest/lock.go`:
- Around line 75-98: MarkProcessed currently writes permanent entries
(lockStateProcessed) into the ConfigMap (threadLockMapName) via
configMapLockClient.MarkProcessed which can cause unbounded growth; add a
concise comment above the MarkProcessed method (or near the constants
lockStateProcessed/threadLockMapName) documenting that processed entries are
retained, the expected growth rate, and the intended cleanup strategy (e.g.,
periodic GC job, TTL pruning, or compaction) and, if applicable, mention where
that job will live or how retention is bounded so reviewers know this is
intentional.
🪄 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: 936c293c-db53-4703-85d6-e28ce8c971df
📒 Files selected for processing (10)
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.go
| behavior map[IssueRequest]IssueResponse | ||
| closeBehavior map[CloseIssueRequest]CloseIssueResponse | ||
| statusBehavior map[SetIssueStatusRequest]SetIssueStatusResponse | ||
| unwanted []IssueRequest | ||
| unwantedCloseCalls []CloseIssueRequest | ||
| unwantedStatusCalls []SetIssueStatusRequest |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- pkg/slack/events/supportrequest/handler_test.go package ---'
sed -n '1,40p' pkg/slack/events/supportrequest/handler_test.go
printf '\n%s\n' '--- jira fake construction / setup call sites ---'
rg -n 'jira\.NewFake|NewFake\(|CloseIssueRequest|SetIssueStatusRequest|closeBehavior|statusBehavior' \
pkg/slack/events/supportrequest/handler_test.go pkg/jira
printf '\n%s\n' '--- exported helpers on pkg/jira.Fake ---'
rg -n 'func \(f \*Fake\) (With|Add|Expect|Register).*(CloseIssue|SetIssueStatus)' pkg/jiraRepository: openshift/ci-tools
Length of output: 3585
🏁 Script executed:
cat -n pkg/jira/fake.goRepository: openshift/ci-tools
Length of output: 4910
🏁 Script executed:
rg -n 'func.*Fake' pkg/jira/fake.go | head -20Repository: openshift/ci-tools
Length of output: 473
🏁 Script executed:
rg -l 'jira\.NewFake\|\.NewFake' --type go | head -10Repository: openshift/ci-tools
Length of output: 44
🏁 Script executed:
rg -n 'NewFake|&Fake\{|jira\.Fake' --type go | grep -v '^pkg/jira/fake.go'Repository: openshift/ci-tools
Length of output: 42890
🏁 Script executed:
sed -n '55,75p' pkg/slack/modals/bug/view_test.goRepository: openshift/ci-tools
Length of output: 470
🏁 Script executed:
cat -n pkg/slack/modals/modaltesting/submission.goRepository: openshift/ci-tools
Length of output: 1866
Expose public methods to configure close/status behavior.
CloseIssueRequest, SetIssueStatusRequest, and their response types are exported, and Fake.CloseIssue() and Fake.SetIssueStatus() methods are exported. However, NewFake only accepts map[IssueRequest]IssueResponse, and the behavior maps remain unexported with no public setters. Callers outside pkg/jira cannot configure behavior for close or status operations, making the added functionality unreachable.
Suggested API additions
func NewFake(calls map[IssueRequest]IssueResponse) *Fake {
return &Fake{
behavior: calls,
closeBehavior: map[CloseIssueRequest]CloseIssueResponse{},
statusBehavior: map[SetIssueStatusRequest]SetIssueStatusResponse{},
}
}
+
+func (f *Fake) WithCloseBehavior(calls map[CloseIssueRequest]CloseIssueResponse) *Fake {
+ f.closeBehavior = calls
+ return f
+}
+
+func (f *Fake) WithStatusBehavior(calls map[SetIssueStatusRequest]SetIssueStatusResponse) *Fake {
+ f.statusBehavior = calls
+ return f
+}🤖 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 accepts
map[IssueRequest]IssueResponse and the internal maps closeBehavior and
statusBehavior are unexported, so external callers cannot configure CloseIssue
or SetIssueStatus behavior; update the API by either extending NewFake to accept
additional parameters (map[CloseIssueRequest]CloseIssueResponse and
map[SetIssueStatusRequest]SetIssueStatusResponse) or add exported setter methods
on the Fake type (e.g.,
SetCloseBehavior(map[CloseIssueRequest]CloseIssueResponse) and
SetStatusBehavior(map[SetIssueStatusRequest]SetIssueStatusResponse)) that
populate the internal closeBehavior and statusBehavior fields so Fake.CloseIssue
and Fake.SetIssueStatus can be configured from outside pkg/jira.
| // 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 post-create steps durable before MarkProcessed.
After Line 157 succeeds, this thread will not be revisited. That makes both follow-up steps effectively best-effort: a SetIssueStatus failure leaves the new Jira ticket in its default state, and a Slack post failure means handleReactionAdded can no longer find the issue key because it only scans replies for the bot’s supportRequestPrefix message. Please either persist the thread→issue mapping somewhere durable or move MarkProcessed to the point where the transition/link-back is guaranteed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/slack/events/supportrequest/handler.go` around lines 154 - 174, The
thread is marked processed too early (locker.MarkProcessed with
event.ThreadTimeStamp), making subsequent actions best-effort; either persist
the thread→issue mapping before calling locker.MarkProcessed or move the
MarkProcessed call to after the durable operations complete. Specifically update
the logic around locker.MarkProcessed, filer.SetIssueStatus(issue.Key, ...), and
the postMessageWithRetry(...) that posts supportRequestPrefix so that the
thread→issue mapping (used by handleReactionAdded) is written durably first or
MarkProcessed is invoked only after SetIssueStatus and the Slack post both
succeed (or after you persist the mapping elsewhere) to guarantee the transition
and link-back are reliable.
|
@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. |
1 participant
Add support-request handling for long forum threads by creating DPTP Jira tickets, posting thread guidance, and closing linked tickets from :closed: reactions with replica-safe locking and transition-aware Jira updates.
Summary by CodeRabbit
New Features
Tests
Documentation