Skip to content
This repository was archived by the owner on Apr 15, 2026. It is now read-only.

Commit 7291f05

Browse files
Copilotvmvarela
Copilot
and
vmvarela
committed
Add comprehensive PR integrations#3008 unresolved comments status report
Co-authored-by: vmvarela <[email protected]>
1 parent d262156 commit 7291f05

1 file changed

Lines changed: 232 additions & 0 deletions

File tree

‎PR_3008_UNRESOLVED_COMMENTS_STATUS.md‎

Lines changed: 232 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,232 @@
1+
# PR #3008 Unresolved Comments Status Report
2+
3+
**Pull Request**: [#3008 - Add support for Enterprise Teams](https://github.com/integrations/terraform-provider-github/pull/3008)
4+
**Repository**: integrations/terraform-provider-github
5+
**Status**: Open
6+
**Total Review Comments**: 45
7+
**Generated**: 2026-01-16
8+
9+
## Summary
10+
11+
This report provides a comprehensive status of all review comments on PR #3008, focusing specifically on the **3 unresolved threads** that require attention before the PR can be merged.
12+
13+
## PR Overview
14+
15+
- **Title**: [FEAT] Add support for Enterprise Teams
16+
- **Author**: @vmvarela
17+
- **Created**: 2025-12-17
18+
- **Last Updated**: 2026-01-10
19+
- **Commits**: 24
20+
- **Files Changed**: 19
21+
- **Additions**: 1,870 lines
22+
- **Review Comments**: 45 total (42 resolved, 3 unresolved)
23+
24+
## Unresolved Comments (3)
25+
26+
### 1. Data Source Description Inconsistency
27+
**Thread ID**: PRRT_kwDOBZHf085oO5Gs
28+
**File**: `github/data_source_github_enterprise_team_membership.go`
29+
**Status**: ⚠️ **UNRESOLVED**
30+
**Reviewer**: @copilot-pull-request-reviewer
31+
**Date**: 2026-01-06
32+
33+
**Issue**: The Description field says "Manages membership in a GitHub enterprise team" which is incorrect for a data source. Data sources read/query existing data rather than manage it.
34+
35+
**Current Code**:
36+
```go
37+
Description: "Manages membership in a GitHub enterprise team.",
38+
```
39+
40+
**Suggested Fix**:
41+
```go
42+
Description: "Retrieves information about membership in a GitHub enterprise team.",
43+
```
44+
45+
**Impact**: Medium - This is a documentation/description issue that affects user understanding but not functionality.
46+
47+
**Recommendation**: Accept the suggestion and update the description to accurately reflect that this is a data source (read-only) not a resource (manages state).
48+
49+
---
50+
51+
### 2. Empty Description Field Handling
52+
**Thread ID**: PRRT_kwDOBZHf085oO5HT
53+
**File**: `github/resource_github_enterprise_team.go`
54+
**Status**: ⚠️ **UNRESOLVED**
55+
**Reviewer**: @copilot-pull-request-reviewer
56+
**Date**: 2026-01-06
57+
58+
**Issue**: The description field is unconditionally wrapped with `githubv3.String()` even when it's empty. This means an empty string will be sent in the API request.
59+
60+
**Current Code**:
61+
```go
62+
OrganizationSelectionType: githubv3.String(orgSelection),
63+
}
64+
// Description is always set, even if empty
65+
```
66+
67+
**Suggested Fix**:
68+
```go
69+
OrganizationSelectionType: githubv3.String(orgSelection),
70+
}
71+
if description != "" {
72+
req.Description = githubv3.String(description)
73+
}
74+
```
75+
76+
**Impact**: Low - API might receive unnecessary empty strings, but this is unlikely to cause functional issues.
77+
78+
**Recommendation**: Accept the suggestion to conditionally set the Description field only when non-empty, similar to how groupID is handled in the same function.
79+
80+
---
81+
82+
### 3. Read-after-Create/Update Pattern (Critical)
83+
**Thread ID**: PRRT_kwDOBZHf085oxj2-
84+
**File**: `github/resource_github_enterprise_team_membership.go`
85+
**Status**: ⚠️ **UNRESOLVED**
86+
**Reviewer**: @deiga
87+
**Date**: 2026-01-09
88+
89+
**Issue**: The code uses the deprecated "Read-after-Create/Update" pattern. The project no longer wants to use this pattern.
90+
91+
**Context**: This is related to comment #35 which states:
92+
> "We don't want to use the `Read` after `Create` or `Update` pattern anymore. If there are computed fields, you should set them in `Update` or `Create` directly"
93+
94+
**Current Pattern**:
95+
```go
96+
func resourceGithubEnterpriseTeamMembershipCreate(...) {
97+
// ... create logic ...
98+
return resourceGithubEnterpriseTeamMembershipRead(ctx, d, meta)
99+
}
100+
101+
func resourceGithubEnterpriseTeamMembershipUpdate(...) {
102+
// ... update logic ...
103+
return resourceGithubEnterpriseTeamMembershipRead(ctx, d, meta)
104+
}
105+
```
106+
107+
**Recommended Fix**:
108+
```go
109+
func resourceGithubEnterpriseTeamMembershipCreate(...) {
110+
// ... create logic ...
111+
// Set computed fields directly from API response
112+
return nil
113+
}
114+
115+
func resourceGithubEnterpriseTeamMembershipUpdate(...) {
116+
// ... update logic ...
117+
// Set computed fields directly from API response
118+
return nil
119+
}
120+
```
121+
122+
**Impact**: High - This is a code pattern/architectural issue that affects maintainability and aligns with project standards.
123+
124+
**Recommendation**:
125+
1. Remove the `return resourceGithubEnterpriseTeamMembershipRead(...)` calls from Create and Update functions
126+
2. Set any computed fields directly from the API response instead
127+
3. Apply this pattern to `resource_github_enterprise_team.go` and `resource_github_enterprise_team_organizations.go` as well (per related comments #33 and #34)
128+
129+
**Note**: The PR author has already addressed this pattern in PR #6 of their fork (vmvarela/terraform-provider-github), so they're aware of this requirement. The fix needs to be applied to the upstream PR #3008.
130+
131+
---
132+
133+
## Resolved Comments (42)
134+
135+
The following categories of comments have been successfully resolved:
136+
137+
### Schema and Validation (7 comments - All Resolved ✅)
138+
- Added `ValidateDiagFunc` for enterprise slug validation
139+
- Added `ValidateDiagFunc` for team_id validation
140+
- Implemented `ExactlyOneOf` for slug/team_id fields
141+
- Removed redundant `ConflictsWith` constraints
142+
- Removed unnecessary validation checks from CRUD functions
143+
144+
### Documentation Branding (6 comments - All Resolved ✅)
145+
- Fixed "Github" → "GitHub" capitalization in all page_title fields
146+
- Fixed grammatical error "Create and manages" → "Creates and manages"
147+
148+
### go-github SDK Usage (1 comment - Resolved ✅)
149+
- Acknowledged need to use go-github SDK instead of direct REST API calls
150+
- Waiting for SDK v81+ release with Enterprise Teams support
151+
152+
### Code Quality and Best Practices (28 comments - All Resolved ✅)
153+
- Removed meaningless `_ = resp` assignments
154+
- Removed redundant `testCase` wrapper pattern in tests
155+
- Added top-level `Description` fields to data sources
156+
- Used `testResourcePrefix` for consistent test resource naming
157+
- Used constants for field names in data sources
158+
- Moved utility functions to `util_enterprise_teams.go`
159+
- Separated `enterprise_team` into `team_slug` and `team_id` fields with `ExactlyOneOf`
160+
- Improved error handling and messages
161+
- Removed unused variables and dead code
162+
163+
---
164+
165+
## Critical Path to Merge
166+
167+
To get PR #3008 ready for merge, the following must be addressed **in priority order**:
168+
169+
### Priority 1: Architectural Pattern (MUST FIX)
170+
1. **Remove Read-after-Create/Update pattern** across all three resources:
171+
- `resource_github_enterprise_team.go`
172+
- `resource_github_enterprise_team_membership.go`
173+
- `resource_github_enterprise_team_organizations.go`
174+
175+
### Priority 2: Code Quality (SHOULD FIX)
176+
2. **Fix empty description handling** in `resource_github_enterprise_team.go`
177+
3. **Update data source description** in `data_source_github_enterprise_team_membership.go`
178+
179+
### Priority 3: SDK Dependency (BLOCKED - External)
180+
- **Wait for go-github v81+** release with Enterprise Teams API support
181+
- Once available, replace direct REST API calls with SDK methods
182+
183+
---
184+
185+
## Reviewer Activity Summary
186+
187+
| Reviewer | Comments | Status |
188+
|----------|----------|--------|
189+
| @deiga | 28 | 25 resolved, 3 unresolved |
190+
| @copilot-pull-request-reviewer | 17 | 15 resolved, 2 unresolved |
191+
192+
**Note**: @deiga is the primary human reviewer and maintainer. Their unresolved comments should be prioritized.
193+
194+
---
195+
196+
## Recommendations for PR Author
197+
198+
1. **Immediate Action Required**:
199+
- Address the 3 unresolved comments, particularly the Read-after-Create/Update pattern issue
200+
- Since you've already fixed this pattern in your fork's PR #6, you can apply the same changes to the upstream PR
201+
202+
2. **Reference Implementation**:
203+
- Your fork's PR #6 ([vmvarela/terraform-provider-github#6](https://github.com/vmvarela/terraform-provider-github/pull/6)) shows the correct pattern
204+
- The commit "feat(enterprise_team): address PR review feedback" demonstrates the required changes
205+
206+
3. **Next Steps**:
207+
- Update the PR with fixes for the 3 unresolved comments
208+
- Wait for go-github SDK v81+ release (external dependency)
209+
- Request re-review from @deiga once changes are complete
210+
211+
---
212+
213+
## Additional Context
214+
215+
### Related PRs
216+
- **Fork PR #6**: Contains fixes for review feedback from upstream PR #3008
217+
- Already merged into fork's enterprise-teams branch
218+
- Shows the correct implementation pattern requested by reviewers
219+
220+
### Testing Status
221+
- All 7 acceptance tests passing in fork
222+
- Tests include: enterprise team CRUD, membership, organizations, and data sources
223+
224+
---
225+
226+
## Conclusion
227+
228+
**Overall Status**: PR is nearly ready for merge, pending resolution of 3 minor unresolved comments and one external dependency (go-github SDK update).
229+
230+
**Time Estimate**: The 3 unresolved comments can be addressed in 1-2 hours of focused work, based on the complexity and existing reference implementation in the fork.
231+
232+
**Blocker**: The go-github SDK dependency (waiting for v81+ release) is the only hard blocker, but this doesn't prevent addressing the code review comments now.

0 commit comments

Comments
 (0)