annotate permissions/v2 default builder#75
Conversation
📝 WalkthroughWalkthroughThis pull request integrates capability-based permissions into the Slack connector. The main connector initialization is updated to use a default capabilities builder, and resource type definitions are enhanced with fine-grained capability permissions annotations covering users, workspaces, groups, and roles. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
|
Claude finished @laurenleach's task in 1m 21s —— View job PR Review: Annotate permissions/v2 default builder
SummaryThis PR does two things:
ReviewLooks good overall. The changes are clean, well-organized, and the permission annotations per resource type are logically sound. A few observations:
The capabilities builder uses a zero-value Helper function (
Permission scope accuracy — Spot-checked the scopes against the resource type usage:
The bot vs. user token scope categorization via comments is a nice touch for maintainability. No issues found. LGTM. |
![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
🧹 Nitpick comments (1)
pkg/connector/resource_types.go (1)
8-12: Preallocate permission slice capacity in helperAt Line 9, preallocating
cp.Permissionstolen(perms)avoids incremental reallocations in the append loop.Suggested diff
func capabilityPermissions(perms ...string) *v2.CapabilityPermissions { - cp := &v2.CapabilityPermissions{} + cp := &v2.CapabilityPermissions{ + Permissions: make([]*v2.CapabilityPermission, 0, len(perms)), + } for _, p := range perms { cp.Permissions = append(cp.Permissions, &v2.CapabilityPermission{Permission: p}) } return cp }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/connector/resource_types.go` around lines 8 - 12, The helper capabilityPermissions preallocates cp.Permissions to avoid incremental reallocations: inside capabilityPermissions, initialize cp.Permissions with make([]*v2.CapabilityPermission, 0, len(perms)) (or make with length len(perms) and assign by index) before the loop, then populate it with the CapabilityPermission values for each p; this ensures capacity is reserved for len(perms) and improves performance when appending in the loop.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/baton-slack/main.go`:
- Around line 17-21: Add CLI flags --base-url (string) and --insecure (bool) in
main(), parse them before calling config.RunConnector, and propagate their
values into the connector configuration used by RunConnector. Concretely:
declare flag variables (e.g., baseURL, insecure), call flag.Parse(), then set
the appropriate fields on cfg.Configuration (or call a setter on
cfg.Configuration) so the value of baseURL is used as the connector API base URL
and insecure toggles TLS verification (e.g., disable cert verification in the
connector's HTTP/TLS setup). Keep the rest of the invocation of
config.RunConnector(ctx, connectorName, version, cfg.Configuration,
connector.New,
connectorrunner.WithDefaultCapabilitiesConnectorBuilderV2(&connector.Slack{}),
connectorrunner.WithSessionStoreEnabled()) unchanged except that
cfg.Configuration now contains the parsed baseURL and insecure values.
---
Nitpick comments:
In `@pkg/connector/resource_types.go`:
- Around line 8-12: The helper capabilityPermissions preallocates cp.Permissions
to avoid incremental reallocations: inside capabilityPermissions, initialize
cp.Permissions with make([]*v2.CapabilityPermission, 0, len(perms)) (or make
with length len(perms) and assign by index) before the loop, then populate it
with the CapabilityPermission values for each p; this ensures capacity is
reserved for len(perms) and improves performance when appending in the loop.
| func main() { | ||
| ctx := context.Background() | ||
| config.RunConnector(ctx, connectorName, version, cfg.Configuration, connector.New, | ||
| connectorrunner.WithDefaultCapabilitiesConnectorBuilderV2(&connector.Slack{}), | ||
| connectorrunner.WithSessionStoreEnabled()) |
There was a problem hiding this comment.
Add required --base-url and --insecure CLI support
Line 19 currently runs with static connector configuration and does not expose --base-url / --insecure handling required for mock-server and self-signed cert workflows.
As per coding guidelines, "Use command-line flags for API URLs and configuration rather than hardcoding them to allow mock server testing and environment flexibility. Support --base-url and --insecure command-line flags for mock server testing and self-signed certificate handling."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/baton-slack/main.go` around lines 17 - 21, Add CLI flags --base-url
(string) and --insecure (bool) in main(), parse them before calling
config.RunConnector, and propagate their values into the connector configuration
used by RunConnector. Concretely: declare flag variables (e.g., baseURL,
insecure), call flag.Parse(), then set the appropriate fields on
cfg.Configuration (or call a setter on cfg.Configuration) so the value of
baseURL is used as the connector API base URL and insecure toggles TLS
verification (e.g., disable cert verification in the connector's HTTP/TLS
setup). Keep the rest of the invocation of config.RunConnector(ctx,
connectorName, version, cfg.Configuration, connector.New,
connectorrunner.WithDefaultCapabilitiesConnectorBuilderV2(&connector.Slack{}),
connectorrunner.WithSessionStoreEnabled()) unchanged except that
cfg.Configuration now contains the parsed baseURL and insecure values.
2 participants
Summary by CodeRabbit
Release Notes
New Features
Refactor