Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThe PR removes enterprise-level Slack connector functionality, including SCIM/IDP group support, enterprise role management, and configuration fields. Enterprise API endpoints, client methods, action handlers, and resource builders are deleted across the codebase. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR removes enterprise dependencies and integration, simplifying the Slack connector to use only the regular Slack API. The changes eliminate support for Enterprise Grid features, SCIM API operations, and account provisioning capabilities.
- Removed enterprise client and all enterprise-specific functionality
- Simplified configuration by removing enterprise token, SSO, and gov environment settings
- Removed support for user groups, IDP groups, enterprise roles, and provisioning operations
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/helpers.go | Removed enterprise-specific pagination types and helper functions |
| pkg/connector/workspace_test.go | Removed tests for workspace grant/revoke operations |
| pkg/connector/workspace.go | Simplified workspace builder and removed grant/revoke methods; removed enterprise role grants |
| pkg/connector/user_group.go | Deleted entire user group resource type implementation |
| pkg/connector/user.go | Simplified user builder and removed account provisioning capability |
| pkg/connector/roles.go | Simplified workspace role builder and removed grant/revoke methods |
| pkg/connector/resource_types.go | Removed user group, IDP group, and enterprise role resource types |
| pkg/connector/group.go | Deleted entire IDP group resource type implementation |
| pkg/connector/enterprise_roles.go | Deleted entire enterprise role resource type implementation |
| pkg/connector/connector.go | Removed enterprise client initialization and action manager registration |
| pkg/connector/client/*.go | Deleted all enterprise client implementation files |
| pkg/connector/actions.go | Deleted account enable/disable action implementations |
| pkg/config/config.go | Removed enterprise token, SSO enabled, and gov environment configuration fields |
| pkg/config/conf.gen.go | Updated generated config struct to remove enterprise fields |
| baton_capabilities.json | Updated capabilities to reflect removal of provisioning and enterprise features |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| options := slack.GetUsersOptionTeamID(resource.Id.Resource) | ||
| users, err := o.client.GetUsersContext(ctx, options) | ||
| if err != nil { | ||
| return nil, nil, err | ||
| } | ||
|
|
||
| outputAnnotations := annotations.New() | ||
| users, nextCursor, ratelimitData, err := o.enterpriseClient.GetUsers( | ||
| ctx, | ||
| resource.Id.Resource, | ||
| bag.PageToken(), | ||
| ) | ||
| outputAnnotations.WithRateLimiting(ratelimitData) | ||
| if err != nil { | ||
| return nil, nil, err | ||
| } | ||
|
|
||
| pageToken, err := bag.NextToken(nextCursor) | ||
| if err != nil { | ||
| return nil, nil, err | ||
| annos, err := pkg.AnnotationsForError(err) | ||
| return nil, &resources.SyncOpResults{Annotations: annos}, err | ||
| } |
There was a problem hiding this comment.
The Grants function doesn't implement pagination support. The GetUsersContext method may return a paginated list of users, but the current implementation doesn't handle the attrs.PageToken parameter or set a NextPageToken in the response. This could lead to incomplete grant data for workspaces with many users.
Consider adding pagination support similar to how it's done in the List function, which uses ParsePageToken and bag.NextToken() to handle pagination properly.
| options := slack.GetUsersOptionTeamID(parentResourceID.Resource) | ||
| users, err := o.client.GetUsersContext(ctx, options) | ||
| if err != nil { | ||
| annos, err := pkg.AnnotationsForError(err) | ||
| return nil, &resource.SyncOpResults{Annotations: annos}, err | ||
| } |
There was a problem hiding this comment.
The List function doesn't implement pagination support. The GetUsersContext method may return a paginated list of users, but the current implementation doesn't handle the attrs.PageToken parameter or set a NextPageToken in the response. This could lead to incomplete user data for workspaces with many users.
Consider adding pagination support using ParsePageToken and bag.NextToken() similar to the workspace List function.
| package config | ||
|
|
||
| import "reflect" | ||
| import "reflect" |
There was a problem hiding this comment.
Trailing whitespace after the import statement. This should be removed for consistency with Go formatting standards.
| import "reflect" | |
| import "reflect" |
| func (c *Slack) Metadata(ctx context.Context) (*v2.ConnectorMetadata, error) { | ||
| return &v2.ConnectorMetadata{ | ||
| DisplayName: "Slack", | ||
| Description: "Connector syncing users, workspaces, user groups and workspace roles from Slack to Baton.", |
There was a problem hiding this comment.
The description mentions "user groups" but the PR removes user group support. The description should be updated to reflect that only users, workspaces, and workspace roles are now being synced.
Suggested change: "Connector syncing users, workspaces and workspace roles from Slack to Baton."
| Description: "Connector syncing users, workspaces, user groups and workspace roles from Slack to Baton.", | |
| Description: "Connector syncing users, workspaces and workspace roles from Slack to Baton.", |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (2)
pkg/connector/user.go (1)
33-46: Duplicateuser_idkey assignment.The
profilemap setsuser_idtwice: first at line 33 withuser.ID, then at line 46 withfmt.Sprint(user.ID). Sinceuser.IDis already a string, the second assignment is redundant and overwrites the first.Remove the duplicate assignment:
profile["is_deleted"] = user.Deleted - profile["user_id"] = fmt.Sprint(user.ID)pkg/connector/workspace.go (1)
121-131: GetUsersContext call lacks pagination support, risking memory exhaustion and API timeouts for large workspaces.The
GetUsersContextmethod is called without pagination parameters. Slack'susers.listAPI requires cursor-based pagination and defaults to attempting to return the entire user set when no limit is specified—which can cause errors for large workspaces. The codebase already demonstrates the pagination pattern in theListmethod (line 69-75) usingListTeamsParameters{Cursor: bag.PageToken()}. TheGrantsmethod and similar call inuser.goshould implement equivalent pagination to safely handle large result sets.
🧹 Nitpick comments (1)
pkg/connector/roles.go (1)
42-56: Struct and builder simplified, butclientfield appears unused.The
workspaceRoleTypestruct stores aclientfield, but none of the methods (List,Entitlements,Grants) use it. TheListmethod usespkg.MakeResourceListwith static role data from therolesmap.Consider removing the unused
clientfield if it's not needed for future functionality:type workspaceRoleType struct { resourceType *v2.ResourceType - client *slack.Client } -func workspaceRoleBuilder(client *slack.Client) *workspaceRoleType { +func workspaceRoleBuilder() *workspaceRoleType { return &workspaceRoleType{ resourceType: resourceTypeWorkspaceRole, - client: client, } }This would also require updating
connector.goline 106 to callworkspaceRoleBuilder()without arguments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
baton_capabilities.jsonis excluded by none and included by none
📒 Files selected for processing (20)
pkg/config/conf.gen.go(1 hunks)pkg/config/config.go(0 hunks)pkg/connector/actions.go(0 hunks)pkg/connector/client/helpers.go(0 hunks)pkg/connector/client/mock.go(0 hunks)pkg/connector/client/models.go(0 hunks)pkg/connector/client/path.go(0 hunks)pkg/connector/client/request.go(0 hunks)pkg/connector/client/slack.go(0 hunks)pkg/connector/client/slack_interface.go(0 hunks)pkg/connector/connector.go(4 hunks)pkg/connector/enterprise_roles.go(0 hunks)pkg/connector/group.go(0 hunks)pkg/connector/resource_types.go(0 hunks)pkg/connector/roles.go(2 hunks)pkg/connector/user.go(3 hunks)pkg/connector/user_group.go(0 hunks)pkg/connector/workspace.go(4 hunks)pkg/connector/workspace_test.go(0 hunks)pkg/helpers.go(0 hunks)
💤 Files with no reviewable changes (15)
- pkg/helpers.go
- pkg/connector/actions.go
- pkg/connector/client/helpers.go
- pkg/connector/client/mock.go
- pkg/connector/client/request.go
- pkg/config/config.go
- pkg/connector/client/slack_interface.go
- pkg/connector/enterprise_roles.go
- pkg/connector/resource_types.go
- pkg/connector/group.go
- pkg/connector/workspace_test.go
- pkg/connector/user_group.go
- pkg/connector/client/path.go
- pkg/connector/client/slack.go
- pkg/connector/client/models.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-28T17:48:53.644Z
Learnt from: mateoHernandez123
Repo: ConductorOne/baton-slack PR: 57
File: pkg/connector/roles.go:217-219
Timestamp: 2025-10-28T17:48:53.644Z
Learning: In the baton-slack connector, when client methods like SetWorkspaceRole(), RemoveUser(), or other enterprise/slack client calls already wrap errors with specific gRPC codes via handleError(), use fmt.Errorf with %w to preserve those codes while adding context. Only use uhttp.WrapErrors for errors generated directly in the function (e.g., validation, precondition checks). Double-wrapping with uhttp.WrapErrors would overwrite the specific codes (Unavailable, PermissionDenied, etc.) needed for proper retry logic and alerting.
<!-- [/add_learning]
Applied to files:
pkg/connector/user.gopkg/connector/roles.gopkg/connector/connector.gopkg/connector/workspace.go
🧬 Code graph analysis (3)
pkg/connector/user.go (1)
pkg/helpers.go (1)
MakeResourceList(37-59)
pkg/connector/connector.go (2)
pkg/config/conf.gen.go (1)
Slack(6-8)pkg/helpers.go (1)
WrapSlackClientError(173-222)
pkg/connector/workspace.go (1)
pkg/helpers.go (1)
AnnotationsForError(227-242)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: CodeQL analysis (go)
- GitHub Check: go-lint
🔇 Additional comments (13)
pkg/config/conf.gen.go (1)
1-8: Configuration struct simplified correctly.The
Slackstruct now only contains theTokenfield, which aligns with the PR objective to remove enterprise dependencies. Since this is a generated file, ensure the generator input/schema was updated accordingly rather than editing this file directly.pkg/connector/user.go (3)
13-16: Struct simplification looks good.The
userResourceTypestruct has been appropriately simplified by removing enterprise-related fields.
129-147: Resource list construction is correct.The
MakeResourceListusage matches the helper signature frompkg/helpers.goand correctly transformsslack.Userobjects to resources via the inline callback.
150-155: Builder simplification is consistent.The
userBuildernow only accepts a*slack.Client, matching the simplified struct and aligning with the removal of enterprise dependencies.pkg/connector/connector.go (3)
66-90: Initialization flow correctly simplified.The
NewSlackfunction now only takesapiKey, validates withAuthTestContext, and returns a simplifiedSlackstruct. Error handling appropriately usespkg.WrapSlackClientError.
92-100:Newfunction correctly wires configuration.The function properly extracts
config.Tokenand passes it toNewSlack, matching the simplified configuration struct.
102-108: ResourceSyncers correctly updated.All three builders (
userBuilder,workspaceBuilder,workspaceRoleBuilder) now receive onlys.client, consistent with the simplified builder signatures across the codebase.pkg/connector/roles.go (2)
108-138: Entitlement description strings simplified.The entitlement descriptions now use static text ("the Slack workspace") instead of interpolating workspace-specific names. This is a reasonable simplification given the enterprise removal.
140-155: Grants method correctly returns empty with explanatory comment.The comment clearly explains that role grants are emitted during user grant calculation for efficiency. This pattern is consistent with the Slack API's structure.
pkg/connector/workspace.go (4)
17-31: Workspace type and builder correctly simplified.The
workspaceResourceTypestruct andworkspaceBuilderfunction have been appropriately simplified to remove enterprise-related fields.
69-78: List pagination correctly implemented.The
Listmethod properly usesListTeamsContextwith cursor-based pagination, derivingNextPageTokenfrom the Slack API response.
191-197: Member role assignment logic is correct but complex.The condition correctly identifies regular members by excluding restricted users, ultra-restricted users, invited users, bots, and deleted users. Consider extracting this to a helper function for readability if this pattern is used elsewhere.
207-210: Workspace member grant correctly appended.Each non-stranger user receives a workspace member grant in addition to any role-specific grants.
| type Slack struct { | ||
| client *slack.Client | ||
| apiKey string | ||
| enterpriseClient *enterprise.Client | ||
| enterpriseID string | ||
| ssoEnabled bool | ||
| govEnv bool | ||
| client *slack.Client | ||
| apiKey string | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if apiKey field is used anywhere in the connector package
rg -n '\bapiKey\b' --type=go pkg/connector/Repository: ConductorOne/baton-slack
Length of output: 326
🏁 Script executed:
#!/bin/bash
# Check if apiKey field is accessed from outside connector package
rg -n '\.apiKey\b' --type=goRepository: ConductorOne/baton-slack
Length of output: 50
Remove the unused apiKey field from the Slack struct.
The apiKey field is assigned during initialization but never accessed after the slack.Client is created. Since all operations use s.client, this field is dead code and should be removed:
type Slack struct {
client *slack.Client
apiKey string // ← Remove this line
}Update NewSlack to not assign it:
return &Slack{
client: client,
// apiKey: apiKey, ← Remove this assignment
}, nil🤖 Prompt for AI Agents
In pkg/connector/connector.go around lines 20 to 23, the Slack struct contains
an unused apiKey field that is assigned in NewSlack but never read; remove the
apiKey field declaration from the struct and also remove any assignment of
apiKey in NewSlack so the struct only holds the slack.Client pointer and
initialization returns &Slack{client: client}, nil.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 22 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| package config | ||
|
|
||
| import "reflect" | ||
| import "reflect" |
There was a problem hiding this comment.
Removed trailing whitespace after 'reflect' import statement.
| import "reflect" | |
| import "reflect" |
btipling
left a comment
btipling
left a comment
There was a problem hiding this comment.
I would like to wait on this PR until we have safely deployed it as a containerized connector and removed it from C1 as merging this work would prevent us from updating the connector in C1 in case we have issues with the lambda
2 participants
this connector will just use the regular API, no enterprise client, credentials, much simpler.
also because there's no enterprise support, no grant or revoke are supported
resource types removed (now only available at https://github.com/ConductorOne/baton-slack-enterprise):
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.