Conversation
Add Slack as a new source type in the When struct, enabling ChatOps-driven agent execution from Slack messages via Socket Mode. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Greptile SummaryThis PR adds the Key changes:
Minor findings (all P2/non-blocking):
Confidence Score: 5/5Safe to merge — schema-only change with correct deepcopy generation and consistent CRD manifests; all remaining findings are P2 style/quality suggestions. No runtime behavior is introduced in this PR. The Go type definition, deepcopy, and both CRD YAML files are internally consistent. The three comments are all P2: missing ID format validation, undocumented template variables (deferred to PR 3 by design), and a redundant deepcopy line. None of these are blocking issues. api/v1alpha1/taskspawner_types.go — consider adding channel/user ID pattern validation and documenting Slack template variables before the implementation PRs land. Important Files Changed
Class Diagram%%{init: {'theme': 'neutral'}}%%
classDiagram
class When {
+GitHubIssues *GitHubIssues
+GitHubPullRequests *GitHubPullRequests
+Cron *Cron
+Jira *Jira
+GitHubWebhook *GitHubWebhook
+LinearWebhook *LinearWebhook
+Slack *Slack
}
class Slack {
+SecretRef SecretReference
+TriggerCommand string
+Channels []string
+AllowedUsers []string
+ResponseChannel string
+PollInterval string
}
class SecretReference {
+Name string
}
When --> Slack : "new in this PR"
Slack --> SecretReference
|
| return out | ||
| } | ||
|
|
||
| // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. |
There was a problem hiding this comment.
Redundant SecretRef assignment in DeepCopyInto
The line out.SecretRef = in.SecretRef on line 645 is redundant. The preceding *out = *in (line 644) already performs a full shallow copy of all value-type fields, which includes SecretRef (a struct containing only a Name string). The second assignment is harmless since SecretReference contains no pointer or slice fields, but it adds noise to generated code that's typically expected to be minimal.
If this file is truly autogenerated by controller-gen, regenerating it after updating the type should produce the canonical form. If the redundancy is intentional (e.g., the generator explicitly re-assigns embedded structs), this can be ignored.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
This file is auto-generated by controller-gen — we don't hand-edit it. The redundant assignment is how the generator handles embedded structs; every other source type in this file has the same pattern.
| // Channels optionally restricts which Slack channels the bot listens in. | ||
| // Values are channel IDs (e.g., "C0123456789"). When empty, the bot | ||
| // listens in every channel it has been invited to. | ||
| // +optional | ||
| Channels []string `json:"channels,omitempty"` | ||
|
|
||
| // AllowedUsers optionally restricts which Slack users can trigger tasks. | ||
| // Values are Slack user IDs (e.g., "U0123456789"). When empty, any user | ||
| // in the channel can trigger tasks. | ||
| // +optional | ||
| AllowedUsers []string `json:"allowedUsers,omitempty"` |
There was a problem hiding this comment.
No item-level validation on channel/user ID fields
Channels and AllowedUsers accept any strings as Slack IDs, but there is no validation constraining the format. Slack channel IDs always start with C (e.g. C0123456789) and user IDs always start with U (e.g. U0123456789). Empty strings in either list would pass validation today and silently produce unexpected filtering behavior at runtime (PR 2).
Consider adding +kubebuilder:validation:items:Pattern markers to catch obviously invalid IDs at admission time:
// +kubebuilder:validation:items:Pattern=`^C[A-Z0-9]+$`
Channels []string `json:"channels,omitempty"`
// +kubebuilder:validation:items:Pattern=`^U[A-Z0-9]+$`
AllowedUsers []string `json:"allowedUsers,omitempty"`Similarly, ResponseChannel at line 443 could carry +kubebuilder:validation:Pattern=^C[A-Z0-9]+$ to guard against typos.
There was a problem hiding this comment.
Skipping ID format validation for now — Slack IDs have varied prefixes beyond just C and U (e.g., G for group channels, D for DM channels, W for enterprise users). A regex like ^C[A-Z0-9]+$ would reject valid configs. We'll validate at runtime with clear error messages instead.
Bot responses will always be posted as thread replies to the originating message. No override is needed. Co-Authored-By: Claude Opus 4.6 <[email protected]>
What type of PR is this?
/kind feature
What this PR does / why we need it:
Adds the
Slackstruct to theWhenunion type in the TaskSpawner CRD, enabling Slack as a new task source. This is the foundational schema change — no runtime behavior yet.The
Slacksource uses Socket Mode (outbound WebSocket, no ingress required) and supports:Which issue(s) this PR is related to:
Part of kelos-dev#595
Towards AIE-17
Special notes for your reviewer:
This is PR 1 of 4 for the Slack integration. Subsequent PRs will add:
The
slack-go/slackGo dependency is not added in this PR —go mod tidyremoves it since nothing imports it yet. It will be added in PR 2.Does this PR introduce a user-facing change?