feat(slack-bot): automate support-request Jira workflow#5111
Conversation
|
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
openshift-ci
Bot
added
the
approved
|
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:
WalkthroughAdds a Slack "support-request" mode: when forum threads exceed a configurable reply threshold the bot files a Jira issue (with activity-type and optional PagerDuty-based reassignment), posts the Jira link in-thread, and supports closing the Jira via a Changes
Sequence Diagram(s)sequenceDiagram
participant User as Slack User
participant Bot as Slack Bot
participant Lock as Lock Client
participant CM as K8s ConfigMap
participant PD as PagerDuty
participant Jira as Jira
User->>Bot: Post message in thread
Bot->>Bot: Count replies, check threshold & existing Jira marker
Bot->>Lock: Acquire(threadTS)
Lock->>CM: Read/Update lock entry
CM-->>Lock: Lock acquired
Lock-->>Bot: Acquired
Bot->>PD: (optional) Query on-call
PD-->>Bot: On-call user/email
Bot->>Jira: FileIssue(permalink, activity-type, reporter)
Jira-->>Bot: Issue created (KEY)
Bot->>Jira: SetIssueStatus(KEY, "In Progress")
Jira-->>Bot: OK
Bot->>Bot: Post Jira link in thread
Bot->>Lock: MarkProcessed(threadTS, KEY)
Lock->>CM: Set processed:<KEY>
User->>Bot: Add :closed: reaction on root message
Bot->>Bot: Resolve issueKey from lock or thread replies
Bot->>Jira: CloseIssue(issueKey, "Done")
Jira-->>Bot: Closed
Bot->>Bot: Post closure confirmation to thread
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 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 |
![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
| // 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.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/slack-bot/main.go (1)
228-241:⚠️ Potential issue | 🟠 MajorDouble-pointer bug in
loadConfig.The
loadConfigfunction takesconfig interface{}and then passes&configtoyaml.Unmarshal. WhenloadKeywordsConfigcallsloadConfig(configPath, config)whereconfigis already aninterface{}holding a pointer, this creates a double-pointer situation that will fail to unmarshal correctly.🐛 Proposed fix
-func loadConfig(configPath string, config interface{}) error { +func loadConfig(configPath string, config interface{}) error { configContent, err := os.ReadFile(configPath) if err != nil { return fmt.Errorf("failed to read config: %w", err) } - if err = yaml.Unmarshal(configContent, &config); err != nil { + if err = yaml.Unmarshal(configContent, config); err != nil { return fmt.Errorf("failed to unmarshall config: %w", err) } return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/slack-bot/main.go` around lines 228 - 241, The loadConfig function is passing &config (a pointer to the empty-interface value) into yaml.Unmarshal which creates a double-pointer when callers (like loadKeywordsConfig) already pass a pointer; change loadConfig to pass config directly into yaml.Unmarshal (i.e., use yaml.Unmarshal(configContent, config)) and ensure callers supply a pointer to the concrete struct (e.g., *MyConfig) so Unmarshal writes into that pointer; keep the function signature loadConfig(configPath string, config interface{}) error and update any callers that might pass non-pointers to pass pointers instead.
🧹 Nitpick comments (2)
pkg/slack/events/supportrequest/lock.go (1)
17-23: Consider adding cleanup for old processed entries.The ConfigMap stores processed thread mappings indefinitely. Over time, this could grow to include thousands of entries. Consider adding a cleanup mechanism (e.g., periodic job or TTL for processed entries) to prevent unbounded growth.
🤖 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 17 - 23, The ConfigMap keys under threadLockMapName use lockStateProcessedPrefix and currently never expire, which can lead to unbounded growth; add a cleanup mechanism that periodically scans entries with lockStateProcessedPrefix and deletes those older than a configurable retention (e.g., processedRetentionDuration) — implement this as a background goroutine or a StartLockCleanup(retention, interval) function that reads values (or stores a timestamp when marking processed), parses the timestamp or expiry stored alongside the processed state, and prunes stale keys on each interval using the same ConfigMap client used by the lock code; ensure the new logic uses threadLockMapName and lockStateProcessedPrefix to identify candidates and expose the retention/interval as config for testing.pkg/jira/issues.go (1)
255-261: Jira user lookup may fail when multiple users match the email.The code expects exactly one Jira user for the helpdesk email, but
FindUserswithmaxResults=20could return multiple results if the email is a partial match or multiple accounts exist. Consider filtering results more precisely or handling multiple matches gracefully.♻️ Suggested approach
jiraUsers, response, err := f.jiraClient.FindUsers(url.QueryEscape(assigneeEmail), 20) if err := jirautil.HandleJiraError(response, err); err != nil { return fmt.Errorf("could not find jira user by helpdesk email: %w", err) } - if len(jiraUsers) != 1 { - return fmt.Errorf("could not find a single jira user for helpdesk email %s: got %d", assigneeEmail, len(jiraUsers)) + if len(jiraUsers) == 0 { + return fmt.Errorf("could not find jira user for helpdesk email %s", assigneeEmail) + } + // Use first match; Jira's user search typically returns best matches first + if len(jiraUsers) > 1 { + logger.WithField("email", assigneeEmail).Debug("Multiple Jira users found for helpdesk email, using first match") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/jira/issues.go` around lines 255 - 261, The current lookup assumes exactly one result from f.jiraClient.FindUsers which can return multiple partial matches; after calling FindUsers and handling errors via jirautil.HandleJiraError, filter the returned jiraUsers slice for an exact match against assigneeEmail (case-insensitive compare to the user's EmailAddress or other authoritative field returned by the client), then if exactly one match use that, if zero keep the existing "could not find" error, and if >1 return a clear error listing the ambiguous matches (e.g., their accountIds or emails) so the caller can disambiguate; update the code around FindUsers, jiraUsers and the error returns to implement this filtering and improved error for multiple matches.
🤖 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/issues.go`:
- Around line 296-298: The PagerDuty time parameters are being passed with
since.String()/until.String() which use Go's default time format; update the
calls to use RFC3339 by replacing since.String() and until.String() with
since.Format(time.RFC3339) and until.Format(time.RFC3339) in the
pagerduty.ListOnCallUsers invocation (look for the ListOnCallUsers call and
pagerduty.ListOnCallUsersOptions in pkg/jira/issues.go), and make the same
change where since/until are used in cmd/sprint-automation/main.go so all
PagerDuty API time parameters use ISO 8601.
---
Outside diff comments:
In `@cmd/slack-bot/main.go`:
- Around line 228-241: The loadConfig function is passing &config (a pointer to
the empty-interface value) into yaml.Unmarshal which creates a double-pointer
when callers (like loadKeywordsConfig) already pass a pointer; change loadConfig
to pass config directly into yaml.Unmarshal (i.e., use
yaml.Unmarshal(configContent, config)) and ensure callers supply a pointer to
the concrete struct (e.g., *MyConfig) so Unmarshal writes into that pointer;
keep the function signature loadConfig(configPath string, config interface{})
error and update any callers that might pass non-pointers to pass pointers
instead.
---
Nitpick comments:
In `@pkg/jira/issues.go`:
- Around line 255-261: The current lookup assumes exactly one result from
f.jiraClient.FindUsers which can return multiple partial matches; after calling
FindUsers and handling errors via jirautil.HandleJiraError, filter the returned
jiraUsers slice for an exact match against assigneeEmail (case-insensitive
compare to the user's EmailAddress or other authoritative field returned by the
client), then if exactly one match use that, if zero keep the existing "could
not find" error, and if >1 return a clear error listing the ambiguous matches
(e.g., their accountIds or emails) so the caller can disambiguate; update the
code around FindUsers, jiraUsers and the error returns to implement this
filtering and improved error for multiple matches.
In `@pkg/slack/events/supportrequest/lock.go`:
- Around line 17-23: The ConfigMap keys under threadLockMapName use
lockStateProcessedPrefix and currently never expire, which can lead to unbounded
growth; add a cleanup mechanism that periodically scans entries with
lockStateProcessedPrefix and deletes those older than a configurable retention
(e.g., processedRetentionDuration) — implement this as a background goroutine or
a StartLockCleanup(retention, interval) function that reads values (or stores a
timestamp when marking processed), parses the timestamp or expiry stored
alongside the processed state, and prunes stale keys on each interval using the
same ConfigMap client used by the lock code; ensure the new logic uses
threadLockMapName and lockStateProcessedPrefix to identify candidates and expose
the retention/interval as config for testing.
🪄 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: dec78fca-de49-484b-a8e5-407427469a9c
📒 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
✅ 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/slack/events/router/router.go
jmguzik
force-pushed
the
jira-ops
branch
2 times, most recently
from
2bf2b19 to
14a9ccd
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/slack-bot/main.go (1)
228-234:⚠️ Potential issue | 🔴 CriticalRemove the
&operator on line 233 to pass the actual pointer through the interface, not a pointer to the interface.When
loadKeywordsConfigcallsloadConfigwith&keywordsConfig, the pointer is wrapped in theinterface{}parameter. On line 233, taking&configcreates a*interface{}, which preventsyaml.Unmarshalfrom type-asserting and unmarshaling into the original struct. This silently leaves the config empty even when the file is valid.Fix
- if err = yaml.Unmarshal(configContent, &config); err != nil { + if err = yaml.Unmarshal(configContent, config); err != nil {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/slack-bot/main.go` around lines 228 - 234, The yaml.Unmarshal call in loadConfig is passing a pointer to the interface value (using &config) which creates a *interface{} and prevents unmarshaling into the underlying struct; change the call in function loadConfig to pass the interface value directly (use config, not &config) so the original pointer (e.g., &keywordsConfig) received via the interface is used as the destination for yaml.Unmarshal.
♻️ Duplicate comments (1)
pkg/slack/events/supportrequest/handler.go (1)
159-178:⚠️ Potential issue | 🟠 MajorDon't abort after
FileIssuewithout leaving a durable lookup path.If
locker.MarkProcessedfails on Lines 162-163, the Jira issue already exists but we return before posting the Slack marker. That leaves the thread with no discoverable thread→issue mapping, so once the processing TTL expires a later reply can file a second ticket for the same thread. Please keep the fallback marker path alive even when the lock write fails, and only treat the lock as durable afterMarkProcessedsucceeds.🤖 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 159 - 178, The code currently sets shouldReleaseLock = false then returns if locker.MarkProcessed(event.ThreadTimeStamp, issue.Key) fails, leaving no durable thread→issue mapping; change the flow so that: attempt locker.MarkProcessed(event.ThreadTimeStamp, issue.Key) and if it returns an error, log the error (using logger.WithError(...).Warn(...)) but do NOT return—continue to post the Slack marker via postMessageWithRetry so the thread has a discoverable mapping; only set shouldReleaseLock = false after locker.MarkProcessed succeeds (so the lock is treated as durable), and keep the existing call to filer.SetIssueStatus(issue.Key, jira.StatusInProgress, logger) and the postMessageWithRetry logic unchanged except for removing the early return on MarkProcessed failure.
🧹 Nitpick comments (1)
cmd/slack-bot/main.go (1)
118-119: Default this feature off until rollout is explicit.Using the forum channel as the default here means the new ticket-creating automation turns on as soon as this binary is deployed, even if no deployment config was updated. For a brand-new workflow, an empty default for
--support-request-channel-idis a much safer rollout posture.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/slack-bot/main.go` around lines 118 - 119, The default channel ID for the support-request feature is set to "CBN38N3MW" causing the automation to enable on deploy; change the flag registration for supportRequestChannelID (the call that sets --support-request-channel-id) to use an empty string as the default instead of "CBN38N3MW" so the feature is disabled by default, and optionally update the flag description to note empty means disabled; keep the supportRequestThreshold flag as-is (supportRequestThreshold / --support-request-threshold).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@cmd/slack-bot/main.go`:
- Around line 228-234: The yaml.Unmarshal call in loadConfig is passing a
pointer to the interface value (using &config) which creates a *interface{} and
prevents unmarshaling into the underlying struct; change the call in function
loadConfig to pass the interface value directly (use config, not &config) so the
original pointer (e.g., &keywordsConfig) received via the interface is used as
the destination for yaml.Unmarshal.
---
Duplicate comments:
In `@pkg/slack/events/supportrequest/handler.go`:
- Around line 159-178: The code currently sets shouldReleaseLock = false then
returns if locker.MarkProcessed(event.ThreadTimeStamp, issue.Key) fails, leaving
no durable thread→issue mapping; change the flow so that: attempt
locker.MarkProcessed(event.ThreadTimeStamp, issue.Key) and if it returns an
error, log the error (using logger.WithError(...).Warn(...)) but do NOT
return—continue to post the Slack marker via postMessageWithRetry so the thread
has a discoverable mapping; only set shouldReleaseLock = false after
locker.MarkProcessed succeeds (so the lock is treated as durable), and keep the
existing call to filer.SetIssueStatus(issue.Key, jira.StatusInProgress, logger)
and the postMessageWithRetry logic unchanged except for removing the early
return on MarkProcessed failure.
---
Nitpick comments:
In `@cmd/slack-bot/main.go`:
- Around line 118-119: The default channel ID for the support-request feature is
set to "CBN38N3MW" causing the automation to enable on deploy; change the flag
registration for supportRequestChannelID (the call that sets
--support-request-channel-id) to use an empty string as the default instead of
"CBN38N3MW" so the feature is disabled by default, and optionally update the
flag description to note empty means disabled; keep the supportRequestThreshold
flag as-is (supportRequestThreshold / --support-request-threshold).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: afa64123-c1c9-43b4-bbd3-3677c160877a
📒 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
✅ 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 (2)
- pkg/jira/issues.go
- pkg/jira/issues_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pkg/jira/issues.go (1)
296-298:⚠️ Potential issue | 🟠 MajorUse RFC3339 format for PagerDuty time parameters.
since.String()anduntil.String()produce Go's default time format (2006-01-02 15:04:05.999999999 -0700 MST), but the PagerDuty API requires ISO 8601 format. This same issue exists on lines 310-312.🐛 Proposed fix
users, err := client.ListOnCallUsers(schedule.ID, pagerduty.ListOnCallUsersOptions{ - Since: since.String(), - Until: until.String(), + Since: since.Format(time.RFC3339), + Until: until.Format(time.RFC3339), })And similarly for
ListOverrides:overrides, err := client.ListOverrides(schedule.ID, pagerduty.ListOverridesOptions{ - Since: since.String(), - Until: until.String(), + Since: since.Format(time.RFC3339), + Until: until.Format(time.RFC3339), })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/jira/issues.go` around lines 296 - 298, The PagerDuty time parameters are being passed as since.String() and until.String(), which produce Go's default time format; change these to use RFC3339 (ISO 8601) by formatting the time values with time.RFC3339 (e.g., since.Format(time.RFC3339) and until.Format(time.RFC3339)) when building pagerduty.ListOnCallUsersOptions and similarly for ListOverrides calls so the Since and Until fields meet PagerDuty's expected ISO 8601 format.
🧹 Nitpick comments (3)
pkg/slack/events/supportrequest/lock.go (2)
151-154: SHA1 truncation is acceptable here but consider documenting collision bounds.Using 8 bytes (64 bits) of SHA1 for key derivation means ~2^32 threads before expecting a collision (birthday bound). This is safe for typical Slack channel volumes, but a brief inline comment would help future readers understand the tradeoff.
🤖 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 151 - 154, The lockNameForThread function currently returns the first 8 bytes of SHA1 without documenting collision risk; update the function (lockNameForThread) to include a concise inline comment explaining that truncating SHA1 to 8 bytes yields ~64-bit identifiers with a birthday-collision threshold around 2^32 items, why this is acceptable for expected Slack thread volumes, and note the tradeoff and how to increase length if stronger collision resistance is later required.
64-64: Consider adding context timeouts for Kubernetes API calls.All Kubernetes client operations use
context.TODO(), which never times out. If the API server becomes unresponsive, the handler could block indefinitely. Since this runs in an event handler path, consider using a context with a reasonable timeout.♻️ Example refactor
+const lockOperationTimeout = 10 * time.Second + func (c *configMapLockClient) Acquire(threadTS string) (bool, error) { key := lockNameForThread(threadTS) for i := 0; i < maxLockRetries; i++ { + ctx, cancel := context.WithTimeout(context.Background(), lockOperationTimeout) lockMap, err := c.getOrCreateLockMap() + cancel() if err != nil { return false, err }Apply similar changes to
MarkProcessed,GetProcessedIssueKey,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` at line 64, The Kubernetes client calls currently use context.TODO() and can block indefinitely; update them to use a context with timeout (e.g., ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second); defer cancel()) and pass that ctx into c.client.Update and all other API calls inside Lock.Update, MarkProcessed, GetProcessedIssueKey, Release, and getOrCreateLockMap so operations time out if the API server is unresponsive; ensure you import time and always call defer cancel() after creating the timed context.pkg/slack/events/supportrequest/handler.go (1)
246-246: Nit: empty line at start of function body.Line 246 is an empty line immediately after the function signature opening brace.
♻️ Cosmetic fix
func closeIssueAndNotify(client messageClient, filer jira.IssueFiler, channelID, threadTS, issueKey string, logger *logrus.Entry) (bool, error) { - closed, err := filer.CloseIssue(issueKey, jira.ResolutionDone, logger)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/slack/events/supportrequest/handler.go` at line 246, Remove the stray empty line immediately after the function's opening brace in the supportrequest handler function (the function whose body currently starts with a blank line in pkg/slack/events/supportrequest/handler.go); place the first statement directly after the opening brace so the function body has no leading blank line and ensure no other leading whitespace-only lines remain.
🤖 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/issues.go`:
- Around line 303-319: When len(users) > 1 and overrides.Lookup doesn't yield
exactly one override, don't fail outright; instead treat the ambiguity by
returning the first user as a sensible fallback. Modify the logic around the
client.ListOverrides call (referencing schedule.ID,
pagerduty.ListOverridesOptions, overrides.Overrides and the users slice) so that
if err != nil you still return the wrapped error, but if
len(overrides.Overrides) != 1 you return &users[0] (and optionally record a
warning via your logger) instead of returning a fmt.Errorf; update the
error/message to reflect this fallback behavior.
---
Duplicate comments:
In `@pkg/jira/issues.go`:
- Around line 296-298: The PagerDuty time parameters are being passed as
since.String() and until.String(), which produce Go's default time format;
change these to use RFC3339 (ISO 8601) by formatting the time values with
time.RFC3339 (e.g., since.Format(time.RFC3339) and until.Format(time.RFC3339))
when building pagerduty.ListOnCallUsersOptions and similarly for ListOverrides
calls so the Since and Until fields meet PagerDuty's expected ISO 8601 format.
---
Nitpick comments:
In `@pkg/slack/events/supportrequest/handler.go`:
- Line 246: Remove the stray empty line immediately after the function's opening
brace in the supportrequest handler function (the function whose body currently
starts with a blank line in pkg/slack/events/supportrequest/handler.go); place
the first statement directly after the opening brace so the function body has no
leading blank line and ensure no other leading whitespace-only lines remain.
In `@pkg/slack/events/supportrequest/lock.go`:
- Around line 151-154: The lockNameForThread function currently returns the
first 8 bytes of SHA1 without documenting collision risk; update the function
(lockNameForThread) to include a concise inline comment explaining that
truncating SHA1 to 8 bytes yields ~64-bit identifiers with a birthday-collision
threshold around 2^32 items, why this is acceptable for expected Slack thread
volumes, and note the tradeoff and how to increase length if stronger collision
resistance is later required.
- Line 64: The Kubernetes client calls currently use context.TODO() and can
block indefinitely; update them to use a context with timeout (e.g., ctx, cancel
:= context.WithTimeout(context.Background(), 5*time.Second); defer cancel()) and
pass that ctx into c.client.Update and all other API calls inside Lock.Update,
MarkProcessed, GetProcessedIssueKey, Release, and getOrCreateLockMap so
operations time out if the API server is unresponsive; ensure you import time
and always call defer cancel() after creating the timed context.
🪄 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: 9e3afa36-5575-450a-aea2-b21ed40bfa4b
📒 Files selected for processing (11)
cmd/slack-bot/README.mdcmd/slack-bot/main.gogo.modpkg/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
✅ 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 (2)
- cmd/slack-bot/main.go
- pkg/jira/issues_test.go
| if len(users) == 0 { | ||
| return nil, fmt.Errorf("did not get any users when querying PagerDuty for the '%s' on-call", query) | ||
| } | ||
| if len(users) == 1 { | ||
| return &users[0], nil | ||
| } | ||
|
|
||
| overrides, err := client.ListOverrides(schedule.ID, pagerduty.ListOverridesOptions{ | ||
| Since: since.String(), | ||
| Until: until.String(), | ||
| }) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not query PagerDuty for the '%s' overrides: %w", query, err) | ||
| } | ||
| if len(overrides.Overrides) != 1 { | ||
| return nil, fmt.Errorf("did not get exactly one override when querying PagerDuty for the '%s' overrides: %d", query, len(overrides.Overrides)) | ||
| } |
There was a problem hiding this comment.
Edge case: multiple on-call users without exactly one override.
When len(users) > 1, the code assumes there's exactly one override to resolve ambiguity. However, multiple users can be on-call simultaneously without overrides (e.g., during shift handover windows or multi-person schedules). Consider returning the first user as a fallback, or documenting why this strict check is intentional.
🛡️ Possible fallback approach
if len(overrides.Overrides) != 1 {
- return nil, fmt.Errorf("did not get exactly one override when querying PagerDuty for the '%s' overrides: %d", query, len(overrides.Overrides))
+ // No single override to disambiguate; return first scheduled user
+ return &users[0], nil
}📝 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.
| if len(users) == 0 { | |
| return nil, fmt.Errorf("did not get any users when querying PagerDuty for the '%s' on-call", query) | |
| } | |
| if len(users) == 1 { | |
| return &users[0], nil | |
| } | |
| overrides, err := client.ListOverrides(schedule.ID, pagerduty.ListOverridesOptions{ | |
| Since: since.String(), | |
| Until: until.String(), | |
| }) | |
| if err != nil { | |
| return nil, fmt.Errorf("could not query PagerDuty for the '%s' overrides: %w", query, err) | |
| } | |
| if len(overrides.Overrides) != 1 { | |
| return nil, fmt.Errorf("did not get exactly one override when querying PagerDuty for the '%s' overrides: %d", query, len(overrides.Overrides)) | |
| } | |
| if len(users) == 0 { | |
| return nil, fmt.Errorf("did not get any users when querying PagerDuty for the '%s' on-call", query) | |
| } | |
| if len(users) == 1 { | |
| return &users[0], nil | |
| } | |
| overrides, err := client.ListOverrides(schedule.ID, pagerduty.ListOverridesOptions{ | |
| Since: since.String(), | |
| Until: until.String(), | |
| }) | |
| if err != nil { | |
| return nil, fmt.Errorf("could not query PagerDuty for the '%s' overrides: %w", query, err) | |
| } | |
| if len(overrides.Overrides) != 1 { | |
| // No single override to disambiguate; return first scheduled user | |
| return &users[0], nil | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/jira/issues.go` around lines 303 - 319, When len(users) > 1 and
overrides.Lookup doesn't yield exactly one override, don't fail outright;
instead treat the ambiguity by returning the first user as a sensible fallback.
Modify the logic around the client.ListOverrides call (referencing schedule.ID,
pagerduty.ListOverridesOptions, overrides.Overrides and the users slice) so that
if err != nil you still return the wrapped error, but if
len(overrides.Overrides) != 1 you return &users[0] (and optionally record a
warning via your logger) instead of returning a fmt.Errorf; update the
error/message to reflect this fallback behavior.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
pkg/jira/issues.go (2)
296-298:⚠️ Potential issue | 🟠 MajorUse RFC3339 format for PagerDuty time parameters.
since.String()anduntil.String()produce Go's default time format (2006-01-02 15:04:05.999999999 -0700 MST), but the PagerDuty API requires ISO 8601 format.Suggested fix
users, err := client.ListOnCallUsers(schedule.ID, pagerduty.ListOnCallUsersOptions{ - Since: since.String(), - Until: until.String(), + Since: since.Format(time.RFC3339), + Until: until.Format(time.RFC3339), })Also apply to Lines 311-312:
overrides, err := client.ListOverrides(schedule.ID, pagerduty.ListOverridesOptions{ - Since: since.String(), - Until: until.String(), + Since: since.Format(time.RFC3339), + Until: until.Format(time.RFC3339), }),
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/jira/issues.go` around lines 296 - 298, The PagerDuty time parameters are being passed using since.String()/until.String(), which yields Go's default time format; update the calls to client.ListOnCallUsers in this file (the invocation that creates users, err := client.ListOnCallUsers(schedule.ID, pagerduty.ListOnCallUsersOptions{...}) and the second similar invocation later) to pass times formatted as RFC3339 by replacing Since: since.String(), Until: until.String() with Since: since.Format(time.RFC3339), Until: until.Format(time.RFC3339) and import the time package if not already imported.
303-319:⚠️ Potential issue | 🟡 MinorEdge case: multiple on-call users without exactly one override.
When
len(users) > 1, the code requires exactly one override to resolve ambiguity. However, multiple users can be on-call simultaneously during shift handover windows or multi-person schedules without overrides. Consider returning the first user as a fallback instead of erroring.Possible fallback approach
if len(overrides.Overrides) != 1 { - return nil, fmt.Errorf("did not get exactly one override when querying PagerDuty for the '%s' overrides: %d", query, len(overrides.Overrides)) + // No single override to disambiguate; return first scheduled user + return &users[0], nil },
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/jira/issues.go` around lines 303 - 319, When len(users) > 1 and the override lookup (client.ListOverrides using schedule.ID, Since/Until) does not return exactly one override, do not return an error; instead return the first user as a deterministic fallback (return &users[0], nil). Keep the existing error handling for the client.ListOverrides call itself, but replace the fmt.Errorf branch that errors on len(overrides.Overrides) != 1 with logic that logs or comments the ambiguity and returns &users[0] to resolve multi-person on-call windows.
🧹 Nitpick comments (3)
pkg/slack/events/supportrequest/lock.go (1)
40-73: Consider propagating context for cancellation support.All Kubernetes client operations use
context.TODO(). For better operational control (e.g., graceful shutdown, request timeouts), consider accepting acontext.Contextparameter and propagating it to client calls.Example signature change
func (c *configMapLockClient) Acquire(ctx context.Context, threadTS string) (bool, error) { // ... if err := c.client.Update(ctx, lockMap); err != nil {🤖 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 Acquire to accept and propagate a context so Kubernetes calls can be cancelled: update the method signature of configMapLockClient.Acquire to func (c *configMapLockClient) Acquire(ctx context.Context, threadTS string) (bool, error) and replace uses of context.TODO() inside (notably the c.client.Update call and any other client calls in this function) to use the passed ctx; ensure callers are updated accordingly to provide a context.pkg/slack/events/supportrequest/lock_test.go (1)
61-81: Consider adding a comment explaining time variable capture.The test relies on modifying
nowafter the closure is created (Line 73), which works because Go closures capture variables by reference. This is correct but might confuse future readers.+ // now is captured by reference, allowing test to advance time by reassigning now = now.Add(48 * time.Hour)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/slack/events/supportrequest/lock_test.go` around lines 61 - 81, Add a short clarifying comment above the now variable/closure in TestConfigMapLockProcessedNeverReacquires explaining that the time provider passed into newTestLockClient is a closure that captures the now variable by reference, so mutating now later (now = now.Add(...)) will change the time returned by the closure; reference the time variable name "now" and the closure func() time.Time and the test helper "newTestLockClient" so reviewers can quickly find and understand the behavior.pkg/slack/events/router/router.go (1)
20-20: Consider a configuration struct for cleaner signature.
ForEventsnow has 13 parameters. While this follows existing patterns, extracting support-request-specific parameters into a struct could improve readability for future extensions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/slack/events/router/router.go` at line 20, The ForEvents function signature is too long; extract related parameters into a dedicated config struct (e.g., SupportRequestConfig) and replace the long parameter list with that struct to improve readability. Create a struct containing supportRequestThreadMessageThreshold, requireWorkflowsInForum, helpdeskAlias, forumChannelId, reviewRequestWorkflowID, namespace, supportRequestChannelID (and any other support-request-specific fields) and change ForEvents(client *slack.Client, filer jira.IssueFiler, kubeClient ctrlruntimeclient.Client, config config.Getter, gcsClient *storage.Client, keywordsConfig helpdesk.KeywordsConfig, supportCfg SupportRequestConfig) events.Handler; update all callers of ForEvents to construct and pass the new SupportRequestConfig, and adjust any tests or documentation that reference the old parameter order.
🤖 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/handler.go`:
- Around line 154-158: If filer.FileIssue succeeds but locker.MarkProcessed
fails, log and surface the created Jira issue key to avoid orphaned issues:
after calling filer.FileIssue (the variable issue), ensure the MarkProcessed
error handler includes the issue.Key (or issue.ID) in the processLogger/error
logs and in the postMessageWithRetry Slack notification (e.g., augment the
"Failed to mark processed" message with the created issue key and a link or
identifier) so operators can manually clean up or track the orphaned Jira
ticket.
---
Duplicate comments:
In `@pkg/jira/issues.go`:
- Around line 296-298: The PagerDuty time parameters are being passed using
since.String()/until.String(), which yields Go's default time format; update the
calls to client.ListOnCallUsers in this file (the invocation that creates users,
err := client.ListOnCallUsers(schedule.ID,
pagerduty.ListOnCallUsersOptions{...}) and the second similar invocation later)
to pass times formatted as RFC3339 by replacing Since: since.String(), Until:
until.String() with Since: since.Format(time.RFC3339), Until:
until.Format(time.RFC3339) and import the time package if not already imported.
- Around line 303-319: When len(users) > 1 and the override lookup
(client.ListOverrides using schedule.ID, Since/Until) does not return exactly
one override, do not return an error; instead return the first user as a
deterministic fallback (return &users[0], nil). Keep the existing error handling
for the client.ListOverrides call itself, but replace the fmt.Errorf branch that
errors on len(overrides.Overrides) != 1 with logic that logs or comments the
ambiguity and returns &users[0] to resolve multi-person on-call windows.
---
Nitpick comments:
In `@pkg/slack/events/router/router.go`:
- Line 20: The ForEvents function signature is too long; extract related
parameters into a dedicated config struct (e.g., SupportRequestConfig) and
replace the long parameter list with that struct to improve readability. Create
a struct containing supportRequestThreadMessageThreshold,
requireWorkflowsInForum, helpdeskAlias, forumChannelId, reviewRequestWorkflowID,
namespace, supportRequestChannelID (and any other support-request-specific
fields) and change ForEvents(client *slack.Client, filer jira.IssueFiler,
kubeClient ctrlruntimeclient.Client, config config.Getter, gcsClient
*storage.Client, keywordsConfig helpdesk.KeywordsConfig, supportCfg
SupportRequestConfig) events.Handler; update all callers of ForEvents to
construct and pass the new SupportRequestConfig, and adjust any tests or
documentation that reference the old parameter order.
In `@pkg/slack/events/supportrequest/lock_test.go`:
- Around line 61-81: Add a short clarifying comment above the now
variable/closure in TestConfigMapLockProcessedNeverReacquires explaining that
the time provider passed into newTestLockClient is a closure that captures the
now variable by reference, so mutating now later (now = now.Add(...)) will
change the time returned by the closure; reference the time variable name "now"
and the closure func() time.Time and the test helper "newTestLockClient" so
reviewers can quickly find and understand the behavior.
In `@pkg/slack/events/supportrequest/lock.go`:
- Around line 40-73: Change Acquire to accept and propagate a context so
Kubernetes calls can be cancelled: update the method signature of
configMapLockClient.Acquire to func (c *configMapLockClient) Acquire(ctx
context.Context, threadTS string) (bool, error) and replace uses of
context.TODO() inside (notably the c.client.Update call and any other client
calls in this function) to use the passed ctx; ensure callers are updated
accordingly to provide a context.
🪄 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: 48519143-74e8-484b-a13c-40e8fbb7e454
📒 Files selected for processing (11)
cmd/slack-bot/README.mdcmd/slack-bot/main.gogo.modpkg/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
✅ Files skipped from review due to trivial changes (2)
- cmd/slack-bot/README.md
- go.mod
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/jira/issues_test.go
- cmd/slack-bot/main.go
| issue, err := filer.FileIssue(jira.IssueTypeTask, title, description, event.User, logger) | ||
| if err != nil { | ||
| _ = postMessageWithRetry(client, channelID, slack.MsgOptionText("Failed to create corresponding support request in Jira.", false), slack.MsgOptionTS(event.ThreadTimeStamp)) | ||
| return true, err | ||
| } |
There was a problem hiding this comment.
Orphaned Jira issue on MarkProcessed failure.
If filer.FileIssue succeeds but locker.MarkProcessed fails (Line 162), the Jira issue is created but the handler returns an error, potentially leaving an orphaned issue that could be recreated on retry. This is an unlikely edge case, but worth documenting or logging the created issue key for manual cleanup.
Suggested logging improvement
if err := locker.MarkProcessed(event.ThreadTimeStamp, issue.Key); err != nil {
+ logger.WithField("issue", issue.Key).Error("Failed to persist thread-to-issue mapping; Jira issue may be orphaned")
return true, err
}🤖 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 - 158, If
filer.FileIssue succeeds but locker.MarkProcessed fails, log and surface the
created Jira issue key to avoid orphaned issues: after calling filer.FileIssue
(the variable issue), ensure the MarkProcessed error handler includes the
issue.Key (or issue.ID) in the processLogger/error logs and in the
postMessageWithRetry Slack notification (e.g., augment the "Failed to mark
processed" message with the created issue key and a link or identifier) so
operators can manually clean up or track the orphaned Jira ticket.
There was a problem hiding this comment.
its safe to ignore imo
There was a problem hiding this comment.
@jmguzik Understood, acknowledged. The edge case is unlikely in practice given the locking mechanism, and the Jira issue would still be discoverable in the project if needed. Thanks for the context!
jmguzik
force-pushed
the
jira-ops
branch
2 times, most recently
from
33cfeed to
9932793
Compare
|
/test images |
jmguzik
force-pushed
the
jira-ops
branch
2 times, most recently
from
6e1cd39 to
82214cf
Compare
jmguzik
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
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. |
openshift-ci
Bot
added
the
needs-rebase
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
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. Co-authored with Cursor. Signed-off-by: Jakub Guzik <jguzik@redhat.com>
openshift-ci
Bot
removed
the
needs-rebase
Signed-off-by: Jakub Guzik <jguzik@redhat.com>
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
|
Scheduling tests matching the |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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 |
| } | ||
|
|
||
| // HandlerWithLock creates a handler with a cross-replica lock implementation. | ||
| func HandlerWithLock(client messageClient, filer jira.IssueFiler, channelID string, threadMessageThreshold int, locker lockClient) events.PartialHandler { |
There was a problem hiding this comment.
You have two constructors: Handler and HandlerWithLock. It's a sign you might consider using the "Golang Functional Options Pattern".
| return retryableErr.Retryable() | ||
| } | ||
| var netErr net.Error | ||
| if errors.As(err, &netErr) { |
There was a problem hiding this comment.
Why net.Error is retryable?
| lockStateProcessedPrefix = "processed:" | ||
| ) | ||
|
|
||
| type configMapLockClient struct { |
There was a problem hiding this comment.
Why a ConfigMap for locking?
|
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. |
|
@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. |
openshift-merge-bot
Bot
merged commit 260ebf2
into
openshift:main
3 participants
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