refactor(enterprise_team): address PR review feedback #6
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Apply code review feedback from @deiga on enterprise teams resources: - Remove Read-after-Create/Update pattern in all enterprise team resources, setting computed fields directly from API response instead - Remove unnecessary re-discovery of team slug in Update (always available from Read) - Remove meaningless `_ = resp` assignment in Delete - Eliminate redundant `testCase` wrapper pattern in acceptance tests - Separate `enterprise_team` field into `team_slug` and `team_id` with ExactlyOneOf validation for membership and organizations resources - Move shared helper functions to util_enterprise_teams.go: - findEnterpriseTeamBySlugOrID - listAllEnterpriseTeamOrganizations - Use "/" as ID separator instead of ":" because enterprise team slugs contain ":" (e.g., "ent:team-name") - Use state data in Delete instead of API call for organizations resource - Update documentation with new schema
There was a problem hiding this comment.
Pull request overview
This refactoring PR addresses code review feedback for enterprise team resources by removing the Read-after-Create/Update pattern, eliminating redundant code, improving schema validation with separate team_slug and team_id fields, and consolidating shared utilities.
Changes:
- Removed Read-after-Create/Update calls across all enterprise team resources, setting computed fields directly from API responses instead
- Split the
enterprise_teamfield into separateteam_slugandteam_idfields with ExactlyOneOf validation for membership and organizations resources - Moved shared helper functions to
util_enterprise_teams.goand changed ID separator from ":" to "/" to avoid conflicts with team slug format - Simplified test structure by removing redundant wrapper patterns and updated documentation to reflect new schema
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| github/util_enterprise_teams.go | Added ID building/parsing functions and moved shared helper functions (findEnterpriseTeamBySlugOrID, listAllEnterpriseTeamOrganizations) |
| github/resource_github_enterprise_team.go | Removed Read-after-Create/Update, removed re-discovery logic in Update, removed meaningless assignment in Delete |
| github/resource_github_enterprise_team_organizations.go | Split enterprise_team into team_slug/team_id, removed Read-after-Create/Update, used state data in Delete |
| github/resource_github_enterprise_team_membership.go | Split enterprise_team into team_slug/team_id, removed Read-after-Create, set computed fields from API response |
| github/resource_github_enterprise_team_test.go | Removed redundant testCase wrapper pattern, updated field references from enterprise_team to team_slug |
| github/data_source_github_enterprise_team_test.go | Updated field references from enterprise_team to team_slug, fixed formatting |
| github/data_source_github_enterprise_teams_test.go | Fixed PreCheck formatting for consistency |
| github/data_source_github_enterprise_team.go | Removed findEnterpriseTeamBySlugOrID (moved to util file), removed unused import |
| github/data_source_github_enterprise_team_organizations.go | Removed listAllEnterpriseTeamOrganizations (moved to util file), removed unused import |
| website/docs/r/enterprise_team_organizations.html.markdown | Updated documentation to reflect new team_slug/team_id schema |
| website/docs/r/enterprise_team_membership.html.markdown | Updated documentation to reflect new team_slug/team_id schema |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Fix team_slug drift: only set team_slug in Create when user originally provided it, not when using team_id - Remove dead code: delete unused findEnterpriseTeamBySlugOrID function - Document separator safety: add note that GitHub slugs cannot contain '/' - Add MinItems validation test for organization_slugs
github-actions
bot
added
the
Type: Documentation
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 11 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- In Create: set team_id in state when user provides it (not just team_slug) - In Read: only set team_slug if it was configured or during import - This prevents drift when users configure team_id instead of team_slug
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 11 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Use team.ID from API response instead of user input value when setting team_id in state. This ensures consistency with how team_slug is handled.
- Fix ExactlyOneOf in data_source_github_enterprise_team: must include the field itself (slug) in the list, add validation for slug field - Add 404 handling in Delete for membership and organizations resources to gracefully handle already-deleted resources - Handle error in Import function instead of discarding with _ =
- Fix group_id: now always sent in Create/Update requests, allowing empty string to clear the IdP group association - Align data source naming with resources: enterprise_team -> team_slug in data_source_github_enterprise_team_membership and data_source_github_enterprise_team_organizations - Update documentation for renamed fields - Use consistent ID functions across data sources
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Apply code review feedback from @deiga on enterprise teams resources:
Changes
_ = respassignment in DeletetestCasewrapper pattern in acceptance testsenterprise_teamfield intoteam_slugandteam_idwith ExactlyOneOf validation for membership and organizations resourcesfindEnterpriseTeamBySlugOrIDlistAllEnterpriseTeamOrganizationsent:team-name)Testing
All 7 acceptance tests pass:
TestAccGithubEnterpriseTeamTestAccGithubEnterpriseTeamOrganizationsTestAccGithubEnterpriseTeamMembershipTestAccGithubEnterpriseTeamDataSourceTestAccGithubEnterpriseTeamsDataSourceTestAccGithubEnterpriseTeamOrganizationsDataSourceTestAccGithubEnterpriseTeamMembershipDataSource