Skip to content

Commit 2182312

Browse files
deigadeiga
authored
[FEAT] Add ruleset rule for pull request required reviewers (#3073)
* 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 * 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 * 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(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 * Refactor tests to adhere to convention Signed-off-by: Timo Sand <[email protected]> * Remove DRY schema Signed-off-by: Timo Sand <[email protected]> --------- Signed-off-by: Timo Sand <[email protected]>
1 parent 41c3ace commit 2182312

8 files changed

Lines changed: 714 additions & 18 deletions

‎github/resource_github_organization_ruleset.go‎

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -249,6 +249,48 @@ func resourceGithubOrganizationRuleset() *schema.Resource {
249249
Default: false,
250250
Description: "All conversations on code must be resolved before a pull request can be merged. Defaults to `false`.",
251251
},
252+
"required_reviewers": {
253+
Type: schema.TypeList,
254+
Optional: true,
255+
Description: "Require specific reviewers to approve pull requests targeting matching branches. Note: This feature is in beta and subject to change.",
256+
Elem: &schema.Resource{
257+
Schema: map[string]*schema.Schema{
258+
"reviewer": {
259+
Type: schema.TypeList,
260+
Required: true,
261+
MaxItems: 1,
262+
Description: "The reviewer that must review matching files.",
263+
Elem: &schema.Resource{
264+
Schema: map[string]*schema.Schema{
265+
"id": {
266+
Type: schema.TypeInt,
267+
Required: true,
268+
Description: "The ID of the reviewer that must review.",
269+
},
270+
"type": {
271+
Type: schema.TypeString,
272+
Required: true,
273+
ValidateDiagFunc: toDiagFunc(validation.StringInSlice([]string{"Team"}, false), "type"),
274+
Description: "The type of reviewer. Currently only `Team` is supported.",
275+
},
276+
},
277+
},
278+
},
279+
"file_patterns": {
280+
Type: schema.TypeList,
281+
Required: true,
282+
MinItems: 1,
283+
Description: "File patterns (fnmatch syntax) that this reviewer must approve.",
284+
Elem: &schema.Schema{Type: schema.TypeString},
285+
},
286+
"minimum_approvals": {
287+
Type: schema.TypeInt,
288+
Required: true,
289+
Description: "Minimum number of approvals required from this reviewer. Set to 0 to make approval optional.",
290+
},
291+
},
292+
},
293+
},
252294
},
253295
},
254296
},

‎github/resource_github_organization_ruleset_test.go‎

Lines changed: 159 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -809,6 +809,165 @@ resource "github_organization_ruleset" "test" {
809809
})
810810
})
811811
})
812+
813+
t.Run("updates_required_reviewers", func(t *testing.T) {
814+
randomID := acctest.RandStringFromCharSet(5, acctest.CharSetAlphaNum)
815+
teamName := fmt.Sprintf("%steam-req-rev-%s", testResourcePrefix, randomID)
816+
rulesetName := fmt.Sprintf("%s-ruleset-req-rev-%s", testResourcePrefix, randomID)
817+
818+
config := `
819+
resource "github_team" "test" {
820+
name = "%s"
821+
}
822+
823+
resource "github_organization_ruleset" "test" {
824+
name = "%s"
825+
target = "branch"
826+
enforcement = "active"
827+
828+
conditions {
829+
repository_name {
830+
include = ["~ALL"]
831+
exclude = []
832+
}
833+
834+
ref_name {
835+
include = ["~ALL"]
836+
exclude = []
837+
}
838+
}
839+
840+
rules {
841+
pull_request {
842+
allowed_merge_methods = ["merge", "squash"]
843+
required_approving_review_count = 1
844+
845+
required_reviewers {
846+
reviewer {
847+
id = github_team.test.id
848+
type = "Team"
849+
}
850+
file_patterns = ["*.go", "src/**/*.ts"]
851+
minimum_approvals = %d
852+
}
853+
}
854+
}
855+
}
856+
`
857+
858+
resource.Test(t, resource.TestCase{
859+
PreCheck: func() { skipUnlessHasPaidOrgs(t) },
860+
ProviderFactories: providerFactories,
861+
Steps: []resource.TestStep{
862+
{
863+
Config: fmt.Sprintf(config, teamName, rulesetName, 1),
864+
Check: resource.ComposeTestCheckFunc(
865+
resource.TestCheckResourceAttr("github_organization_ruleset.test", "name", rulesetName),
866+
resource.TestCheckResourceAttr("github_organization_ruleset.test", "target", "branch"),
867+
resource.TestCheckResourceAttr("github_organization_ruleset.test", "enforcement", "active"),
868+
resource.TestCheckResourceAttr("github_organization_ruleset.test", "rules.0.pull_request.0.required_reviewers.#", "1"),
869+
resource.TestCheckResourceAttr("github_organization_ruleset.test", "rules.0.pull_request.0.required_reviewers.0.minimum_approvals", "1"),
870+
resource.TestCheckResourceAttr("github_organization_ruleset.test", "rules.0.pull_request.0.required_reviewers.0.file_patterns.#", "2"),
871+
resource.TestCheckResourceAttr("github_organization_ruleset.test", "rules.0.pull_request.0.required_reviewers.0.file_patterns.0", "*.go"),
872+
resource.TestCheckResourceAttr("github_organization_ruleset.test", "rules.0.pull_request.0.required_reviewers.0.file_patterns.1", "src/**/*.ts"),
873+
resource.TestCheckResourceAttr("github_organization_ruleset.test", "rules.0.pull_request.0.required_reviewers.0.reviewer.0.type", "Team"),
874+
),
875+
},
876+
{
877+
Config: fmt.Sprintf(config, teamName, rulesetName, 2),
878+
Check: resource.ComposeTestCheckFunc(
879+
resource.TestCheckResourceAttr("github_organization_ruleset.test", "rules.0.pull_request.0.required_reviewers.0.minimum_approvals", "2"),
880+
),
881+
},
882+
{
883+
ResourceName: "github_organization_ruleset.test",
884+
ImportState: true,
885+
ImportStateVerify: true,
886+
ImportStateVerifyIgnore: []string{"etag"},
887+
},
888+
},
889+
})
890+
})
891+
t.Run("creates_rule_with_multiple_required_reviewers", func(t *testing.T) {
892+
randomID := acctest.RandStringFromCharSet(5, acctest.CharSetAlphaNum)
893+
teamName1 := fmt.Sprintf("%steam-req-rev-1-%s", testResourcePrefix, randomID)
894+
teamName2 := fmt.Sprintf("%steam-req-rev-2-%s", testResourcePrefix, randomID)
895+
rulesetName := fmt.Sprintf("%s-ruleset-multi-rev-%s", testResourcePrefix, randomID)
896+
897+
config := fmt.Sprintf(`
898+
resource "github_team" "test1" {
899+
name = "%s"
900+
}
901+
902+
resource "github_team" "test2" {
903+
name = "%s"
904+
}
905+
906+
resource "github_organization_ruleset" "test" {
907+
name = "%s"
908+
target = "branch"
909+
enforcement = "active"
910+
911+
conditions {
912+
repository_name {
913+
include = ["~ALL"]
914+
exclude = []
915+
}
916+
917+
ref_name {
918+
include = ["~ALL"]
919+
exclude = []
920+
}
921+
}
922+
923+
rules {
924+
pull_request {
925+
allowed_merge_methods = ["merge", "squash"]
926+
required_approving_review_count = 1
927+
928+
required_reviewers {
929+
reviewer {
930+
id = github_team.test1.id
931+
type = "Team"
932+
}
933+
file_patterns = ["*.go"]
934+
minimum_approvals = 1
935+
}
936+
937+
required_reviewers {
938+
reviewer {
939+
id = github_team.test2.id
940+
type = "Team"
941+
}
942+
file_patterns = ["*.md", "docs/**/*"]
943+
minimum_approvals = 1
944+
}
945+
}
946+
}
947+
}
948+
`, teamName1, teamName2, rulesetName)
949+
950+
resource.Test(t, resource.TestCase{
951+
PreCheck: func() { skipUnlessHasPaidOrgs(t) },
952+
ProviderFactories: providerFactories,
953+
Steps: []resource.TestStep{
954+
{
955+
Config: config,
956+
Check: resource.ComposeTestCheckFunc(
957+
resource.TestCheckResourceAttr("github_organization_ruleset.test", "name", rulesetName),
958+
resource.TestCheckResourceAttr("github_organization_ruleset.test", "target", "branch"),
959+
resource.TestCheckResourceAttr("github_organization_ruleset.test", "enforcement", "active"),
960+
resource.TestCheckResourceAttr("github_organization_ruleset.test", "rules.0.pull_request.0.required_reviewers.#", "2"),
961+
resource.TestCheckResourceAttr("github_organization_ruleset.test", "rules.0.pull_request.0.required_reviewers.0.minimum_approvals", "1"),
962+
resource.TestCheckResourceAttr("github_organization_ruleset.test", "rules.0.pull_request.0.required_reviewers.0.file_patterns.#", "1"),
963+
resource.TestCheckResourceAttr("github_organization_ruleset.test", "rules.0.pull_request.0.required_reviewers.0.file_patterns.0", "*.go"),
964+
resource.TestCheckResourceAttr("github_organization_ruleset.test", "rules.0.pull_request.0.required_reviewers.1.minimum_approvals", "1"),
965+
resource.TestCheckResourceAttr("github_organization_ruleset.test", "rules.0.pull_request.0.required_reviewers.1.file_patterns.#", "2"),
966+
),
967+
},
968+
},
969+
})
970+
})
812971
}
813972

814973
func TestOrganizationPushRulesetSupport(t *testing.T) {

‎github/resource_github_repository_ruleset.go‎

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -236,6 +236,48 @@ func resourceGithubRepositoryRuleset() *schema.Resource {
236236
Default: false,
237237
Description: "All conversations on code must be resolved before a pull request can be merged. Defaults to `false`.",
238238
},
239+
"required_reviewers": {
240+
Type: schema.TypeList,
241+
Optional: true,
242+
Description: "Require specific reviewers to approve pull requests targeting matching branches. Note: This feature is in beta and subject to change.",
243+
Elem: &schema.Resource{
244+
Schema: map[string]*schema.Schema{
245+
"reviewer": {
246+
Type: schema.TypeList,
247+
Required: true,
248+
MaxItems: 1,
249+
Description: "The reviewer that must review matching files.",
250+
Elem: &schema.Resource{
251+
Schema: map[string]*schema.Schema{
252+
"id": {
253+
Type: schema.TypeInt,
254+
Required: true,
255+
Description: "The ID of the reviewer that must review.",
256+
},
257+
"type": {
258+
Type: schema.TypeString,
259+
Required: true,
260+
ValidateDiagFunc: toDiagFunc(validation.StringInSlice([]string{"Team"}, false), "type"),
261+
Description: "The type of reviewer. Currently only `Team` is supported.",
262+
},
263+
},
264+
},
265+
},
266+
"file_patterns": {
267+
Type: schema.TypeList,
268+
Required: true,
269+
MinItems: 1,
270+
Description: "File patterns (fnmatch syntax) that this reviewer must approve.",
271+
Elem: &schema.Schema{Type: schema.TypeString},
272+
},
273+
"minimum_approvals": {
274+
Type: schema.TypeInt,
275+
Required: true,
276+
Description: "Minimum number of approvals required from this reviewer. Set to 0 to make approval optional.",
277+
},
278+
},
279+
},
280+
},
239281
},
240282
},
241283
},

0 commit comments

Comments
 (0)