feat: add excluded_members support to team settings#3095
feat: add excluded_members support to team settings#3095janeklb wants to merge 9 commits intointegrations:mainfrom
Conversation
|
👋 Hi! Thank you for this contribution! Just to let you know, our GitHub SDK team does a round of issue and PR reviews twice a week, every Monday and Friday! We have a process in place for prioritizing and responding to your input. Because you are a part of this community please feel free to comment, add to, or pick up any issues/PRs that are labeled with |
- Add excluded_team_member_node_ids field to review_request_delegation - Allow teams to exclude specific members from PR review assignments - Update schema with proper validation and documentation - Add comprehensive test coverage for the new functionality - Update documentation with usage examples Resolves integrations#1972
748f47d to
c7bc8af
Compare
- Change excluded_team_member_node_ids to excluded_team_members - Accept GitHub usernames instead of GraphQL node IDs - Add getUserNodeId helper function to lookup node IDs automatically - Update tests to use friendly usernames like 'octocat', 'defunkt' - Update documentation with simpler examples - Makes configuration much more user-friendly and intuitive
3ee2bc2 to
638bd45
Compare
- Change ExcludedTeamMemberIds field type from []string to []githubv4.ID - Convert string node IDs to githubv4.ID type when building exclusion list - Ensures compliance with GitHub GraphQL API specification for UpdateTeamReviewAssignmentInput
a4029ae to
9750a01
Compare
stevehipwell
left a comment
There was a problem hiding this comment.
@janeklb based on your comment am I correct in thinking that Copilot created this whole PR for you? Are you a subject matter expert in Terraform or the GitHub API?
| for _, v := range excludedMembers.(*schema.Set).List() { | ||
| if v != nil { | ||
| username := v.(string) | ||
| nodeId, err := getUserNodeId(ctx, meta, username) |
There was a problem hiding this comment.
This is an expensive operation that you're calling repeatedly, have you looked at making this require fewer API calls?
There was a problem hiding this comment.
I haven't looked into it but it did cross my mind to see if there's a "batch" endpoint that accepts a list of usernames instead of going one by one. I will check it out
Correct -- I am not an sme in these areas. I've been wanting this feature so I tried the "code with copilot" button and iterated on the solution a little bit. |
- Replace single-user getUserNodeId with batch getBatchUserNodeIds function - Use GraphQL field aliases to fetch multiple user node IDs in one request - Reduce API calls from N individual requests to 1 batch request - Follow existing pattern from data_source_github_users.go - Improve performance and API rate limit usage for teams with multiple excluded members - Add proper error wrapping with %w format verb
0b4687e to
c5627f8
Compare
78dd9d5 to
9832b12
Compare
- Add workaround in resourceGithubTeamSettingsRead to preserve excluded_members from current state - GitHub's GraphQL API currently doesn't support reading back excluded team member IDs - This prevents Terraform from showing unwanted changes during refresh operations - Fixes test failure where excluded_members appeared as additions in plan after apply
b997af0 to
67c0712
Compare
|
@janeklb I don't think we can take this PR while there isn't a way to query the current state, that fundamentally breaks how Terraform works. |
Totally fair |
Implements support for excluding specific team members from PR review assignments.
Resolves #1972
Changes:
excluded_membersfield toreview_request_delegationUsage: