Only emit user grants (where we emit the role grants) if role resource type is enabled #55
Conversation
…rce type is enabled
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| func userBuilder(client *linear.Client, skipRoleGrants bool) *userResourceType { | ||
| if skipRoleGrants { | ||
| resourceTypeUser.Annotations = annotations.New(&v2.SkipEntitlementsAndGrants{}) | ||
| } else { | ||
| resourceTypeUser.Annotations = annotations.New(&v2.SkipEntitlements{}) | ||
| } | ||
| return &userResourceType{ |
There was a problem hiding this comment.
🟡 Suggestion: userBuilder mutates the package-level global resourceTypeUser.Annotations as a side effect. This is functionally correct for the single-connector-per-process usage here, but it makes the resource type's annotations depend on construction order and would be surprising if the connector were ever instantiated twice in-process with different skipRoleGrants values (last write wins). Consider assigning the annotations to a per-builder copy of the resource type instead of the shared global. (confidence: medium)
There was a problem hiding this comment.
This is a big deal if it runs ever in C1, but in a lambda it is specific to the connector id. I might copy the resource type just because it feels less problematic instead of changing the global package variable imo, even thought it doesn't really matter.
Connector PR Review: Only emit user grants (where we emit the role grants) if role resource type is enabledBlocking Issues: 0 | Suggestions: 0 | Threads Resolved: 0 Review SummaryThe new commit only regenerated metadata: Security IssuesNone found. Correctness IssuesNone found. SuggestionsNone. |
btipling
left a comment
btipling
left a comment
There was a problem hiding this comment.
When connectors ran in C1 modifying the global resource variable caused an incident so I have a PTSD comment, but it doesn't actually impact running in a lambda
| func userBuilder(client *linear.Client, skipRoleGrants bool) *userResourceType { | ||
| if skipRoleGrants { | ||
| resourceTypeUser.Annotations = annotations.New(&v2.SkipEntitlementsAndGrants{}) | ||
| } else { | ||
| resourceTypeUser.Annotations = annotations.New(&v2.SkipEntitlements{}) | ||
| } | ||
| return &userResourceType{ |
There was a problem hiding this comment.
This is a big deal if it runs ever in C1, but in a lambda it is specific to the connector id. I might copy the resource type just because it feels less problematic instead of changing the global package variable imo, even thought it doesn't really matter.
| t.Fatalf("failed to create client: %v", err) | ||
| } | ||
| return userBuilder(client) | ||
| return userBuilder(client, false) |
There was a problem hiding this comment.
🟡 Suggestion: The core behavior of this PR — userBuilder emitting SkipEntitlements vs SkipEntitlementsAndGrants based on skipRoleGrants — has no direct test. Consider a small table-driven test asserting ResourceType().GetAnnotations() contains the expected annotation for each value of skipRoleGrants, so a future refactor doesn't silently drop the grant-skipping behavior. (confidence: medium)
Connector PR Review: Only emit user grants (where we emit the role grants) if role resource type is enabledBlocking Issues: 0 | Suggestions: 1 | Threads Resolved: 0 Review SummaryThe full PR diff was scanned for security and correctness. This change wires the SDK's Security IssuesNone found. Correctness IssuesNone found. Suggestions
Prompt for AI agents |
3 participants
Only emit user grants (where we emit the role grants) if role resource type is enabled