feat: initial version #1
Conversation
WalkthroughThis update introduces a comprehensive overhaul of the Terraform module, shifting its focus to managing GitHub teams, memberships, and repository collaborators. The changes include new and updated input variables with validation, expanded resource definitions for teams, memberships, collaborators, and team settings, and detailed outputs. The module now enforces organization owner roles and supports review request delegation. Supporting files such as documentation, examples, tests, and CI/CD workflows are also updated or added to match the new functionality. Legacy context and random resource logic are removed to streamline the module’s purpose. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Module
participant GitHub
User->>Module: Provide organization, teams, collaborators inputs
Module->>GitHub: Create teams
Module->>GitHub: Add team members with roles
Module->>GitHub: Set team settings (review delegation)
Module->>GitHub: Add repository collaborators
Module->>GitHub: Accept collaborator invitations
Module->>Module: Enforce org owner as maintainer (check)
Module->>User: Output teams, memberships, collaborators, settings
Poem
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (1)
tests/main.tftest.hcl (1)
1-137: 🛠️ Refactor suggestionAdd review request delegation tests
The newgithub_team_settingsresource for delegation isn’t covered here. Please add arunblock assertinggithub_team_settings.default["platform-engineers"].review_request_delegation.*attributes (e.g., algorithm, member_count, notify).
🧹 Nitpick comments (11)
.trunk/trunk.yaml (1)
23-23: Add Renovate linter v39.245.3
Including Renovate will automate dependency updates—consider verifying any custom Renovate configuration for compatibility.aqua.yaml (1)
4-8: Optional: Enforce package checksums
We currently have thechecksumblock commented out. For stronger supply‑chain security, consider enabling Aqua’s checksum validation (uncomment and adjust as needed) to ensure the integrity of downloaded packages.variables.tf (2)
1-4: Consider adding non‑empty validation forgithub_organization
Right now any string (including"") is allowed. To catch misconfigurations early, you could add:validation { condition = length(var.github_organization) > 0 error_message = "github_organization must not be empty" }
9-10: Clarify naming ofpermission_diff_suppression
The field name is a bit verbose and could be confusing. Consider renaming to something likesuppress_permission_difffor better readability and consistency.examples/complete/versions.tf (1)
2-2: Align version constraint style (optional)
Root uses>= 1.7while the example uses~> 1.7. For consistency, consider matching the root constraint or noting the deliberate difference..github/workflows/test.yaml (3)
1-8: Consider adding a manual trigger
Includingworkflow_dispatchunderon:enables maintainers to run the workflow on demand.
24-31: Pin action versions via tags
Instead of SHA commits, consider using the latest semver tags (e.g.,actions/checkout@v4) for readability and maintainability while still ensuring stability.
44-45: Add explicit step names
Naming these steps (e.g.,name: terraform init,name: terraform test) improves log readability.examples/complete/main.tf (1)
6-31: Demonstrate optional attributes in example
Consider illustrating optional fields (e.g.,parent_team_id,ldap_dn,create_default_maintainer) in the example to help users explore full module capabilities.checks.tf (1)
23-39: Enhance check metadata
You might consider adding adescriptionand explicitseverityargument to thecheckblock for better clarity in CI failures.main.tf (1)
1-28: Flattening locals is effective
Themergeapproach works, though you could also use a single nestedforcomprehension for brevity.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
.github/workflows/test.yaml(1 hunks).gitignore(0 hunks).trunk/trunk.yaml(2 hunks)README.md(2 hunks)aqua.yaml(1 hunks)checks.tf(1 hunks)context.tf(0 hunks)examples/complete/main.tf(1 hunks)examples/complete/outputs.tf(0 hunks)examples/complete/variables.tf(0 hunks)examples/complete/versions.tf(1 hunks)main.tf(1 hunks)outputs.tf(1 hunks)tests/main.tftest.hcl(1 hunks)variables.tf(1 hunks)versions.tf(1 hunks)
💤 Files with no reviewable changes (4)
- .gitignore
- examples/complete/outputs.tf
- examples/complete/variables.tf
- context.tf
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.tf`: You're a Terraform expert who has thoroughly studied all the documentation from Hashicorp https://developer.hashicorp.com/terraform/docs and OpenTofu https://opentofu.or...
**/*.tf: You're a Terraform expert who has thoroughly studied all the documentation from Hashicorp https://developer.hashicorp.com/terraform/docs and OpenTofu https://opentofu.org/docs/.
You have a strong grasp of Terraform syntax and prioritize providing accurate and insightful code suggestions.
As a fan of the Cloud Posse / SweetOps ecosystem, you incorporate many of their best practices https://docs.cloudposse.com/best-practices/terraform/ while balancing them with general Terraform guidelines.
examples/complete/main.tfversions.tfexamples/complete/versions.tfchecks.tfoutputs.tfmain.tfvariables.tf
🧠 Learnings (1)
versions.tf (1)
Learnt from: gberenice
PR: masterpointio/terraform-spacelift-automation#3
File: modules/spacelift-automation/variables.tf:0-0
Timestamp: 2024-10-30T16:38:33.362Z
Learning: Spacelift requires a specific Terraform version number; it does not support version constraints like "~> 1.7.0".
🔇 Additional comments (31)
.trunk/trunk.yaml (4)
5-5: Bump Trunk CLI version to 1.22.12
Updating the CLI version ensures access to the latest features and fixes.
10-10: Upgradetrunkplugin source to v1.6.8
Aligns the plugin reference with the latest release for improved stability.
26-26: Upgrade Checkov to v3.2.405
This patch version includes critical policy checks. Please confirm that all existing Terraform policies still pass in CI.
32-32: Upgrade TruffleHog to v3.88.23
The new release improves secret-detection accuracy. Ensure no new findings are introduced in your pipelines.aqua.yaml (1)
11-15: Approve version bumps
The Aqua packages have been correctly updated to v4.346.0, terraform‑docs v0.20.0, Terraform v1.11.4, and OpenTofu v1.9.0, which align with the module’s new requirements.versions.tf (2)
2-2: Update Terraform version constraint
Raising therequired_versionto>= 1.7correctly aligns with the usage of new object defaults and validation features.
5-7: Switch to GitHub provider
Declaring thegithubprovider fromintegrations/githubat>= 6.0is appropriate for the new resources.README.md (3)
3-3: Approve updated module title
The new# terraform-github-teamsheader clearly reflects the module’s purpose.
29-61: Verify examplesourceaddress
The example usessource = "masterpointio/teams/github". Please confirm this matches the published module on the Terraform Registry or update it to the correct path (e.g.,"masterpointio/terraform-github-teams/github").
144-149: Approve updated badge links
The release, contributor, and issue links now point toterraform-github-teams, matching the new repository name..github/workflows/test.yaml (2)
20-23: Matrix strategy is well-defined
Usingtf: [tofu, terraform]cleanly tests both CLI variants. Nicely done!
35-42: Aqua installation and tagging look good
Pinning both the installer and version ensures reproducible CLI environments.examples/complete/main.tf (2)
1-5: Example module block is correctly configured
Thesource,github_organization, and module naming align well with usage patterns.
33-40: Repository collaborator block is clear
Your example forrepository_collaboratorsshows realistic permission usage. Nice work!checks.tf (3)
1-7: Organization owners set computed correctly
Usingtoseton the comprehension ensures uniqueness for the owner logins.
8-17: Invalid memberships collection is straightforward
The comprehension cleanly builds the error list for owners not inmaintainerrole.
19-21: GitHub organization data source is used appropriately
Fetching the current org users enables dynamic enforcement.tests/main.tftest.hcl (6)
1-4: Mock provider alias is set correctly
This sets up themockGitHub provider for isolated testing.
5-54: Variable inputs are comprehensive
Defininggithub_organization,teams, andrepository_collaboratorsaligns well with module inputs.
56-72: Developer team validation is solid
Checks for default privacy and name ensure core team settings behave as expected.
74-90: Platform engineers test covers privacy override
Good to see overrides validated for non-default configurations.
92-113: Team membership assertions look correct
Validating both maintainer and member roles helps catch misconfigurations early.
115-136: Repository collaborator assertions are on point
Testing multiple repos and permission levels provides strong coverage.main.tf (5)
30-39: Dynamic team creation is well-structured
Iterating withfor_each = var.teamsand mapping attributes aligns with best practices.
41-47: Team memberships are correctly provisioned
Usinglocal.memberships_mapensures unique keys and clean iteration.
49-56: Repository collaborator provisioning is solid
Includingpermission_diff_suppressioncovers drift scenarios nicely.
58-62: Invitation accepter ensures idempotence
Automatically accepting invites completes the collaboration flow.
64-73: Team settings delegation is implemented correctly
The conditionalfor_eachonteams_with_delegationcleanly scopes this resource.outputs.tf (3)
1-11: Approve the structuredteamsoutputThe
teamsblock cleanly maps each GitHub team name to its core properties (id, slug, name, privacy). This provides a clear, typed map that's easy to consume in downstream modules.
13-22: Approve theteam_membershipsoutput mappingUsing the resource instance key (e.g.
team:username) as the map key clearly identifies each membership. Exposingteam_id,username, androlealigns well with expected IaC patterns.
24-33: Approve therepository_collaboratorsoutput mappingThis for‑expression cleanly surfaces collaborator permissions keyed by
repo:username. It’s consistent with the other outputs and makes it straightforward to verify or reference collaborator settings.
| for team_key, team in var.teams : | ||
| lower(replace(replace(team.name, " ", "-"), "/[^a-zA-Z0-9-]/", "")) == team_key | ||
| ]) | ||
| error_message = "Team map key must match the slugified team name (lowercase, spaces replaced with hyphens, special characters removed)." |
There was a problem hiding this comment.
Fix slugification regex
The replace(..., "/[^a-zA-Z0-9-]/", "") pattern will not strip special characters because Terraform’s replace expects the raw regex (no / delimiters). This will break the slug check.
Apply this diff:
- lower(replace(replace(team.name, " ", "-"), "/[^a-zA-Z0-9-]/", "")) == team_key
+ lower(replace(replace(team.name, " ", "-"), "[^a-zA-Z0-9-]", "")) == team_key📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for team_key, team in var.teams : | |
| lower(replace(replace(team.name, " ", "-"), "/[^a-zA-Z0-9-]/", "")) == team_key | |
| ]) | |
| error_message = "Team map key must match the slugified team name (lowercase, spaces replaced with hyphens, special characters removed)." | |
| for team_key, team in var.teams : | |
| lower(replace(replace(team.name, " ", "-"), "[^a-zA-Z0-9-]", "")) == team_key | |
| ]) | |
| error_message = "Team map key must match the slugified team name (lowercase, spaces replaced with hyphens, special characters removed)." |
| permissions: | ||
| actions: read | ||
| checks: write | ||
| contents: read | ||
| id-token: write | ||
| pull-requests: read | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Minimize id-token permission
If you’re not leveraging OIDC, you can restrict id-token to read or remove it to adhere to the principle of least privilege.
There was a problem hiding this comment.
@gberenice this is looking awesome. My comments are mostly questions + small nits.
Let's roll this out in mp-infra so we can see what this looks like for real.
| create_default_maintainer = each.value.create_default_maintainer | ||
| } | ||
|
|
||
| resource "github_team_membership" "default" { |
There was a problem hiding this comment.
Just so I'm clear: Will this invite a new team member to an organization if they're not currently in the org? I think we should make sure that is clear in the README as well -- important info.
There was a problem hiding this comment.
No, this is a team membership. I added a new resource to manage org membership and updated the doc. Le mw know your thoughts!
| output "teams" { | ||
| description = "Map of team names to their properties including ID and slug" | ||
| value = { | ||
| for name, team in github_team.default : name => { |
There was a problem hiding this comment.
How is this different from value = github_team.default?
There was a problem hiding this comment.
My concern is that value = github_team.default would output ALL attributes of the resource, while the current format provides a cleaner output by explicitly selecting the most important (IMO) attributes. Does that make sense?
| providers = { | ||
| github = github.mock | ||
| } |
There was a problem hiding this comment.
Aha this makes it so the GH provider is not initialized at all? So we're just testing the logic and not the underlying provider's logic?
There was a problem hiding this comment.
Yeah, we're using a completely mocked version of the GitHub provider, not the real one: no actual GitHub API calls are made, so no real resources are created/modified/deleted. I feel like this is safer, because we're not actually making any changes to GitHub org membership and team structure.
This still allows us to test the module's logic and transformations, and expected resource configurations. How do you feel about this approach?
| for team_key, team in var.teams : | ||
| lower(replace(replace(team.name, " ", "-"), "/[^a-zA-Z0-9-]/", "")) == team_key | ||
| ]) | ||
| error_message = "Team map key must match the slugified team name (lowercase, spaces replaced with hyphens, special characters removed)." |
There was a problem hiding this comment.
Why do we need this validation? The key doesn't seem to be used AFAICT so why do they need to match + follow a specific pattern.
There was a problem hiding this comment.
I added this validation to ensure that the team's map key matches GitHub's slug format for team URLs and API endpoints. As GitHub automatically generates a slug for each team (used in URLs and API calls), I'd like to keep the consistency between our module and GitHub's API.
| name = "Developers" | ||
| members = [ | ||
| { | ||
| username = "rachel" |
There was a problem hiding this comment.
I love the Friends reference
| default = 2 | ||
| variable "github_organization" { | ||
| type = string | ||
| description = "The GitHub organization name" |
There was a problem hiding this comment.
Given integrations/terraform-provider-github#241, I wonder if we should add that github_organization is case insensitive?
There was a problem hiding this comment.
Hmm, I think this is fixed. I don't see drifts in mp-infra root module, and we have there various cases.
See https://github.com/masterpointio/mp-infra/pull/60
|
|
||
| variable "repository_collaborators" { | ||
| type = map(list(object({ | ||
| username = string |
There was a problem hiding this comment.
Given integrations/terraform-provider-github#241, I wonder if we should add that username is case insensitive?
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
tests/main.tftest.hcl (1)
1-223: 🛠️ Refactor suggestionAdd tests for review request delegation
There are currently no assertions for thegithub_team_settingsresource with delegation. It’s valuable to validate that the delegation settings are applied correctly. For example:run "validate_review_request_delegation_settings" { command = plan providers = { github = github.mock } assert { condition = github_team_settings.default["platform-engineers"].review_request_delegation.algorithm == "ROUND_ROBIN" error_message = "Delegation algorithm should be ROUND_ROBIN" } assert { condition = github_team_settings.default["platform-engineers"].review_request_delegation.member_count == 2 error_message = "Delegation member_count should be 2" } assert { condition = github_team_settings.default["platform-engineers"].review_request_delegation.notify == true error_message = "Delegation notify flag should be true" } }
♻️ Duplicate comments (1)
variables.tf (1)
115-120: Fix slugification regex
Terraform’sreplaceexpects raw regex without/delimiters, so the current pattern"/[^a-zA-Z0-9-]/"won’t strip special characters. Apply:- lower(replace(replace(team.name, " ", "-"), "/[^a-zA-Z0-9-]/", "")) == team_key + lower(replace(replace(team.name, " ", "-"), "[^a-zA-Z0-9-]", "")) == team_key
🧹 Nitpick comments (6)
examples/complete/versions.tf (1)
1-3: Loosen Terraform version constraint
Pinningrequired_version = "1.9.0"forces exactly that version. Consider allowing patch upgrades (or future minors) by using a range or pessimistic constraint, for example:required_version = "~> 1.9.0" # allows 1.9.x # or required_version = ">= 1.9.0, < 2.0.0" # allows any 1.xvariables.tf (2)
1-4: Add non-empty validation forgithub_organization
To catch empty or unintended values early, you can enforce that the organization name is not blank:variable "github_organization" { type = string description = "The GitHub organization name" validation { condition = var.github_organization != "" error_message = "github_organization must not be empty." } }
73-81: Simplify delegation validation withcoalesce
You can make the review request delegation checks more concise by leveragingcoalesce:validation { condition = alltrue([ for team in values(var.teams) : contains( ["ROUND_ROBIN", "LOAD_BALANCE"], coalesce(team.review_request_delegation.algorithm, "ROUND_ROBIN") ) ]) error_message = "Review request delegation algorithm must be either 'ROUND_ROBIN' or 'LOAD_BALANCE'." }Likewise, use
coalesce(team.review_request_delegation.member_count, 1) >= 1for the member_count check.examples/complete/main.tf (1)
6-23: Explicitly show membership toggle
Sinceorganization_memberships_enableddefaults totrue, consider adding it in the example to clarify that Terraform will manage org memberships:organization_memberships_enabled = truemain.tf (1)
53-61: Optional: Explicitdepends_onfor clarity
Although Terraform infers the dependency from the reference togithub_team.defaultingithub_team_membership, you may add an explicitdepends_on = [github_team.default]to make the ordering clear:resource "github_team_membership" "default" { for_each = local.memberships_map team_id = each.value.team_id username = each.value.username role = each.value.role depends_on = [github_team.default] }checks.tf (1)
18-28: Consider stable ordering for user listTo ensure predictable ordering in
data.github_users, wrap the distinct list insort(...)for reproducible data lookups and test consistency:- all_usernames = distinct(concat( + all_usernames = sort(distinct(concat( # Organization members [for member in var.organization_memberships : member.username], # Repository collaborators flatten([ for collaborators in var.repository_collaborators : [ for collab in collaborators : collab.username ] ]) - )) + )))
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.terraform-docs.yaml(1 hunks)README.md(2 hunks)checks.tf(1 hunks)examples/complete/main.tf(1 hunks)examples/complete/versions.tf(1 hunks)main.tf(1 hunks)outputs.tf(1 hunks)tests/main.tftest.hcl(1 hunks)variables.tf(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .terraform-docs.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- README.md
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.tf`: You're a Terraform expert who has thoroughly studied all the documentation from Hashicorp https://developer.hashicorp.com/terraform/docs and OpenTofu https://opentofu.or...
**/*.tf: You're a Terraform expert who has thoroughly studied all the documentation from Hashicorp https://developer.hashicorp.com/terraform/docs and OpenTofu https://opentofu.org/docs/.
You have a strong grasp of Terraform syntax and prioritize providing accurate and insightful code suggestions.
As a fan of the Cloud Posse / SweetOps ecosystem, you incorporate many of their best practices https://docs.cloudposse.com/best-practices/terraform/ while balancing them with general Terraform guidelines.
examples/complete/versions.tfoutputs.tfchecks.tfexamples/complete/main.tfvariables.tfmain.tf
🧠 Learnings (1)
outputs.tf (1)
Learnt from: Gowiem
PR: masterpointio/terraform-github-teams#1
File: outputs.tf:35-47
Timestamp: 2025-04-18T16:33:59.357Z
Learning: In Terraform for-expressions that iterate over resource collections, if the collection is empty (no resources of that type exist), Terraform won't execute the loop body at all, preventing potential index errors. For the GitHub Teams module, github_team_settings resources are only created when teams have review_request_delegation configured, so there's no need to add safeguards against empty delegation lists.
🔇 Additional comments (14)
examples/complete/versions.tf (1)
5-8: Approve provider source & version
The GitHub provider is correctly sourced (integrations/github) and pinned (~> 6.0), ensuring compatibility with the module inputs.examples/complete/main.tf (3)
1-4: Approve module instantiation
The example correctly references the module source and sets the requiredgithub_organization.
25-49: Approve teams and delegation example
Theteamsblock demonstratesname,privacy, members with roles, and review request delegation settings in line with your variable definitions and validations.
52-59: Approve repository collaborators example
Therepository_collaboratorsmapping follows the required'owner/repo'pattern and uses valid permissions.checks.tf (5)
1-7: Clear identification of organization ownersThe local
org_ownerscorrectly collects all “admin” users into a set for deduplication, making downstream checks reliable.
9-16: Accurate detection of invalid membershipsThe
invalid_membershipslocal filters org owners who aren’t maintainers and builds a structured list for error reporting. This will produce clear, actionable messages.
31-38: Well-scoped data sourcesThe
github_organizationandgithub_usersdata blocks are properly scoped to use the locals, ensuring only relevant users are fetched. Nice separation of concerns.
39-55: Robust check for organization owner rolesThe
github_team_org_owners_roleassertion leveragesalltrue()to enforce that every org owner has a “maintainer” role. The error aggregation usinginvalid_membershipsis clear and user-friendly.
57-65: Validation of existing GitHub usersThe
validate_users_existcheck succinctly asserts that no unknown logins remain. The formatted error message will clearly list any missing users.outputs.tf (5)
1-11: Comprehensive team outputThe
teamsoutput cleanly maps team names to their core properties (id,slug,name,privacy), making it straightforward to reference teams downstream.
13-22: Structured team memberships outputThe
team_membershipsoutput provides a clear map of membership identifiers to theirteam_id,username, androle, which is great for auditing team composition.
24-33: Detailed repository collaborator outputThe
repository_collaboratorsoutput exposes each collaborator’s repository and permission level in a predictable map structure—very useful for governance checks.
35-47: Handling of team settings with delegationThe
team_settingsoutput captures review request delegation attributes succinctly. Sincegithub_team_settingsresources only exist when delegation is configured, indexing into[0]is safe here.
49-54: Organization membership roles outputThe
organization_membershipsoutput cleanly maps usernames to their org roles, offering a quick reference of membership status.
| locals { | ||
| memberships_map = merge([ | ||
| for team_name, team in var.teams : { | ||
| for member in team.members : "${team_name}:${member.username}" => { | ||
| team_id = github_team.default[team_name].id | ||
| username = member.username | ||
| role = member.role | ||
| } | ||
| } | ||
| ]...) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard merge() when inputs are empty
Using merge([…]…) on an empty list errors out if var.teams (default {}) has no entries. Wrap with a conditional fallback:
locals {
memberships_map = length(var.teams) > 0 ?
merge([
for team_name, team in var.teams : {
for member in team.members :
"${team_name}:${member.username}" => {
team_id = github_team.default[team_name].id
username = member.username
role = member.role
}
}
]...) : {}
}This prevents failures when the map is empty.
| repository_collaborators = merge([ | ||
| for repo_name, collaborators in var.repository_collaborators : { | ||
| for collab in collaborators : "${repo_name}:${collab.username}" => { | ||
| repository = repo_name | ||
| username = collab.username | ||
| permission = collab.permission | ||
| permission_diff_suppression = collab.permission_diff_suppression | ||
| } | ||
| } | ||
| ]...) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard collaborators merge for empty input
Similarly, protect the collaborators merge against an empty var.repository_collaborators:
locals {
repository_collaborators = length(var.repository_collaborators) > 0 ?
merge([
for repo_name, collaborators in var.repository_collaborators : {
for collab in collaborators :
"${repo_name}:${collab.username}" => {
repository = repo_name
username = collab.username
permission = collab.permission
permission_diff_suppression = collab.permission_diff_suppression
}
}
]...) : {}
}
what
why
references
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores
Refactor