Cleanup and fixing comments #35

johnallers · 2025-12-19T12:43:00Z

Summary by CodeRabbit

New Features
- Sync privileges as first-class resources with privilege-to-collaborator entitlements.
- Role listing now includes base and custom roles.
- Projects are discoverable and synchronized as resources.
Improvements
- Folder sync now enumerates folders and project folders, with updated entitlements and grants for collaborator assignments.
- Collaborator and privilege sync flows updated to align with framework expectations.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

coderabbitai · 2025-12-19T12:43:12Z

Walkthrough

Added compile-time ResourceSyncerV2 assertions to multiple builders, removed the env parameter from newPrivilegeBuilder and updated its call sites, and changed listing/entitlement/grant behaviors in privilege, project, role, folder, and collaborator syncers to reflect their resource semantics.

Changes

Cohort / File(s)	Summary
Interface conformance assertions `pkg/connector/collaborator.go`, `pkg/connector/folder.go`, `pkg/connector/privilege.go`, `pkg/connector/project.go`	Added `var _ connectorbuilder.ResourceSyncerV2 = (*<builder>)(nil)` compile-time checks and imported `connectorbuilder`.
Constructor & call-site update `pkg/connector/connector.go`, `pkg/connector/privilege.go`	Removed `env` parameter from `newPrivilegeBuilder` (`newPrivilegeBuilder(client *client.WorkatoClient)`); updated caller in `connector.go` to use the new signature.
Privilege syncer behavior `pkg/connector/privilege.go`	`List` now enumerates privileges (e.g., `workato.AllCompoundPrivileges`), `Entitlements` now describes privilege-to-collaborator assignment, `Grants` signature simplified to ignore params; resource construction via `privilegeResource`.
Project syncer changes `pkg/connector/project.go`	`List` implemented to fetch projects via `GetProjects` and map to `projectResource`; `Entitlements`/`Grants` remain non-assignable/non-grantable (returns nil).
Role syncer changes `pkg/connector/role.go`	`List` now returns base and custom roles (API + base roles); `Entitlements` returns a role-to-collaborator entitlement; `Grants` generate role-related privilege grants with annotations.
Folder builder updates `pkg/connector/folder.go`	Added private field `disableCustomRolesSync`; `List` reworked to return folders/projects by parent type with pagination; `Entitlements` now returns folder-collaborator entitlement; `Grants` list roles for folders (supports pagination and optional role retrieval).
Collaborator minor change `pkg/connector/collaborator.go`	Added `connectorbuilder` import and compile-time assertion for `collaboratorBuilder` implementing `ResourceSyncerV2`.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Pay attention to: privilege.go constructor and Grants signature changes and their callers.
Verify connector.go updated calls align with new newPrivilegeBuilder signature everywhere.
Confirm disableCustomRolesSync initialization/usage in folder.go.
Validate resource-to-entitlement/grant mappings in project.go, role.go, and folder.go.

Possibly related PRs

[BB-1788] Add containerization support implementing ResourceSyncerV2 and SessionStore cache #29 — Large migration to ResourceSyncerV2 and related constructor/signature updates overlapping these changes.
Session rework for containerization #34 — Overlaps on adding ResourceSyncerV2 assertions and removing env from builders.

Suggested reviewers

laurenleach
luisina-santos
JavierCarnelli-ConductorOne
mateovespConductor
agustin-conductor
sergiocorral-conductorone

Poem

🐰 I hopped through code with a little cheer,
Assertions snug and constructors clear,
Privileges counted, folders aligned,
Builders now typed — neat and refined! ✨

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Title check	⚠️ Warning	The title 'Cleanup and fixing comments' is vague and does not accurately represent the substantial code changes in this PR, which include interface assertions, constructor signature changes, method behavioral updates, and significant logic refactors across multiple files.	Update the title to reflect the main changes, such as 'Implement ResourceSyncerV2 interface and refactor resource builders' or 'Update privilege builder and implement interface assertions across resource builders'.
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch jallers/nits

📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 693005a and e6bb63f.

📒 Files selected for processing (2)

pkg/connector/folder.go (5 hunks)
pkg/connector/project.go (3 hunks)

🔇 Additional comments (5)

pkg/connector/project.go (2)

14-15: Good addition of compile-time interface conformance check.

This is a best practice that ensures projectBuilder implements ResourceSyncerV2 at compile time, catching any missing methods early.

49-57: Comments and implementations look good.

The previous typo ("an an") has been fixed, and the comments now accurately describe that projects are not assignable/grantable.

pkg/connector/folder.go (3)

25-26: Good addition of compile-time interface conformance check.

This ensures folderBuilder implements ResourceSyncerV2 at compile time.

37-95: List implementation looks correct.

The double period typo from the previous review has been fixed. The function correctly handles:

nil parent (early return)

Project parent type (fetches project folders)

Folder parent type (fetches subfolders)

Unknown parent types (logs warning and returns gracefully)

111-173: Grants implementation looks well-structured.

The pagination logic using the bag pattern is clean, and the conditional role sync based on disableCustomRolesSync is appropriate. The grant annotations with GrantExpandable are correctly set up for entitlement expansion. The getRoleByFolder function is properly defined in the same package and accessible.

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

pkg/connector/privilege.go (1)
52-59: Typo: stray backtick in display name format string.

Line 58 has a backtick (`) instead of what appears should be no special character or possibly a closing parenthesis.
🔎 Proposed fix
-		entitlement.WithDisplayName(fmt.Sprintf("%s have %s`", collaboratorResourceType.DisplayName, resource.DisplayName)),
+		entitlement.WithDisplayName(fmt.Sprintf("%s have %s", collaboratorResourceType.DisplayName, resource.DisplayName)),

🧹 Nitpick comments (1)

pkg/connector/privilege.go (1)

70-75: Remove unused cache field from privilegeBuilder.

The cache field is initialized in the constructor but never used in any of the privilegeBuilder methods (List, Entitlements, Grants). Other builders in the codebase that use cache (e.g., collaboratorBuilder, roleBuilder) actively reference their cache fields, making this instance clearly unused dead code.

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 37f501a and 693005a.

📒 Files selected for processing (6)

pkg/connector/collaborator.go (2 hunks)
pkg/connector/connector.go (1 hunks)
pkg/connector/folder.go (5 hunks)
pkg/connector/privilege.go (5 hunks)
pkg/connector/project.go (3 hunks)
pkg/connector/role.go (3 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

pkg/connector/privilege.go (1)

pkg/connector/client/client.go (1)

WorkatoClient (28-33)

🔇 Additional comments (9)

pkg/connector/collaborator.go (1)

8-8: LGTM!

The compile-time interface conformance assertion is a good practice. It ensures collaboratorBuilder implements ResourceSyncerV2 at compile time, catching any interface drift early.

Also applies to: 23-24

pkg/connector/connector.go (1)

31-31: LGTM!

The constructor call correctly aligns with the updated newPrivilegeBuilder signature that now only requires the client parameter.

pkg/connector/folder.go (2)

11-11: LGTM!

The compile-time interface conformance assertion ensures folderBuilder implements ResourceSyncerV2 at compile time.

Also applies to: 25-26

97-98: LGTM!

The updated comments now accurately describe folder-focused semantics for Entitlements and Grants methods.

Also applies to: 111-112

pkg/connector/role.go (1)

41-42: LGTM!

The updated comments accurately describe role-focused semantics for List, Entitlements, and Grants methods.

Also applies to: 88-89, 101-101

pkg/connector/project.go (2)

6-6: LGTM!

The compile-time interface conformance assertion ensures projectBuilder implements ResourceSyncerV2 at compile time.

Also applies to: 14-15

24-24: LGTM!

The updated comments accurately describe project-focused semantics.

Also applies to: 54-54

pkg/connector/privilege.go (2)

7-7: LGTM!

The compile-time interface conformance assertion ensures privilegeBuilder implements ResourceSyncerV2 at compile time.

Also applies to: 21-22

32-33: LGTM!

The updated comments accurately describe privilege-focused semantics.

Also applies to: 65-66

pkg/connector/folder.go

pkg/connector/project.go

coderabbitai

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

pkg/connector/folder.go (1)

101-101: Minor typo: assigmentOptions should be assignmentOptions.

Missing 'n' in "assignment".

🔎 Proposed fix

-	assigmentOptions := []entitlement.EntitlementOption{
+	assignmentOptions := []entitlement.EntitlementOption{
 		entitlement.WithGrantableTo(collaboratorResourceType),
 		entitlement.WithDescription(fmt.Sprintf("%s can access %s", collaboratorResourceType.DisplayName, resource.DisplayName)),
 		entitlement.WithDisplayName(fmt.Sprintf("%s access %s", collaboratorResourceType.DisplayName, resource.DisplayName)),
 	}
-	rv = append(rv, entitlement.NewPermissionEntitlement(resource, collaboratorAccessEntitlement, assigmentOptions...))
+	rv = append(rv, entitlement.NewPermissionEntitlement(resource, collaboratorAccessEntitlement, assignmentOptions...))

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 693005a and e6bb63f.

📒 Files selected for processing (2)

pkg/connector/folder.go (5 hunks)
pkg/connector/project.go (3 hunks)

🔇 Additional comments (5)

pkg/connector/project.go (2)

14-15: Good addition of compile-time interface conformance check.

This is a best practice that ensures projectBuilder implements ResourceSyncerV2 at compile time, catching any missing methods early.

49-57: Comments and implementations look good.

The previous typo ("an an") has been fixed, and the comments now accurately describe that projects are not assignable/grantable.

pkg/connector/folder.go (3)

25-26: Good addition of compile-time interface conformance check.

This ensures folderBuilder implements ResourceSyncerV2 at compile time.

37-95: List implementation looks correct.

The double period typo from the previous review has been fixed. The function correctly handles:

nil parent (early return)

Project parent type (fetches project folders)

Folder parent type (fetches subfolders)

Unknown parent types (logs warning and returns gracefully)

111-173: Grants implementation looks well-structured.

The pagination logic using the bag pattern is clean, and the conditional role sync based on disableCustomRolesSync is appropriate. The grant annotations with GrantExpandable are correctly set up for entitlement expansion. The getRoleByFolder function is properly defined in the same package and accessible.

JavierCarnelli-ConductorOne

Looks good! Just out of curiosity, why are you including var _ connectorbuilder.ResourceSyncerV2 = (*resourceBuilder)(nil) to the resources?
What use does it have?

johnallers · 2025-12-19T15:33:52Z

Looks good! Just out of curiosity, why are you including var _ connectorbuilder.ResourceSyncerV2 = (*resourceBuilder)(nil) to the resources? What use does it have?

https://github.com/uber-go/guide/blob/master/style.md#verify-interface-compliance

johnallers added 2 commits December 19, 2025 07:39

verify interfaces

82663d2

cleanup parameters and fix comments

693005a

johnallers requested a review from a team December 19, 2025 12:43

coderabbitai bot reviewed Dec 19, 2025

View reviewed changes

pkg/connector/folder.go Outdated Show resolved Hide resolved

pkg/connector/project.go Outdated Show resolved Hide resolved

fix punctuation and grammar

e6bb63f

coderabbitai bot reviewed Dec 19, 2025

View reviewed changes

JavierCarnelli-ConductorOne approved these changes Dec 19, 2025

View reviewed changes

agustin-conductor approved these changes Dec 22, 2025

View reviewed changes

btipling approved these changes Dec 22, 2025

View reviewed changes

johnallers merged commit ee8f2bf into main Dec 26, 2025
4 checks passed

coderabbitai bot mentioned this pull request Dec 29, 2025

[BB-1886] Add option to sync all environments #37

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Cleanup and fixing comments #35

Cleanup and fixing comments #35

Uh oh!

johnallers commented Dec 19, 2025 •

edited by coderabbitai bot

Loading

Uh oh!

coderabbitai bot commented Dec 19, 2025 •

edited

Loading

Uh oh!

coderabbitai bot left a comment

Uh oh!

Uh oh!

Uh oh!

coderabbitai bot left a comment

Uh oh!

JavierCarnelli-ConductorOne left a comment

Uh oh!

johnallers commented Dec 19, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Cleanup and fixing comments #35

Cleanup and fixing comments #35

Uh oh!

Conversation

johnallers commented Dec 19, 2025 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Dec 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Suggested reviewers

Poem

Pre-merge checks and finishing touches

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

JavierCarnelli-ConductorOne left a comment

Choose a reason for hiding this comment

Uh oh!

johnallers commented Dec 19, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

johnallers commented Dec 19, 2025 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Dec 19, 2025 •

edited

Loading