From e2657851164c1d4e4cd40cf6e6dc504df563fd84 Mon Sep 17 00:00:00 2001 From: Timo Sand Date: Mon, 12 Jan 2026 00:24:40 +0200 Subject: [PATCH 1/6] feat(ruleset): add required_reviewers support for pull_request rules Add support for the required_reviewers rule parameter in pull_request rules for both organization and repository rulesets. This allows requiring specific team reviewers to approve changes based on file patterns. Implementation includes: - requiredReviewersSchema() shared schema definition - expandRequiredReviewers() for Terraform->API conversion - flattenRequiredReviewers() for API->Terraform conversion - Integration into expandRules() and flattenRules() - Schema integration in both org and repo ruleset resources --- .../resource_github_organization_ruleset.go | 1 + github/resource_github_repository_ruleset.go | 1 + github/util_rules.go | 119 ++++++++++++++++++ 3 files changed, 121 insertions(+) diff --git a/github/resource_github_organization_ruleset.go b/github/resource_github_organization_ruleset.go index b225b90a69..d9af2227e8 100644 --- a/github/resource_github_organization_ruleset.go +++ b/github/resource_github_organization_ruleset.go @@ -249,6 +249,7 @@ func resourceGithubOrganizationRuleset() *schema.Resource { Default: false, Description: "All conversations on code must be resolved before a pull request can be merged. Defaults to `false`.", }, + "required_reviewers": requiredReviewersSchema(), }, }, }, diff --git a/github/resource_github_repository_ruleset.go b/github/resource_github_repository_ruleset.go index eacef5f781..b3344df8b2 100644 --- a/github/resource_github_repository_ruleset.go +++ b/github/resource_github_repository_ruleset.go @@ -236,6 +236,7 @@ func resourceGithubRepositoryRuleset() *schema.Resource { Default: false, Description: "All conversations on code must be resolved before a pull request can be merged. Defaults to `false`.", }, + "required_reviewers": requiredReviewersSchema(), }, }, }, diff --git a/github/util_rules.go b/github/util_rules.go index ebf84e679f..25538f56e5 100644 --- a/github/util_rules.go +++ b/github/util_rules.go @@ -8,8 +8,54 @@ import ( "github.com/google/go-github/v82/github" "github.com/hashicorp/terraform-plugin-log/tflog" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" + "github.com/hashicorp/terraform-plugin-sdk/v2/helper/validation" ) +func requiredReviewersSchema() *schema.Schema { + return &schema.Schema{ + Type: schema.TypeList, + Optional: true, + Description: "Require specific reviewers to approve pull requests targeting matching branches. Note: This feature is in beta and subject to change.", + Elem: &schema.Resource{ + Schema: map[string]*schema.Schema{ + "reviewer": { + Type: schema.TypeList, + Required: true, + MaxItems: 1, + Description: "The reviewer that must review matching files.", + Elem: &schema.Resource{ + Schema: map[string]*schema.Schema{ + "id": { + Type: schema.TypeInt, + Required: true, + Description: "The ID of the reviewer that must review.", + }, + "type": { + Type: schema.TypeString, + Required: true, + ValidateDiagFunc: toDiagFunc(validation.StringInSlice([]string{"Team"}, false), "type"), + Description: "The type of reviewer. Currently only `Team` is supported.", + }, + }, + }, + }, + "file_patterns": { + Type: schema.TypeList, + Required: true, + MinItems: 1, + Description: "File patterns (fnmatch syntax) that this reviewer must approve.", + Elem: &schema.Schema{Type: schema.TypeString}, + }, + "minimum_approvals": { + Type: schema.TypeInt, + Required: true, + Description: "Minimum number of approvals required from this reviewer. Set to 0 to make approval optional.", + }, + }, + }, + } +} + // Helper function to safely convert interface{} to int, handling both int and float64. func toInt(v any) int { switch val := v.(type) { @@ -53,6 +99,72 @@ func toPullRequestMergeMethods(input any) []github.PullRequestMergeMethod { return mergeMethods } +// expandRequiredReviewers converts Terraform schema data to go-github RequiredReviewers. +func expandRequiredReviewers(input []any) []*github.RulesetRequiredReviewer { + if len(input) == 0 { + return nil + } + + reviewers := make([]*github.RulesetRequiredReviewer, 0, len(input)) + for _, item := range input { + reviewerMap := item.(map[string]any) + + var reviewer *github.RulesetReviewer + if rv, ok := reviewerMap["reviewer"].([]any); ok && len(rv) != 0 { + reviewerData := rv[0].(map[string]any) + reviewerType := github.RulesetReviewerType(reviewerData["type"].(string)) + reviewer = &github.RulesetReviewer{ + ID: github.Ptr(int64(reviewerData["id"].(int))), + Type: &reviewerType, + } + } + + filePatterns := make([]string, 0) + if fp, ok := reviewerMap["file_patterns"].([]any); ok { + for _, p := range fp { + filePatterns = append(filePatterns, p.(string)) + } + } + + reviewers = append(reviewers, &github.RulesetRequiredReviewer{ + MinimumApprovals: github.Ptr(reviewerMap["minimum_approvals"].(int)), + FilePatterns: filePatterns, + Reviewer: reviewer, + }) + } + return reviewers +} + +// flattenRequiredReviewers converts go-github RequiredReviewers to Terraform schema data. +func flattenRequiredReviewers(reviewers []*github.RulesetRequiredReviewer) []map[string]any { + if len(reviewers) == 0 { + return nil + } + + reviewersList := make([]map[string]any, 0, len(reviewers)) + for _, rr := range reviewers { + reviewerMap := map[string]any{ + "file_patterns": rr.FilePatterns, + "minimum_approvals": 0, + } + if rr.MinimumApprovals != nil { + reviewerMap["minimum_approvals"] = *rr.MinimumApprovals + } + if rr.Reviewer != nil { + reviewerData := map[string]any{} + if rr.Reviewer.ID != nil { + reviewerData["id"] = int(*rr.Reviewer.ID) + } + if rr.Reviewer.Type != nil { + reviewerData["type"] = string(*rr.Reviewer.Type) + } + reviewerMap["reviewer"] = []map[string]any{reviewerData} + } + reviewersList = append(reviewersList, reviewerMap) + } + return reviewersList +} + func resourceGithubRulesetObject(d *schema.ResourceData, org string) github.RepositoryRuleset { isOrgLevel := len(org) > 0 @@ -326,6 +438,12 @@ func expandRules(input []any, org bool) *github.RepositoryRulesetRules { RequiredReviewThreadResolution: pullRequestMap["required_review_thread_resolution"].(bool), AllowedMergeMethods: toPullRequestMergeMethods(allowedMergeMethods), } + + // Add required reviewers if provided + if reqReviewers, ok := pullRequestMap["required_reviewers"].([]any); ok && len(reqReviewers) != 0 { + params.RequiredReviewers = expandRequiredReviewers(reqReviewers) + } + rulesetRules.PullRequest = params } @@ -576,6 +694,7 @@ func flattenRules(ctx context.Context, rules *github.RepositoryRulesetRules, org "required_approving_review_count": rules.PullRequest.RequiredApprovingReviewCount, "required_review_thread_resolution": rules.PullRequest.RequiredReviewThreadResolution, "allowed_merge_methods": rules.PullRequest.AllowedMergeMethods, + "required_reviewers": flattenRequiredReviewers(rules.PullRequest.RequiredReviewers), }) tflog.Debug(ctx, "Flattened Pull Request rules slice", map[string]any{"pull_request": pullRequestSlice}) rulesMap["pull_request"] = pullRequestSlice From e6db37ef9f90a1dfca14f833d245c62225eeb85c Mon Sep 17 00:00:00 2001 From: Timo Sand Date: Mon, 12 Jan 2026 00:25:15 +0200 Subject: [PATCH 2/6] test(ruleset): add tests for required_reviewers feature Add comprehensive unit tests for the expand/flatten helper functions: - TestExpandRequiredReviewers: validates expansion logic - TestExpandRequiredReviewersEmpty: handles edge cases - TestFlattenRequiredReviewers: validates flattening logic - TestFlattenRequiredReviewersEmpty: handles edge cases - TestRoundTripRequiredReviewers: verifies data integrity Add acceptance tests for both organization and repository rulesets: - Single reviewer configuration with file patterns - Multiple reviewers configuration - Update operations (minimum_approvals change) - Import state verification --- ...source_github_organization_ruleset_test.go | 201 ++++++++++++++++++ ...resource_github_repository_ruleset_test.go | 153 +++++++++++++ github/util_rules_test.go | 189 ++++++++++++++++ 3 files changed, 543 insertions(+) diff --git a/github/resource_github_organization_ruleset_test.go b/github/resource_github_organization_ruleset_test.go index fb189c7c92..f0a20e8d61 100644 --- a/github/resource_github_organization_ruleset_test.go +++ b/github/resource_github_organization_ruleset_test.go @@ -934,3 +934,204 @@ func TestOrganizationPushRulesetSupport(t *testing.T) { t.Errorf("Expected 4 restricted file extensions, got %d", len(restrictedExts)) } } + +func TestAccGithubOrganizationRuleset_requiredReviewers(t *testing.T) { + randomID := acctest.RandStringFromCharSet(5, acctest.CharSetAlphaNum) + teamName := fmt.Sprintf("%steam-req-rev-%s", testResourcePrefix, randomID) + rulesetName := fmt.Sprintf("%s-ruleset-req-rev-%s", testResourcePrefix, randomID) + + config := fmt.Sprintf(` +resource "github_team" "test" { + name = "%s" +} + +resource "github_organization_ruleset" "test" { + name = "%s" + target = "branch" + enforcement = "active" + + conditions { + repository_name { + include = ["~ALL"] + exclude = [] + } + + ref_name { + include = ["~ALL"] + exclude = [] + } + } + + rules { + pull_request { + allowed_merge_methods = ["merge", "squash"] + required_approving_review_count = 1 + + required_reviewers { + reviewer { + id = github_team.test.id + type = "Team" + } + file_patterns = ["*.go", "src/**/*.ts"] + minimum_approvals = 1 + } + } + } +} +`, teamName, rulesetName) + + // Updated config: change minimum_approvals from 1 to 2 + configUpdated := fmt.Sprintf(` +resource "github_team" "test" { + name = "%s" +} + +resource "github_organization_ruleset" "test" { + name = "%s" + target = "branch" + enforcement = "active" + + conditions { + repository_name { + include = ["~ALL"] + exclude = [] + } + + ref_name { + include = ["~ALL"] + exclude = [] + } + } + + rules { + pull_request { + allowed_merge_methods = ["merge", "squash"] + required_approving_review_count = 1 + + required_reviewers { + reviewer { + id = github_team.test.id + type = "Team" + } + file_patterns = ["*.go", "src/**/*.ts"] + minimum_approvals = 2 + } + } + } +} +`, teamName, rulesetName) + + resource.Test(t, resource.TestCase{ + PreCheck: func() { skipUnlessHasPaidOrgs(t) }, + ProviderFactories: providerFactories, + Steps: []resource.TestStep{ + { + Config: config, + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr("github_organization_ruleset.test", "name", rulesetName), + resource.TestCheckResourceAttr("github_organization_ruleset.test", "target", "branch"), + resource.TestCheckResourceAttr("github_organization_ruleset.test", "enforcement", "active"), + resource.TestCheckResourceAttr("github_organization_ruleset.test", "rules.0.pull_request.0.required_reviewers.#", "1"), + resource.TestCheckResourceAttr("github_organization_ruleset.test", "rules.0.pull_request.0.required_reviewers.0.minimum_approvals", "1"), + resource.TestCheckResourceAttr("github_organization_ruleset.test", "rules.0.pull_request.0.required_reviewers.0.file_patterns.#", "2"), + resource.TestCheckResourceAttr("github_organization_ruleset.test", "rules.0.pull_request.0.required_reviewers.0.file_patterns.0", "*.go"), + resource.TestCheckResourceAttr("github_organization_ruleset.test", "rules.0.pull_request.0.required_reviewers.0.file_patterns.1", "src/**/*.ts"), + resource.TestCheckResourceAttr("github_organization_ruleset.test", "rules.0.pull_request.0.required_reviewers.0.reviewer.0.type", "Team"), + ), + }, + { + Config: configUpdated, + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr("github_organization_ruleset.test", "rules.0.pull_request.0.required_reviewers.0.minimum_approvals", "2"), + ), + }, + { + ResourceName: "github_organization_ruleset.test", + ImportState: true, + ImportStateVerify: true, + ImportStateVerifyIgnore: []string{"etag"}, + }, + }, + }) +} + +func TestAccGithubOrganizationRuleset_requiredReviewersMultiple(t *testing.T) { + randomID := acctest.RandStringFromCharSet(5, acctest.CharSetAlphaNum) + teamName1 := fmt.Sprintf("%steam-req-rev-1-%s", testResourcePrefix, randomID) + teamName2 := fmt.Sprintf("%steam-req-rev-2-%s", testResourcePrefix, randomID) + rulesetName := fmt.Sprintf("%s-ruleset-multi-rev-%s", testResourcePrefix, randomID) + + config := fmt.Sprintf(` +resource "github_team" "test1" { + name = "%s" +} + +resource "github_team" "test2" { + name = "%s" +} + +resource "github_organization_ruleset" "test" { + name = "%s" + target = "branch" + enforcement = "active" + + conditions { + repository_name { + include = ["~ALL"] + exclude = [] + } + + ref_name { + include = ["~ALL"] + exclude = [] + } + } + + rules { + pull_request { + allowed_merge_methods = ["merge", "squash"] + required_approving_review_count = 1 + + required_reviewers { + reviewer { + id = github_team.test1.id + type = "Team" + } + file_patterns = ["*.go"] + minimum_approvals = 1 + } + + required_reviewers { + reviewer { + id = github_team.test2.id + type = "Team" + } + file_patterns = ["*.md", "docs/**/*"] + minimum_approvals = 1 + } + } + } +} +`, teamName1, teamName2, rulesetName) + + resource.Test(t, resource.TestCase{ + PreCheck: func() { skipUnlessHasPaidOrgs(t) }, + ProviderFactories: providerFactories, + Steps: []resource.TestStep{ + { + Config: config, + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr("github_organization_ruleset.test", "name", rulesetName), + resource.TestCheckResourceAttr("github_organization_ruleset.test", "target", "branch"), + resource.TestCheckResourceAttr("github_organization_ruleset.test", "enforcement", "active"), + resource.TestCheckResourceAttr("github_organization_ruleset.test", "rules.0.pull_request.0.required_reviewers.#", "2"), + resource.TestCheckResourceAttr("github_organization_ruleset.test", "rules.0.pull_request.0.required_reviewers.0.minimum_approvals", "1"), + resource.TestCheckResourceAttr("github_organization_ruleset.test", "rules.0.pull_request.0.required_reviewers.0.file_patterns.#", "1"), + resource.TestCheckResourceAttr("github_organization_ruleset.test", "rules.0.pull_request.0.required_reviewers.0.file_patterns.0", "*.go"), + resource.TestCheckResourceAttr("github_organization_ruleset.test", "rules.0.pull_request.0.required_reviewers.1.minimum_approvals", "1"), + resource.TestCheckResourceAttr("github_organization_ruleset.test", "rules.0.pull_request.0.required_reviewers.1.file_patterns.#", "2"), + ), + }, + }, + }) +} diff --git a/github/resource_github_repository_ruleset_test.go b/github/resource_github_repository_ruleset_test.go index cb4d874154..3cfc6d6ae0 100644 --- a/github/resource_github_repository_ruleset_test.go +++ b/github/resource_github_repository_ruleset_test.go @@ -759,6 +759,159 @@ func TestAccGithubRepositoryRulesetValidation(t *testing.T) { }) } +func TestAccGithubRepositoryRuleset_requiredReviewers(t *testing.T) { + randomID := acctest.RandStringFromCharSet(5, acctest.CharSetAlphaNum) + repoName := fmt.Sprintf("%srepo-ruleset-req-rev-%s", testResourcePrefix, randomID) + teamName := fmt.Sprintf("%steam-req-rev-%s", testResourcePrefix, randomID) + rulesetName := fmt.Sprintf("%s-ruleset-req-rev-%s", testResourcePrefix, randomID) + baseRepoVisibility := "public" + + if testAccConf.authMode == enterprise { + // This enables repos to be created even in GHEC EMU + baseRepoVisibility = "private" + } + + config := fmt.Sprintf(` +resource "github_repository" "test" { + name = "%s" + auto_init = true + visibility = "%s" + + ignore_vulnerability_alerts_during_read = true +} + +resource "github_team" "test" { + name = "%s" +} + +resource "github_team_repository" "test" { + team_id = github_team.test.id + repository = github_repository.test.name + permission = "push" +} + +resource "github_repository_ruleset" "test" { + name = "%s" + repository = github_repository.test.name + target = "branch" + enforcement = "active" + + + conditions { + ref_name { + include = ["~ALL"] + exclude = [] + } + } + + rules { + pull_request { + allowed_merge_methods = ["merge", "squash"] + required_approving_review_count = 1 + + required_reviewers { + reviewer { + id = github_team.test.id + type = "Team" + } + file_patterns = ["*.go"] + minimum_approvals = 1 + } + } + } + + depends_on = [github_team_repository.test] +} +`, repoName, baseRepoVisibility, teamName, rulesetName) + + // Updated config: change minimum_approvals from 1 to 2 + configUpdated := fmt.Sprintf(` +resource "github_repository" "test" { + name = "%s" + auto_init = true + visibility = "%s" + + ignore_vulnerability_alerts_during_read = true +} + +resource "github_team" "test" { + name = "%s" +} + +resource "github_team_repository" "test" { + team_id = github_team.test.id + repository = github_repository.test.name + permission = "push" +} + +resource "github_repository_ruleset" "test" { + name = "%s" + repository = github_repository.test.name + target = "branch" + enforcement = "active" + + + conditions { + ref_name { + include = ["~ALL"] + exclude = [] + } + } + + rules { + pull_request { + allowed_merge_methods = ["merge", "squash"] + required_approving_review_count = 1 + + required_reviewers { + reviewer { + id = github_team.test.id + type = "Team" + } + file_patterns = ["*.go"] + minimum_approvals = 2 + } + } + } + + depends_on = [github_team_repository.test] +} +`, repoName, baseRepoVisibility, teamName, rulesetName) + + resource.Test(t, resource.TestCase{ + PreCheck: func() { skipUnlessHasOrgs(t) }, + ProviderFactories: providerFactories, + Steps: []resource.TestStep{ + { + Config: config, + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr("github_repository_ruleset.test", "name", rulesetName), + resource.TestCheckResourceAttr("github_repository_ruleset.test", "target", "branch"), + resource.TestCheckResourceAttr("github_repository_ruleset.test", "enforcement", "active"), + resource.TestCheckResourceAttr("github_repository_ruleset.test", "rules.0.pull_request.0.required_reviewers.#", "1"), + resource.TestCheckResourceAttr("github_repository_ruleset.test", "rules.0.pull_request.0.required_reviewers.0.minimum_approvals", "1"), + resource.TestCheckResourceAttr("github_repository_ruleset.test", "rules.0.pull_request.0.required_reviewers.0.file_patterns.#", "1"), + resource.TestCheckResourceAttr("github_repository_ruleset.test", "rules.0.pull_request.0.required_reviewers.0.file_patterns.0", "*.go"), + resource.TestCheckResourceAttr("github_repository_ruleset.test", "rules.0.pull_request.0.required_reviewers.0.reviewer.0.type", "Team"), + ), + }, + { + Config: configUpdated, + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr("github_repository_ruleset.test", "rules.0.pull_request.0.required_reviewers.0.minimum_approvals", "2"), + ), + }, + { + ResourceName: "github_repository_ruleset.test", + ImportState: true, + ImportStateVerify: true, + ImportStateIdFunc: importRepositoryRulesetByResourcePaths("github_repository.test", "github_repository_ruleset.test"), + ImportStateVerifyIgnore: []string{"etag"}, + }, + }, + }) +} + func importRepositoryRulesetByResourcePaths(repoLogicalName, rulesetLogicalName string) resource.ImportStateIdFunc { // test importing using an ID of the form : return func(s *terraform.State) (string, error) { diff --git a/github/util_rules_test.go b/github/util_rules_test.go index 7ac73bb02c..3283b4fa6e 100644 --- a/github/util_rules_test.go +++ b/github/util_rules_test.go @@ -625,3 +625,192 @@ func TestFlattenConditions_PushRuleset_WithRepositoryIdOnly(t *testing.T) { t.Errorf("Expected repository IDs [12345, 67890], got %v", repoIDs) } } + +func TestExpandRequiredReviewers(t *testing.T) { + input := []any{ + map[string]any{ + "reviewer": []any{ + map[string]any{ + "id": 12345, + "type": "Team", + }, + }, + "file_patterns": []any{"*.go", "src/**/*.ts"}, + "minimum_approvals": 2, + }, + map[string]any{ + "reviewer": []any{ + map[string]any{ + "id": 67890, + "type": "Team", + }, + }, + "file_patterns": []any{"docs/**/*.md"}, + "minimum_approvals": 1, + }, + } + + result := expandRequiredReviewers(input) + + if len(result) != 2 { + t.Fatalf("Expected 2 reviewers, got %d", len(result)) + } + + // Check first reviewer + if result[0].Reviewer == nil { + t.Fatal("Expected first reviewer to have a Reviewer") + } + if *result[0].Reviewer.ID != 12345 { + t.Errorf("Expected first reviewer ID to be 12345, got %d", *result[0].Reviewer.ID) + } + if *result[0].Reviewer.Type != github.RulesetReviewerTypeTeam { + t.Errorf("Expected first reviewer type to be Team, got %s", *result[0].Reviewer.Type) + } + if *result[0].MinimumApprovals != 2 { + t.Errorf("Expected first reviewer minimum approvals to be 2, got %d", *result[0].MinimumApprovals) + } + if len(result[0].FilePatterns) != 2 { + t.Fatalf("Expected first reviewer to have 2 file patterns, got %d", len(result[0].FilePatterns)) + } + if result[0].FilePatterns[0] != "*.go" || result[0].FilePatterns[1] != "src/**/*.ts" { + t.Errorf("Unexpected file patterns for first reviewer: %v", result[0].FilePatterns) + } + + // Check second reviewer + if result[1].Reviewer == nil { + t.Fatal("Expected second reviewer to have a Reviewer") + } + if *result[1].Reviewer.ID != 67890 { + t.Errorf("Expected second reviewer ID to be 67890, got %d", *result[1].Reviewer.ID) + } + if *result[1].MinimumApprovals != 1 { + t.Errorf("Expected second reviewer minimum approvals to be 1, got %d", *result[1].MinimumApprovals) + } +} + +func TestExpandRequiredReviewersEmpty(t *testing.T) { + result := expandRequiredReviewers([]any{}) + if result != nil { + t.Error("Expected nil for empty input") + } + + result = expandRequiredReviewers(nil) + if result != nil { + t.Error("Expected nil for nil input") + } +} + +func TestFlattenRequiredReviewers(t *testing.T) { + reviewerType := github.RulesetReviewerTypeTeam + reviewers := []*github.RulesetRequiredReviewer{ + { + MinimumApprovals: github.Ptr(2), + FilePatterns: []string{"*.go", "src/**/*.ts"}, + Reviewer: &github.RulesetReviewer{ + ID: github.Ptr(int64(12345)), + Type: &reviewerType, + }, + }, + { + MinimumApprovals: github.Ptr(1), + FilePatterns: []string{"docs/**/*.md"}, + Reviewer: &github.RulesetReviewer{ + ID: github.Ptr(int64(67890)), + Type: &reviewerType, + }, + }, + } + + result := flattenRequiredReviewers(reviewers) + + if len(result) != 2 { + t.Fatalf("Expected 2 reviewers, got %d", len(result)) + } + + // Check first reviewer + if result[0]["minimum_approvals"] != 2 { + t.Errorf("Expected first reviewer minimum approvals to be 2, got %v", result[0]["minimum_approvals"]) + } + filePatterns := result[0]["file_patterns"].([]string) + if len(filePatterns) != 2 { + t.Fatalf("Expected first reviewer to have 2 file patterns, got %d", len(filePatterns)) + } + if filePatterns[0] != "*.go" || filePatterns[1] != "src/**/*.ts" { + t.Errorf("Unexpected file patterns for first reviewer: %v", filePatterns) + } + + reviewerBlock := result[0]["reviewer"].([]map[string]any) + if len(reviewerBlock) != 1 { + t.Fatalf("Expected 1 reviewer block, got %d", len(reviewerBlock)) + } + if reviewerBlock[0]["id"] != 12345 { + t.Errorf("Expected first reviewer ID to be 12345, got %v", reviewerBlock[0]["id"]) + } + if reviewerBlock[0]["type"] != "Team" { + t.Errorf("Expected first reviewer type to be Team, got %v", reviewerBlock[0]["type"]) + } + + // Check second reviewer + if result[1]["minimum_approvals"] != 1 { + t.Errorf("Expected second reviewer minimum approvals to be 1, got %v", result[1]["minimum_approvals"]) + } +} + +func TestFlattenRequiredReviewersEmpty(t *testing.T) { + result := flattenRequiredReviewers(nil) + if result != nil { + t.Error("Expected nil for nil input") + } + + result = flattenRequiredReviewers([]*github.RulesetRequiredReviewer{}) + if result != nil { + t.Error("Expected nil for empty slice input") + } +} + +func TestRoundTripRequiredReviewers(t *testing.T) { + // Start with Terraform-style input + input := []any{ + map[string]any{ + "reviewer": []any{ + map[string]any{ + "id": 12345, + "type": "Team", + }, + }, + "file_patterns": []any{"*.go", "src/**/*.ts"}, + "minimum_approvals": 2, + }, + } + + // Expand to go-github types + expanded := expandRequiredReviewers(input) + + // Flatten back to Terraform types + flattened := flattenRequiredReviewers(expanded) + + // Verify the round trip maintains data + if len(flattened) != 1 { + t.Fatalf("Expected 1 reviewer after round trip, got %d", len(flattened)) + } + + if flattened[0]["minimum_approvals"] != 2 { + t.Errorf("Expected minimum_approvals to be 2 after round trip, got %v", flattened[0]["minimum_approvals"]) + } + + filePatterns := flattened[0]["file_patterns"].([]string) + if len(filePatterns) != 2 { + t.Fatalf("Expected 2 file patterns after round trip, got %d", len(filePatterns)) + } + + reviewerBlock := flattened[0]["reviewer"].([]map[string]any) + if len(reviewerBlock) != 1 { + t.Fatalf("Expected 1 reviewer block after round trip, got %d", len(reviewerBlock)) + } + if reviewerBlock[0]["id"] != 12345 { + t.Errorf("Expected reviewer ID to be 12345 after round trip, got %v", reviewerBlock[0]["id"]) + } + if reviewerBlock[0]["type"] != "Team" { + t.Errorf("Expected reviewer type to be Team after round trip, got %v", reviewerBlock[0]["type"]) + } +} From 450853e63e1f453780aa69c7192d15f769fad31a Mon Sep 17 00:00:00 2001 From: Timo Sand Date: Mon, 12 Jan 2026 00:33:51 +0200 Subject: [PATCH 3/6] docs(ruleset): document required_reviewers rule parameter Add documentation for the new required_reviewers nested block in both organization_ruleset and repository_ruleset resources. Documents the reviewer, file_patterns, and minimum_approvals attributes with their types and requirements. --- .../docs/r/organization_ruleset.html.markdown | 16 ++++++++++++++++ website/docs/r/repository_ruleset.html.markdown | 16 ++++++++++++++++ 2 files changed, 32 insertions(+) diff --git a/website/docs/r/organization_ruleset.html.markdown b/website/docs/r/organization_ruleset.html.markdown index ef9e803d9d..06b5667198 100644 --- a/website/docs/r/organization_ruleset.html.markdown +++ b/website/docs/r/organization_ruleset.html.markdown @@ -220,6 +220,22 @@ The `rules` block supports the following: - `review_draft_pull_requests` - (Optional) (Boolean) Copilot automatically reviews draft pull requests before they are marked as ready for review. Defaults to `false`. +* `required_reviewers` - (Optional) (Block List) Require specific reviewers to approve pull requests. Note: This feature is in beta. (see [below for nested schema](#rules.pull_request.required_reviewers)) + +#### rules.pull_request.required_reviewers #### + +* `reviewer` - (Required) (Block List, Max: 1) The reviewer that must review matching files. (see [below for nested schema](#rules.pull_request.required_reviewers.reviewer)) + +* `file_patterns` - (Required) (List of String) File patterns (fnmatch syntax) that this reviewer must approve. + +* `minimum_approvals` - (Required) (Number) Minimum number of approvals required from this reviewer. Set to 0 to make approval optional. + +#### rules.pull_request.required_reviewers.reviewer #### + +* `id` - (Required) (Number) The ID of the reviewer (Team ID). + +* `type` - (Required) (String) The type of reviewer. Currently only `Team` is supported. + #### rules.required_status_checks #### - `required_check` - (Required) (Block Set, Min: 1) Status checks that are required. Several can be defined. (see [below for nested schema](#rulesrequired_status_checks.required_check)) diff --git a/website/docs/r/repository_ruleset.html.markdown b/website/docs/r/repository_ruleset.html.markdown index 264a716fc8..0fbe447d74 100644 --- a/website/docs/r/repository_ruleset.html.markdown +++ b/website/docs/r/repository_ruleset.html.markdown @@ -222,6 +222,22 @@ The `rules` block supports the following: - `review_draft_pull_requests` - (Optional) (Boolean) Copilot automatically reviews draft pull requests before they are marked as ready for review. Defaults to `false`. +* `required_reviewers` - (Optional) (Block List) Require specific reviewers to approve pull requests. Note: This feature is in beta. (see [below for nested schema](#rules.pull_request.required_reviewers)) + +#### rules.pull_request.required_reviewers #### + +* `reviewer` - (Required) (Block List, Max: 1) The reviewer that must review matching files. (see [below for nested schema](#rules.pull_request.required_reviewers.reviewer)) + +* `file_patterns` - (Required) (List of String) File patterns (fnmatch syntax) that this reviewer must approve. + +* `minimum_approvals` - (Required) (Number) Minimum number of approvals required from this reviewer. Set to 0 to make approval optional. + +#### rules.pull_request.required_reviewers.reviewer #### + +* `id` - (Required) (Number) The ID of the reviewer (Team ID). + +* `type` - (Required) (String) The type of reviewer. Currently only `Team` is supported. + #### rules.required_deployments #### - `required_deployment_environments` - (Required) (List of String) The environments that must be successfully deployed to before branches can be merged. From 1272f66979e7224dd5f06ea7d7323c9e86ca767b Mon Sep 17 00:00:00 2001 From: Timo Sand Date: Mon, 12 Jan 2026 22:54:06 +0200 Subject: [PATCH 4/6] docs(ruleset): fix documentation errors in ruleset resources - Add missing allowed_merge_methods field to pull_request section - Fix merge_queue fields from Required to Optional with defaults - Fix actor_id to show Optional with note about DeployKey - Add DeployKey to actor_type enum list - Add protected field to conditions.repository_name - Fix required_code_scanning_tool anchor link - Remove duplicate do_not_enforce_on_create --- .../docs/r/organization_ruleset.html.markdown | 26 ++++++------ .../docs/r/repository_ruleset.html.markdown | 40 ++++++++++--------- 2 files changed, 36 insertions(+), 30 deletions(-) diff --git a/website/docs/r/organization_ruleset.html.markdown b/website/docs/r/organization_ruleset.html.markdown index 06b5667198..5a7e10d506 100644 --- a/website/docs/r/organization_ruleset.html.markdown +++ b/website/docs/r/organization_ruleset.html.markdown @@ -85,15 +85,15 @@ resource "github_organization_ruleset" "example_push" { file_path_restriction { restricted_file_paths = [".github/workflows/*", "*.env"] } - + max_file_size { max_file_size = 100 # 100 MB } - + max_file_path_length { max_file_path_length = 255 } - + file_extension_restriction { restricted_file_extensions = ["*.exe", "*.dll", "*.so"] } @@ -220,25 +220,27 @@ The `rules` block supports the following: - `review_draft_pull_requests` - (Optional) (Boolean) Copilot automatically reviews draft pull requests before they are marked as ready for review. Defaults to `false`. -* `required_reviewers` - (Optional) (Block List) Require specific reviewers to approve pull requests. Note: This feature is in beta. (see [below for nested schema](#rules.pull_request.required_reviewers)) +- `allowed_merge_methods` - (Required) (List of String, Min: 1) Array of merge methods to be allowed. Allowed values include `merge`, `squash`, and `rebase`. At least one must be enabled. + +- `required_reviewers` - (Optional) (Block List) Require specific reviewers to approve pull requests. Note: This feature is in beta. (see [below for nested schema](#rulespull_requestrequired_reviewers)) #### rules.pull_request.required_reviewers #### -* `reviewer` - (Required) (Block List, Max: 1) The reviewer that must review matching files. (see [below for nested schema](#rules.pull_request.required_reviewers.reviewer)) +- `reviewer` - (Required) (Block List, Max: 1) The reviewer that must review matching files. (see [below for nested schema](#rulespull_requestrequired_reviewersreviewer)) -* `file_patterns` - (Required) (List of String) File patterns (fnmatch syntax) that this reviewer must approve. +- `file_patterns` - (Required) (List of String) File patterns (fnmatch syntax) that this reviewer must approve. -* `minimum_approvals` - (Required) (Number) Minimum number of approvals required from this reviewer. Set to 0 to make approval optional. +- `minimum_approvals` - (Required) (Number) Minimum number of approvals required from this reviewer. Set to 0 to make approval optional. #### rules.pull_request.required_reviewers.reviewer #### -* `id` - (Required) (Number) The ID of the reviewer (Team ID). +- `id` - (Required) (Number) The ID of the reviewer (Team ID). -* `type` - (Required) (String) The type of reviewer. Currently only `Team` is supported. +- `type` - (Required) (String) The type of reviewer. Currently only `Team` is supported. #### rules.required_status_checks #### -- `required_check` - (Required) (Block Set, Min: 1) Status checks that are required. Several can be defined. (see [below for nested schema](#rulesrequired_status_checks.required_check)) +- `required_check` - (Required) (Block Set, Min: 1) Status checks that are required. Several can be defined. (see [below for nested schema](#rulesrequired_status_checksrequired_check)) - `strict_required_status_checks_policy` - (Optional) (Boolean) Whether pull requests targeting a matching branch must be tested with the latest code. This setting will not take effect unless at least one status check is enabled. Defaults to `false`. @@ -306,7 +308,7 @@ The `rules` block supports the following: #### bypass_actors #### -- `actor_id` - (Required) (Number) The ID of the actor that can bypass a ruleset. +- `actor_id` - (Optional) (Number) The ID of the actor that can bypass a ruleset. Some actor types such as `DeployKey` do not have an ID. - `actor_type` (String) The type of actor that can bypass a ruleset. Can be one of: `RepositoryRole`, `Team`, `Integration`, `OrganizationAdmin`. @@ -339,8 +341,8 @@ One of `repository_id` and `repository_name` must be set for the rule to target #### conditions.repository_name #### - `exclude` - (Required) (List of String) Array of repository names or patterns to exclude. The condition will not pass if any of these patterns match. - - `include` - (Required) (List of String) Array of repository names or patterns to include. One of these patterns must match for the condition to pass. Also accepts `~ALL` to include all repositories. +- `protected` - (Optional) (Boolean) Whether renaming of target repositories is prevented. Defaults to `false`. ## Attributes Reference diff --git a/website/docs/r/repository_ruleset.html.markdown b/website/docs/r/repository_ruleset.html.markdown index 0fbe447d74..5c8b72ae4e 100644 --- a/website/docs/r/repository_ruleset.html.markdown +++ b/website/docs/r/repository_ruleset.html.markdown @@ -70,15 +70,15 @@ resource "github_repository_ruleset" "example_push" { file_path_restriction { restricted_file_paths = [".github/workflows/*", "*.env"] } - + max_file_size { max_file_size = 100 # 100 MB } - + max_file_path_length { max_file_path_length = 255 } - + file_extension_restriction { restricted_file_extensions = ["*.exe", "*.dll", "*.so"] } @@ -193,19 +193,19 @@ The `rules` block supports the following: #### rules.merge_queue #### -- `check_response_timeout_minutes` - (Required) (Number)Maximum time for a required status check to report a conclusion. After this much time has elapsed, checks that have not reported a conclusion will be assumed to have failed. Defaults to `60`. +- `check_response_timeout_minutes` - (Optional) (Number) Maximum time for a required status check to report a conclusion. After this much time has elapsed, checks that have not reported a conclusion will be assumed to have failed. Defaults to `60`. -- `grouping_strategy` - (Required) (String)When set to ALLGREEN, the merge commit created by merge queue for each PR in the group must pass all required checks to merge. When set to HEADGREEN, only the commit at the head of the merge group, i.e. the commit containing changes from all of the PRs in the group, must pass its required checks to merge. Can be one of: ALLGREEN, HEADGREEN. Defaults to `ALLGREEN`. +- `grouping_strategy` - (Optional) (String) When set to `ALLGREEN`, the merge commit created by merge queue for each PR in the group must pass all required checks to merge. When set to `HEADGREEN`, only the commit at the head of the merge group, i.e. the commit containing changes from all of the PRs in the group, must pass its required checks to merge. Can be one of: `ALLGREEN`, `HEADGREEN`. Defaults to `ALLGREEN`. -- `max_entries_to_build` - (Required) (Number) Limit the number of queued pull requests requesting checks and workflow runs at the same time. Defaults to `5`. +- `max_entries_to_build` - (Optional) (Number) Limit the number of queued pull requests requesting checks and workflow runs at the same time. Defaults to `5`. -- `max_entries_to_merge` - (Required) (Number) Limit the number of queued pull requests that will be merged together in a group. Defaults to `5`. +- `max_entries_to_merge` - (Optional) (Number) Limit the number of queued pull requests that will be merged together in a group. Defaults to `5`. -- `merge_method` - (Required) (String) Method to use when merging changes from queued pull requests. Can be one of: MERGE, SQUASH, REBASE. Defaults to `MERGE`. +- `merge_method` - (Optional) (String) Method to use when merging changes from queued pull requests. Can be one of: `MERGE`, `SQUASH`, `REBASE`. Defaults to `MERGE`. -- `min_entries_to_merge` - (Required) (Number) The minimum number of PRs that will be merged together in a group. Defaults to `1`. +- `min_entries_to_merge` - (Optional) (Number) The minimum number of PRs that will be merged together in a group. Defaults to `1`. -- `min_entries_to_merge_wait_minutes` - (Required) (Number) The time merge queue should wait after the first PR is added to the queue for the minimum group size to be met. After this time has elapsed, the minimum group size will be ignored and a smaller group will be merged. Defaults to `5`. +- `min_entries_to_merge_wait_minutes` - (Optional) (Number) The time merge queue should wait after the first PR is added to the queue for the minimum group size to be met. After this time has elapsed, the minimum group size will be ignored and a smaller group will be merged. Defaults to `5`. #### rules.pull_request #### @@ -222,21 +222,23 @@ The `rules` block supports the following: - `review_draft_pull_requests` - (Optional) (Boolean) Copilot automatically reviews draft pull requests before they are marked as ready for review. Defaults to `false`. -* `required_reviewers` - (Optional) (Block List) Require specific reviewers to approve pull requests. Note: This feature is in beta. (see [below for nested schema](#rules.pull_request.required_reviewers)) +- `allowed_merge_methods` - (Required) (List of String, Min: 1) Array of merge methods to be allowed. Allowed values include `merge`, `squash`, and `rebase`. At least one must be enabled. + +- `required_reviewers` - (Optional) (Block List) Require specific reviewers to approve pull requests. Note: This feature is in beta. (see [below for nested schema](#rulespull_requestrequired_reviewers)) #### rules.pull_request.required_reviewers #### -* `reviewer` - (Required) (Block List, Max: 1) The reviewer that must review matching files. (see [below for nested schema](#rules.pull_request.required_reviewers.reviewer)) +- `reviewer` - (Required) (Block List, Max: 1) The reviewer that must review matching files. (see [below for nested schema](#rulespull_requestrequired_reviewersreviewer)) -* `file_patterns` - (Required) (List of String) File patterns (fnmatch syntax) that this reviewer must approve. +- `file_patterns` - (Required) (List of String) File patterns (fnmatch syntax) that this reviewer must approve. -* `minimum_approvals` - (Required) (Number) Minimum number of approvals required from this reviewer. Set to 0 to make approval optional. +- `minimum_approvals` - (Required) (Number) Minimum number of approvals required from this reviewer. Set to 0 to make approval optional. #### rules.pull_request.required_reviewers.reviewer #### -* `id` - (Required) (Number) The ID of the reviewer (Team ID). +- `id` - (Required) (Number) The ID of the reviewer (Team ID). -* `type` - (Required) (String) The type of reviewer. Currently only `Team` is supported. +- `type` - (Required) (String) The type of reviewer. Currently only `Team` is supported. #### rules.required_deployments #### @@ -296,7 +298,7 @@ The `rules` block supports the following: #### bypass_actors #### -- `actor_id` - (Number) The ID of the actor that can bypass a ruleset. If `actor_type` is `Integration`, `actor_id` is a GitHub App ID. App ID can be obtained by following instructions from the [Get an App API docs](https://docs.github.com/en/rest/apps/apps?apiVersion=2022-11-28#get-an-app) +- `actor_id` - (Optional) (Number) The ID of the actor that can bypass a ruleset. If `actor_type` is `Integration`, `actor_id` is a GitHub App ID. App ID can be obtained by following instructions from the [Get an App API docs](https://docs.github.com/en/rest/apps/apps?apiVersion=2022-11-28#get-an-app). Some actor types such as `DeployKey` do not have an ID. - `actor_type` (String) The type of actor that can bypass a ruleset. Can be one of: `RepositoryRole`, `Team`, `Integration`, `OrganizationAdmin`, `DeployKey`. @@ -312,7 +314,9 @@ The `rules` block supports the following: #### conditions #### -- `ref_name` - (Required) (Block List, Min: 1, Max: 1) (see [below for nested schema](#conditionsref_name)) +- `ref_name` - (Optional) (Block List, Max: 1) Required for `branch` and `tag` targets. Must NOT be set for `push` targets. (see [below for nested schema](#conditionsref_name)) + +~> **Note:** For `push` targets, do not include `ref_name` in conditions. Push rulesets operate on file content, not on refs. The `conditions` block is optional for push targets. #### conditions.ref_name #### From f3607cc1b423863cf99956ab2d1383b330023be1 Mon Sep 17 00:00:00 2001 From: Timo Sand Date: Wed, 4 Feb 2026 12:53:18 +0200 Subject: [PATCH 5/6] Refactor tests to adhere to convention Signed-off-by: Timo Sand --- ...source_github_organization_ruleset_test.go | 360 ++++++++---------- 1 file changed, 159 insertions(+), 201 deletions(-) diff --git a/github/resource_github_organization_ruleset_test.go b/github/resource_github_organization_ruleset_test.go index f0a20e8d61..539a2511cf 100644 --- a/github/resource_github_organization_ruleset_test.go +++ b/github/resource_github_organization_ruleset_test.go @@ -809,6 +809,165 @@ resource "github_organization_ruleset" "test" { }) }) }) + + t.Run("updates_required_reviewers", func(t *testing.T) { + randomID := acctest.RandStringFromCharSet(5, acctest.CharSetAlphaNum) + teamName := fmt.Sprintf("%steam-req-rev-%s", testResourcePrefix, randomID) + rulesetName := fmt.Sprintf("%s-ruleset-req-rev-%s", testResourcePrefix, randomID) + + config := ` +resource "github_team" "test" { + name = "%s" +} + +resource "github_organization_ruleset" "test" { + name = "%s" + target = "branch" + enforcement = "active" + + conditions { + repository_name { + include = ["~ALL"] + exclude = [] + } + + ref_name { + include = ["~ALL"] + exclude = [] + } + } + + rules { + pull_request { + allowed_merge_methods = ["merge", "squash"] + required_approving_review_count = 1 + + required_reviewers { + reviewer { + id = github_team.test.id + type = "Team" + } + file_patterns = ["*.go", "src/**/*.ts"] + minimum_approvals = %d + } + } + } +} +` + + resource.Test(t, resource.TestCase{ + PreCheck: func() { skipUnlessHasPaidOrgs(t) }, + ProviderFactories: providerFactories, + Steps: []resource.TestStep{ + { + Config: fmt.Sprintf(config, teamName, rulesetName, 1), + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr("github_organization_ruleset.test", "name", rulesetName), + resource.TestCheckResourceAttr("github_organization_ruleset.test", "target", "branch"), + resource.TestCheckResourceAttr("github_organization_ruleset.test", "enforcement", "active"), + resource.TestCheckResourceAttr("github_organization_ruleset.test", "rules.0.pull_request.0.required_reviewers.#", "1"), + resource.TestCheckResourceAttr("github_organization_ruleset.test", "rules.0.pull_request.0.required_reviewers.0.minimum_approvals", "1"), + resource.TestCheckResourceAttr("github_organization_ruleset.test", "rules.0.pull_request.0.required_reviewers.0.file_patterns.#", "2"), + resource.TestCheckResourceAttr("github_organization_ruleset.test", "rules.0.pull_request.0.required_reviewers.0.file_patterns.0", "*.go"), + resource.TestCheckResourceAttr("github_organization_ruleset.test", "rules.0.pull_request.0.required_reviewers.0.file_patterns.1", "src/**/*.ts"), + resource.TestCheckResourceAttr("github_organization_ruleset.test", "rules.0.pull_request.0.required_reviewers.0.reviewer.0.type", "Team"), + ), + }, + { + Config: fmt.Sprintf(config, teamName, rulesetName, 2), + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr("github_organization_ruleset.test", "rules.0.pull_request.0.required_reviewers.0.minimum_approvals", "2"), + ), + }, + { + ResourceName: "github_organization_ruleset.test", + ImportState: true, + ImportStateVerify: true, + ImportStateVerifyIgnore: []string{"etag"}, + }, + }, + }) + }) + t.Run("creates_rule_with_multiple_required_reviewers", func(t *testing.T) { + randomID := acctest.RandStringFromCharSet(5, acctest.CharSetAlphaNum) + teamName1 := fmt.Sprintf("%steam-req-rev-1-%s", testResourcePrefix, randomID) + teamName2 := fmt.Sprintf("%steam-req-rev-2-%s", testResourcePrefix, randomID) + rulesetName := fmt.Sprintf("%s-ruleset-multi-rev-%s", testResourcePrefix, randomID) + + config := fmt.Sprintf(` +resource "github_team" "test1" { + name = "%s" +} + +resource "github_team" "test2" { + name = "%s" +} + +resource "github_organization_ruleset" "test" { + name = "%s" + target = "branch" + enforcement = "active" + + conditions { + repository_name { + include = ["~ALL"] + exclude = [] + } + + ref_name { + include = ["~ALL"] + exclude = [] + } + } + + rules { + pull_request { + allowed_merge_methods = ["merge", "squash"] + required_approving_review_count = 1 + + required_reviewers { + reviewer { + id = github_team.test1.id + type = "Team" + } + file_patterns = ["*.go"] + minimum_approvals = 1 + } + + required_reviewers { + reviewer { + id = github_team.test2.id + type = "Team" + } + file_patterns = ["*.md", "docs/**/*"] + minimum_approvals = 1 + } + } + } +} +`, teamName1, teamName2, rulesetName) + + resource.Test(t, resource.TestCase{ + PreCheck: func() { skipUnlessHasPaidOrgs(t) }, + ProviderFactories: providerFactories, + Steps: []resource.TestStep{ + { + Config: config, + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr("github_organization_ruleset.test", "name", rulesetName), + resource.TestCheckResourceAttr("github_organization_ruleset.test", "target", "branch"), + resource.TestCheckResourceAttr("github_organization_ruleset.test", "enforcement", "active"), + resource.TestCheckResourceAttr("github_organization_ruleset.test", "rules.0.pull_request.0.required_reviewers.#", "2"), + resource.TestCheckResourceAttr("github_organization_ruleset.test", "rules.0.pull_request.0.required_reviewers.0.minimum_approvals", "1"), + resource.TestCheckResourceAttr("github_organization_ruleset.test", "rules.0.pull_request.0.required_reviewers.0.file_patterns.#", "1"), + resource.TestCheckResourceAttr("github_organization_ruleset.test", "rules.0.pull_request.0.required_reviewers.0.file_patterns.0", "*.go"), + resource.TestCheckResourceAttr("github_organization_ruleset.test", "rules.0.pull_request.0.required_reviewers.1.minimum_approvals", "1"), + resource.TestCheckResourceAttr("github_organization_ruleset.test", "rules.0.pull_request.0.required_reviewers.1.file_patterns.#", "2"), + ), + }, + }, + }) + }) } func TestOrganizationPushRulesetSupport(t *testing.T) { @@ -934,204 +1093,3 @@ func TestOrganizationPushRulesetSupport(t *testing.T) { t.Errorf("Expected 4 restricted file extensions, got %d", len(restrictedExts)) } } - -func TestAccGithubOrganizationRuleset_requiredReviewers(t *testing.T) { - randomID := acctest.RandStringFromCharSet(5, acctest.CharSetAlphaNum) - teamName := fmt.Sprintf("%steam-req-rev-%s", testResourcePrefix, randomID) - rulesetName := fmt.Sprintf("%s-ruleset-req-rev-%s", testResourcePrefix, randomID) - - config := fmt.Sprintf(` -resource "github_team" "test" { - name = "%s" -} - -resource "github_organization_ruleset" "test" { - name = "%s" - target = "branch" - enforcement = "active" - - conditions { - repository_name { - include = ["~ALL"] - exclude = [] - } - - ref_name { - include = ["~ALL"] - exclude = [] - } - } - - rules { - pull_request { - allowed_merge_methods = ["merge", "squash"] - required_approving_review_count = 1 - - required_reviewers { - reviewer { - id = github_team.test.id - type = "Team" - } - file_patterns = ["*.go", "src/**/*.ts"] - minimum_approvals = 1 - } - } - } -} -`, teamName, rulesetName) - - // Updated config: change minimum_approvals from 1 to 2 - configUpdated := fmt.Sprintf(` -resource "github_team" "test" { - name = "%s" -} - -resource "github_organization_ruleset" "test" { - name = "%s" - target = "branch" - enforcement = "active" - - conditions { - repository_name { - include = ["~ALL"] - exclude = [] - } - - ref_name { - include = ["~ALL"] - exclude = [] - } - } - - rules { - pull_request { - allowed_merge_methods = ["merge", "squash"] - required_approving_review_count = 1 - - required_reviewers { - reviewer { - id = github_team.test.id - type = "Team" - } - file_patterns = ["*.go", "src/**/*.ts"] - minimum_approvals = 2 - } - } - } -} -`, teamName, rulesetName) - - resource.Test(t, resource.TestCase{ - PreCheck: func() { skipUnlessHasPaidOrgs(t) }, - ProviderFactories: providerFactories, - Steps: []resource.TestStep{ - { - Config: config, - Check: resource.ComposeTestCheckFunc( - resource.TestCheckResourceAttr("github_organization_ruleset.test", "name", rulesetName), - resource.TestCheckResourceAttr("github_organization_ruleset.test", "target", "branch"), - resource.TestCheckResourceAttr("github_organization_ruleset.test", "enforcement", "active"), - resource.TestCheckResourceAttr("github_organization_ruleset.test", "rules.0.pull_request.0.required_reviewers.#", "1"), - resource.TestCheckResourceAttr("github_organization_ruleset.test", "rules.0.pull_request.0.required_reviewers.0.minimum_approvals", "1"), - resource.TestCheckResourceAttr("github_organization_ruleset.test", "rules.0.pull_request.0.required_reviewers.0.file_patterns.#", "2"), - resource.TestCheckResourceAttr("github_organization_ruleset.test", "rules.0.pull_request.0.required_reviewers.0.file_patterns.0", "*.go"), - resource.TestCheckResourceAttr("github_organization_ruleset.test", "rules.0.pull_request.0.required_reviewers.0.file_patterns.1", "src/**/*.ts"), - resource.TestCheckResourceAttr("github_organization_ruleset.test", "rules.0.pull_request.0.required_reviewers.0.reviewer.0.type", "Team"), - ), - }, - { - Config: configUpdated, - Check: resource.ComposeTestCheckFunc( - resource.TestCheckResourceAttr("github_organization_ruleset.test", "rules.0.pull_request.0.required_reviewers.0.minimum_approvals", "2"), - ), - }, - { - ResourceName: "github_organization_ruleset.test", - ImportState: true, - ImportStateVerify: true, - ImportStateVerifyIgnore: []string{"etag"}, - }, - }, - }) -} - -func TestAccGithubOrganizationRuleset_requiredReviewersMultiple(t *testing.T) { - randomID := acctest.RandStringFromCharSet(5, acctest.CharSetAlphaNum) - teamName1 := fmt.Sprintf("%steam-req-rev-1-%s", testResourcePrefix, randomID) - teamName2 := fmt.Sprintf("%steam-req-rev-2-%s", testResourcePrefix, randomID) - rulesetName := fmt.Sprintf("%s-ruleset-multi-rev-%s", testResourcePrefix, randomID) - - config := fmt.Sprintf(` -resource "github_team" "test1" { - name = "%s" -} - -resource "github_team" "test2" { - name = "%s" -} - -resource "github_organization_ruleset" "test" { - name = "%s" - target = "branch" - enforcement = "active" - - conditions { - repository_name { - include = ["~ALL"] - exclude = [] - } - - ref_name { - include = ["~ALL"] - exclude = [] - } - } - - rules { - pull_request { - allowed_merge_methods = ["merge", "squash"] - required_approving_review_count = 1 - - required_reviewers { - reviewer { - id = github_team.test1.id - type = "Team" - } - file_patterns = ["*.go"] - minimum_approvals = 1 - } - - required_reviewers { - reviewer { - id = github_team.test2.id - type = "Team" - } - file_patterns = ["*.md", "docs/**/*"] - minimum_approvals = 1 - } - } - } -} -`, teamName1, teamName2, rulesetName) - - resource.Test(t, resource.TestCase{ - PreCheck: func() { skipUnlessHasPaidOrgs(t) }, - ProviderFactories: providerFactories, - Steps: []resource.TestStep{ - { - Config: config, - Check: resource.ComposeTestCheckFunc( - resource.TestCheckResourceAttr("github_organization_ruleset.test", "name", rulesetName), - resource.TestCheckResourceAttr("github_organization_ruleset.test", "target", "branch"), - resource.TestCheckResourceAttr("github_organization_ruleset.test", "enforcement", "active"), - resource.TestCheckResourceAttr("github_organization_ruleset.test", "rules.0.pull_request.0.required_reviewers.#", "2"), - resource.TestCheckResourceAttr("github_organization_ruleset.test", "rules.0.pull_request.0.required_reviewers.0.minimum_approvals", "1"), - resource.TestCheckResourceAttr("github_organization_ruleset.test", "rules.0.pull_request.0.required_reviewers.0.file_patterns.#", "1"), - resource.TestCheckResourceAttr("github_organization_ruleset.test", "rules.0.pull_request.0.required_reviewers.0.file_patterns.0", "*.go"), - resource.TestCheckResourceAttr("github_organization_ruleset.test", "rules.0.pull_request.0.required_reviewers.1.minimum_approvals", "1"), - resource.TestCheckResourceAttr("github_organization_ruleset.test", "rules.0.pull_request.0.required_reviewers.1.file_patterns.#", "2"), - ), - }, - }, - }) -} From 374aae80bb802307af39264e714cd668a7d5ef24 Mon Sep 17 00:00:00 2001 From: Timo Sand Date: Wed, 4 Feb 2026 12:58:21 +0200 Subject: [PATCH 6/6] Remove DRY schema Signed-off-by: Timo Sand --- .../resource_github_organization_ruleset.go | 43 ++++++++++++++++- github/resource_github_repository_ruleset.go | 43 ++++++++++++++++- github/util_rules.go | 46 ------------------- 3 files changed, 84 insertions(+), 48 deletions(-) diff --git a/github/resource_github_organization_ruleset.go b/github/resource_github_organization_ruleset.go index d9af2227e8..8624250883 100644 --- a/github/resource_github_organization_ruleset.go +++ b/github/resource_github_organization_ruleset.go @@ -249,7 +249,48 @@ func resourceGithubOrganizationRuleset() *schema.Resource { Default: false, Description: "All conversations on code must be resolved before a pull request can be merged. Defaults to `false`.", }, - "required_reviewers": requiredReviewersSchema(), + "required_reviewers": { + Type: schema.TypeList, + Optional: true, + Description: "Require specific reviewers to approve pull requests targeting matching branches. Note: This feature is in beta and subject to change.", + Elem: &schema.Resource{ + Schema: map[string]*schema.Schema{ + "reviewer": { + Type: schema.TypeList, + Required: true, + MaxItems: 1, + Description: "The reviewer that must review matching files.", + Elem: &schema.Resource{ + Schema: map[string]*schema.Schema{ + "id": { + Type: schema.TypeInt, + Required: true, + Description: "The ID of the reviewer that must review.", + }, + "type": { + Type: schema.TypeString, + Required: true, + ValidateDiagFunc: toDiagFunc(validation.StringInSlice([]string{"Team"}, false), "type"), + Description: "The type of reviewer. Currently only `Team` is supported.", + }, + }, + }, + }, + "file_patterns": { + Type: schema.TypeList, + Required: true, + MinItems: 1, + Description: "File patterns (fnmatch syntax) that this reviewer must approve.", + Elem: &schema.Schema{Type: schema.TypeString}, + }, + "minimum_approvals": { + Type: schema.TypeInt, + Required: true, + Description: "Minimum number of approvals required from this reviewer. Set to 0 to make approval optional.", + }, + }, + }, + }, }, }, }, diff --git a/github/resource_github_repository_ruleset.go b/github/resource_github_repository_ruleset.go index b3344df8b2..460473e857 100644 --- a/github/resource_github_repository_ruleset.go +++ b/github/resource_github_repository_ruleset.go @@ -236,7 +236,48 @@ func resourceGithubRepositoryRuleset() *schema.Resource { Default: false, Description: "All conversations on code must be resolved before a pull request can be merged. Defaults to `false`.", }, - "required_reviewers": requiredReviewersSchema(), + "required_reviewers": { + Type: schema.TypeList, + Optional: true, + Description: "Require specific reviewers to approve pull requests targeting matching branches. Note: This feature is in beta and subject to change.", + Elem: &schema.Resource{ + Schema: map[string]*schema.Schema{ + "reviewer": { + Type: schema.TypeList, + Required: true, + MaxItems: 1, + Description: "The reviewer that must review matching files.", + Elem: &schema.Resource{ + Schema: map[string]*schema.Schema{ + "id": { + Type: schema.TypeInt, + Required: true, + Description: "The ID of the reviewer that must review.", + }, + "type": { + Type: schema.TypeString, + Required: true, + ValidateDiagFunc: toDiagFunc(validation.StringInSlice([]string{"Team"}, false), "type"), + Description: "The type of reviewer. Currently only `Team` is supported.", + }, + }, + }, + }, + "file_patterns": { + Type: schema.TypeList, + Required: true, + MinItems: 1, + Description: "File patterns (fnmatch syntax) that this reviewer must approve.", + Elem: &schema.Schema{Type: schema.TypeString}, + }, + "minimum_approvals": { + Type: schema.TypeInt, + Required: true, + Description: "Minimum number of approvals required from this reviewer. Set to 0 to make approval optional.", + }, + }, + }, + }, }, }, }, diff --git a/github/util_rules.go b/github/util_rules.go index 25538f56e5..7e4418cb6e 100644 --- a/github/util_rules.go +++ b/github/util_rules.go @@ -8,54 +8,8 @@ import ( "github.com/google/go-github/v82/github" "github.com/hashicorp/terraform-plugin-log/tflog" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" - "github.com/hashicorp/terraform-plugin-sdk/v2/helper/validation" ) -func requiredReviewersSchema() *schema.Schema { - return &schema.Schema{ - Type: schema.TypeList, - Optional: true, - Description: "Require specific reviewers to approve pull requests targeting matching branches. Note: This feature is in beta and subject to change.", - Elem: &schema.Resource{ - Schema: map[string]*schema.Schema{ - "reviewer": { - Type: schema.TypeList, - Required: true, - MaxItems: 1, - Description: "The reviewer that must review matching files.", - Elem: &schema.Resource{ - Schema: map[string]*schema.Schema{ - "id": { - Type: schema.TypeInt, - Required: true, - Description: "The ID of the reviewer that must review.", - }, - "type": { - Type: schema.TypeString, - Required: true, - ValidateDiagFunc: toDiagFunc(validation.StringInSlice([]string{"Team"}, false), "type"), - Description: "The type of reviewer. Currently only `Team` is supported.", - }, - }, - }, - }, - "file_patterns": { - Type: schema.TypeList, - Required: true, - MinItems: 1, - Description: "File patterns (fnmatch syntax) that this reviewer must approve.", - Elem: &schema.Schema{Type: schema.TypeString}, - }, - "minimum_approvals": { - Type: schema.TypeInt, - Required: true, - Description: "Minimum number of approvals required from this reviewer. Set to 0 to make approval optional.", - }, - }, - }, - } -} - // Helper function to safely convert interface{} to int, handling both int and float64. func toInt(v any) int { switch val := v.(type) {