Skip to content

Refactor to use StaticEntitlements for standard resource types#145

Merged
btipling merged 2 commits intomainfrom
bt/refactor-static-entitlements
Apr 15, 2026
Merged

Refactor to use StaticEntitlements for standard resource types#145
btipling merged 2 commits intomainfrom
bt/refactor-static-entitlements

Conversation

@btipling
Copy link
Copy Markdown
Contributor

This PR refactors the Entitlements implementation for Org, Team, Repository, OrgRole, and EnterpriseRole to use StaticEntitlements.

By moving these to static templates, we reduce the number of tasks generated during a sync, as the SDK can now define these entitlements once per resource type instead of per resource instance. This is especially impactful for connectors with a large number of repositories or teams.

This change refactors the Entitlements implementation for Org, Team,
Repository, OrgRole, and EnterpriseRole to use StaticEntitlements.

By moving these to static templates, we reduce the number of tasks
generated during a sync, as the SDK can now define these entitlements
once per resource type instead of per resource instance. This is
especially impactful for connectors with a large number of repositories
or teams.

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@btipling btipling requested a review from a team April 15, 2026 17:53
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 15, 2026

Claude finished @btipling's task in 2m 40s —— View job


PR Review: Refactor to use StaticEntitlements

  • Gather context and read changed files
  • Review changes for correctness and best practices
  • Post review feedback

Overall: This looks good. The refactor is clean, consistent across all five files, and correctly implements the StaticEntitlementSyncerV2 interface from the SDK.

What the PR does

Each of the five resource types (Org, Team, Repository, OrgRole, EnterpriseRole) is refactored in the same way:

  1. Entitlements() now returns nil, nil, nil (no per-resource entitlements)
  2. A new StaticEntitlements() method returns template entitlements with nil resource
  3. Resource-specific display names/descriptions are replaced with generic ones
  4. V1Identifier annotations are removed from entitlements (they remain on resources and grants where they're still appropriate)

This is the correct pattern — the SDK type-checks for StaticEntitlementSyncerV2 and calls StaticEntitlements() once per resource type instead of calling Entitlements() per resource instance.

Correctness

  • nil resource is safe: Proto getters are nil-safe in Go, so NewEntitlementID(nil, name) produces "::name" as the template ID. The SDK handles stamping actual resource identity when applying these templates.
  • Grants are unchanged: All Grants() methods still correctly reference resource and produce proper grant IDs — these are unaffected by the refactor.
  • Provisioning (Grant/Revoke) is unchanged: The Grant() and Revoke() methods parse entitlement IDs using strings.Split(en.Id, ":") expecting 3 parts. With static entitlement IDs formatted as "::permission", the split still produces 3 parts (["", "", "permission"]), so this continues to work correctly.
  • No unused imports introduced: fmt is still used in all files for other purposes.

Nits (non-blocking)

  1. Minor style inconsistency in enterprise_role.go:137: uses rv := []*v2.Entitlement{} while the other four files use rv := make([]*v2.Entitlement, 0, N). Not a functional issue, just a style difference.

  2. Capacity hint in enterprise_role.go: Since there's exactly 1 entitlement, make([]*v2.Entitlement, 0, 1) would be more consistent with the other files and avoid a minor allocation.

Summary

Clean refactor, consistent pattern across all resource types, no correctness issues. The performance win from reducing per-resource entitlement tasks (especially for connectors with many repos/teams) is a clear benefit. LGTM.

Copy link
Copy Markdown

@luisina-santos luisina-santos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without SkipEntitlements annotation I think baton-sdk still goes for each resources trying to sync Entitlements also with StaticEntitlements implemented

…ments

Prevents the syncer from calling the per-resource Entitlements() method
(which now returns nil) for types that define their entitlements via
StaticEntitlements instead.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 15, 2026

Claude finished @btipling's task in 1m 57s —— View job


PR Review: Refactor to use StaticEntitlements

  • Gather context and read changed files
  • Review changes for correctness and best practices
  • Post review feedback

Overall: This looks good. The refactor is clean and consistent, and the follow-up commit correctly addresses luisina-santos' feedback about needing SkipEntitlements.

What the PR does (2 commits)

Commit 1 (56e5da2): For all five resource types (Org, Team, Repository, OrgRole, EnterpriseRole):

  1. Entitlements() now returns nil, nil, nil (no-op)
  2. New StaticEntitlements() method defines template entitlements with nil resource
  3. Display names/descriptions become generic (no per-resource names)
  4. V1Identifier annotations removed from entitlements (they remain on resources and grants)

Commit 2 (ca683fd): Adds SkipEntitlements annotation to these resource types via a new skipEntitlementsAnnotations() helper, so the SDK doesn't still call Entitlements() per resource.

Provisioning compatibility (Grant/Revoke)

I verified all four Grant methods that parse entitlement IDs:

  • repository.go:376 and team.go:311: strings.Split(en.Id, ":") expecting 3 parts, extracting enIDParts[2] as the permission level. The SDK stamps the actual resource onto the template, so the entitlement ID at provisioning time is still "resourceType:resourceId:permission" — 3 parts, works correctly.
  • org_role.go:345: Same split pattern — works correctly.
  • org.go:286-287 / org.go:374-375: Constructs expected IDs via entitlement.NewEntitlementID(en.Resource, ...) and compares against en.Id. Since en.Resource is the stamped actual resource at provisioning time, these comparisons still match. Works correctly.

All Grant/Revoke methods derive resource context from en.Resource / entitlement.Resource, which the SDK populates from the actual resource instance, not the template. No issues here.

Nits (non-blocking)

  1. Style inconsistency in enterprise_role.go:137: Uses rv := []*v2.Entitlement{} while the other four files use rv := make([]*v2.Entitlement, 0, N). Since there's exactly 1 entitlement, make([]*v2.Entitlement, 0, 1) would be more consistent. Fix this →

Summary

Clean refactor with correct SDK interface implementation. The SkipEntitlements addition in the second commit properly ensures the SDK won't redundantly call Entitlements() per resource. Provisioning paths are unaffected. LGTM.

@btipling btipling merged commit 4b92231 into main Apr 15, 2026
7 of 8 checks passed
@btipling btipling deleted the bt/refactor-static-entitlements branch April 15, 2026 18:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants