Skip to content

Commit 11d233c

Browse files
deigadeiga
authored
[MAINT] Fix github_organization_ruleset and github_repository_ruleset with push target (#2958)
* Update descriptions and validations Signed-off-by: Timo Sand <[email protected]> * Add `CustomizeDiff` logic to validate on `plan` Signed-off-by: Timo Sand <[email protected]> * Add first validation test Signed-off-by: Timo Sand <[email protected]> * Add validation error when `conditions` is missing Signed-off-by: Timo Sand <[email protected]> * Add further validation tests Signed-off-by: Timo Sand <[email protected]> * Remove unnecessary skip blocks as `individual` and `anonymous` access to org rulesets will never be a thing Signed-off-by: Timo Sand <[email protected]> * Switch `ref_name` to `Optional` as `push` doesn't need a `ref_name` Signed-off-by: Timo Sand <[email protected]> * Add Debug logging with `tflog` to validation Signed-off-by: Timo Sand <[email protected]> * Fix validation as `ref_name`, `repository_name` and `repository_id` are empty lists by default Signed-off-by: Timo Sand <[email protected]> * Remove unnecessary panic test Signed-off-by: Timo Sand <[email protected]> * Improve validation output messages Signed-off-by: Timo Sand <[email protected]> * Fix condition to require only one of `repository_name` or `repository_id` Signed-off-by: Timo Sand <[email protected]> * Add validation to `required_workflow.path` Signed-off-by: Timo Sand <[email protected]> * Rename test resources for easier debugging Signed-off-by: Timo Sand <[email protected]> * Fix linter issues Signed-off-by: Timo Sand <[email protected]> * Add validation to ensure `rules.required_status_checks.required_checks.context` is not empty Signed-off-by: Timo Sand <[email protected]> * Add test to ensure that `required_checks` is always required Signed-off-by: Timo Sand <[email protected]> * Fix tests after rebase Signed-off-by: Timo Sand <[email protected]> * Improve legibility of `conditions` description Signed-off-by: Timo Sand <[email protected]> * Update descriptions Signed-off-by: Timo Sand <[email protected]> * Add Acc test for push ruleset. This turns out to be failing as there is a bug in our implementation! Unit tests and fix coming up Signed-off-by: Timo Sand <[email protected]> * Add failing test for `flattenConditions` with no `ref_name` condition Signed-off-by: Timo Sand <[email protected]> * Fix `flattenConditions` to work with `push` rulesets Signed-off-by: Timo Sand <[email protected]> * Add more tests for `flattenConditions` Signed-off-by: Timo Sand <[email protected]> * Enable debug logging in `flattenConditions` Signed-off-by: Timo Sand <[email protected]> * Ensures that `flattenConditions` returns an empty list on empty API response Signed-off-by: Timo Sand <[email protected]> * Add validation for `push` `rules` As they differ from `branch` and `tag` rules Signed-off-by: Timo Sand <[email protected]> * Get `TestAccGithubRepositoryRulesets` to work Signed-off-by: Timo Sand <[email protected]> * `repository_ruleset`: Add tests for validations Signed-off-by: Timo Sand <[email protected]> * `repository_ruleset`: Implement validations for `target`, `conditions` and `rules` Signed-off-by: Timo Sand <[email protected]> * Updated ruleset docs Signed-off-by: Timo Sand <[email protected]> * Extract validation functions to separate utils file with unit tests Signed-off-by: Timo Sand <[email protected]> * Fix push ruleset test config Signed-off-by: Timo Sand <[email protected]> * Remove `repository` target after thorough testing that it doesn't do anything Signed-off-by: Timo Sand <[email protected]> * Use idiomatic test naming convention Signed-off-by: Timo Sand <[email protected]> * Address code structure comment in tests Signed-off-by: Timo Sand <[email protected]> * Fix inconsistent logging Signed-off-by: Timo Sand <[email protected]> * Refactor to use typed constant string for ruleset `Target` Signed-off-by: Timo Sand <[email protected]> * Fix indentation issue with `github_repository_file` and heredocs Signed-off-by: Timo Sand <[email protected]> * Rename files to be more sensible Signed-off-by: Timo Sand <[email protected]> * Refactor validation functions so that Repo and Org share almost everything Signed-off-by: Timo Sand <[email protected]> * Address review comments Signed-off-by: Timo Sand <[email protected]> * Use `github.RepositoryRuleType` instead of strings Signed-off-by: Timo Sand <[email protected]> * Refactor `flattenRules` and `expandRules` to use context Signed-off-by: Timo Sand <[email protected]> * Fix failing validation Signed-off-by: Timo Sand <[email protected]> * Refactor to use `ValidateDiagFunc` and `validation.ToDiagFunc` in repo ruleset Signed-off-by: Timo Sand <[email protected]> * Use `strings.Join` to make `Description` of `target` dynamic Signed-off-by: Timo Sand <[email protected]> * Add validation to `operator` attributes Signed-off-by: Timo Sand <[email protected]> * Add missing validations to repo ruleset attributes Signed-off-by: Timo Sand <[email protected]> * Refactor to use `ValidateDiagFunc` and `validation.ToDiagFunc` in org ruleset * Use `strings.Join` to make `Description` of `target` dynamic * Add validation to `operator` attributes * Add missing validations to org ruleset attributes * Ensure attribute names map correctly to rule names Signed-off-by: Timo Sand <[email protected]> * Don't ignore errors Signed-off-by: Timo Sand <[email protected]> * Address review comments Signed-off-by: Timo Sand <[email protected]> --------- Signed-off-by: Timo Sand <[email protected]>
1 parent 272856f commit 11d233c

10 files changed

Lines changed: 1466 additions & 288 deletions

‎github/resource_github_organization_ruleset.go‎

Lines changed: 150 additions & 83 deletions
Large diffs are not rendered by default.

‎github/resource_github_organization_ruleset_test.go‎

Lines changed: 327 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ package github
22

33
import (
44
"fmt"
5+
"regexp"
56
"testing"
67

78
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/acctest"
@@ -18,6 +19,19 @@ func TestAccGithubOrganizationRuleset(t *testing.T) {
1819
workflowFilePath := ".github/workflows/echo.yaml"
1920

2021
config := fmt.Sprintf(`
22+
locals {
23+
workflow_content = <<EOT
24+
name: Echo Workflow
25+
26+
on: [pull_request]
27+
28+
jobs:
29+
echo:
30+
runs-on: ubuntu-latest
31+
steps:
32+
- run: echo "Hello, world!"
33+
EOT
34+
}
2135
resource "github_repository" "test" {
2236
name = "%s"
2337
visibility = "private"
@@ -28,17 +42,7 @@ resource "github_repository_file" "workflow_file" {
2842
repository = github_repository.test.name
2943
branch = "main"
3044
file = "%[3]s"
31-
content = <<EOT
32-
name: Echo Workflow
33-
34-
on: [pull_request]
35-
36-
jobs:
37-
echo:
38-
runs-on: linux
39-
steps:
40-
- run: echo \"Hello, world!\"
41-
EOT
45+
content = replace(local.workflow_content, "\t", " ") # NOTE: 'content' must be indented with spaces, not tabs
4246
commit_message = "Managed by Terraform"
4347
commit_author = "Terraform User"
4448
commit_email = "[email protected]"
@@ -210,11 +214,6 @@ resource "github_organization_ruleset" "test" {
210214
include = ["~ALL"]
211215
exclude = []
212216
}
213-
214-
ref_name {
215-
include = ["~ALL"]
216-
exclude = []
217-
}
218217
}
219218
220219
rules {
@@ -499,6 +498,317 @@ resource "github_organization_ruleset" "test" {
499498
},
500499
})
501500
})
501+
502+
t.Run("validates_branch_target_requires_ref_name_condition", func(t *testing.T) {
503+
randomID := acctest.RandStringFromCharSet(5, acctest.CharSetAlphaNum)
504+
config := fmt.Sprintf(`
505+
resource "github_organization_ruleset" "test" {
506+
name = "test-validation-%s"
507+
target = "branch"
508+
enforcement = "active"
509+
510+
conditions {
511+
repository_name {
512+
include = ["~ALL"]
513+
exclude = []
514+
}
515+
}
516+
517+
rules {
518+
creation = true
519+
}
520+
}
521+
`, randomID)
522+
523+
resource.Test(t, resource.TestCase{
524+
PreCheck: func() { skipUnlessHasPaidOrgs(t) },
525+
ProviderFactories: providerFactories,
526+
Steps: []resource.TestStep{
527+
{
528+
Config: config,
529+
ExpectError: regexp.MustCompile("ref_name must be set for branch target"),
530+
},
531+
},
532+
})
533+
})
534+
535+
t.Run("validates_tag_target_requires_ref_name_condition", func(t *testing.T) {
536+
randomID := acctest.RandStringFromCharSet(5, acctest.CharSetAlphaNum)
537+
config := fmt.Sprintf(`
538+
resource "github_organization_ruleset" "test" {
539+
name = "test-tag-no-conditions-%s"
540+
target = "tag"
541+
enforcement = "active"
542+
543+
conditions {
544+
repository_name {
545+
include = ["~ALL"]
546+
exclude = []
547+
}
548+
}
549+
550+
rules {
551+
creation = true
552+
}
553+
}
554+
`, randomID)
555+
556+
resource.Test(t, resource.TestCase{
557+
PreCheck: func() { skipUnlessHasPaidOrgs(t) },
558+
ProviderFactories: providerFactories,
559+
Steps: []resource.TestStep{
560+
{
561+
Config: config,
562+
ExpectError: regexp.MustCompile("ref_name must be set for tag target"),
563+
},
564+
},
565+
})
566+
})
567+
568+
t.Run("validates_push_target_rejects_ref_name_condition", func(t *testing.T) {
569+
randomID := acctest.RandStringFromCharSet(5, acctest.CharSetAlphaNum)
570+
resourceName := "test-push-reject-ref-name"
571+
config := fmt.Sprintf(`
572+
resource "github_organization_ruleset" "%s" {
573+
name = "test-push-with-ref-%s"
574+
target = "push"
575+
enforcement = "active"
576+
577+
conditions {
578+
ref_name {
579+
include = ["~ALL"]
580+
exclude = []
581+
}
582+
repository_name {
583+
include = ["~ALL"]
584+
exclude = []
585+
}
586+
}
587+
588+
rules {
589+
# Push rulesets only support push-specific rules
590+
max_file_size {
591+
max_file_size = 100
592+
}
593+
}
594+
}
595+
`, resourceName, randomID)
596+
597+
resource.Test(t, resource.TestCase{
598+
PreCheck: func() { skipUnlessHasPaidOrgs(t) },
599+
ProviderFactories: providerFactories,
600+
Steps: []resource.TestStep{
601+
{
602+
Config: config,
603+
ExpectError: regexp.MustCompile("ref_name must not be set for push target"),
604+
},
605+
},
606+
})
607+
})
608+
609+
t.Run("validates_push_target_rejects_branch_or_tag_rules", func(t *testing.T) {
610+
randomID := acctest.RandStringFromCharSet(5, acctest.CharSetAlphaNum)
611+
resourceName := "test-push-reject-branch-rules"
612+
config := fmt.Sprintf(`
613+
resource "github_organization_ruleset" "%s" {
614+
name = "test-push-branch-rule-%s"
615+
target = "push"
616+
enforcement = "active"
617+
618+
conditions {
619+
repository_name {
620+
include = ["~ALL"]
621+
exclude = []
622+
}
623+
}
624+
625+
rules {
626+
# 'creation' is a branch/tag rule, not valid for push target
627+
creation = true
628+
}
629+
}
630+
`, resourceName, randomID)
631+
632+
resource.Test(t, resource.TestCase{
633+
PreCheck: func() { skipUnlessHasPaidOrgs(t) },
634+
ProviderFactories: providerFactories,
635+
Steps: []resource.TestStep{
636+
{
637+
Config: config,
638+
ExpectError: regexp.MustCompile("rule .* is not valid for push target"),
639+
},
640+
},
641+
})
642+
})
643+
644+
t.Run("validates_branch_target_rejects_push-only_rules", func(t *testing.T) {
645+
randomID := acctest.RandStringFromCharSet(5, acctest.CharSetAlphaNum)
646+
resourceName := "test-branch-reject-push-rules"
647+
config := fmt.Sprintf(`
648+
resource "github_organization_ruleset" "%s" {
649+
name = "test-branch-push-rule-%s"
650+
target = "branch"
651+
enforcement = "active"
652+
653+
conditions {
654+
ref_name {
655+
include = ["~ALL"]
656+
exclude = []
657+
}
658+
repository_name {
659+
include = ["~ALL"]
660+
exclude = []
661+
}
662+
}
663+
664+
rules {
665+
# 'max_file_size' is a push-only rule, not valid for branch target
666+
max_file_size {
667+
max_file_size = 100
668+
}
669+
}
670+
}
671+
`, resourceName, randomID)
672+
673+
resource.Test(t, resource.TestCase{
674+
PreCheck: func() { skipUnlessHasPaidOrgs(t) },
675+
ProviderFactories: providerFactories,
676+
Steps: []resource.TestStep{
677+
{
678+
Config: config,
679+
ExpectError: regexp.MustCompile("rule .* is not valid for branch target"),
680+
},
681+
},
682+
})
683+
})
684+
685+
t.Run("creates_push_ruleset", func(t *testing.T) {
686+
randomID := acctest.RandStringFromCharSet(5, acctest.CharSetAlphaNum)
687+
rulesetName := fmt.Sprintf("%stest-push-%s", testResourcePrefix, randomID)
688+
resourceName := "test-push-ruleset"
689+
resourceFullName := fmt.Sprintf("github_organization_ruleset.%s", resourceName)
690+
config := fmt.Sprintf(`
691+
resource "github_organization_ruleset" "%s" {
692+
name = "%s"
693+
target = "push"
694+
enforcement = "active"
695+
696+
conditions {
697+
repository_name {
698+
include = ["~ALL"]
699+
exclude = []
700+
}
701+
}
702+
703+
rules {
704+
# Push rulesets only support push-specific rules:
705+
# file_path_restriction, max_file_path_length, file_extension_restriction, max_file_size
706+
max_file_size {
707+
max_file_size = 100
708+
}
709+
}
710+
}
711+
`, resourceName, rulesetName)
712+
713+
resource.Test(t, resource.TestCase{
714+
PreCheck: func() { skipUnlessHasPaidOrgs(t) },
715+
ProviderFactories: providerFactories,
716+
Steps: []resource.TestStep{
717+
{
718+
Config: config,
719+
Check: resource.ComposeTestCheckFunc(
720+
resource.TestCheckResourceAttr(resourceFullName, "name", rulesetName),
721+
resource.TestCheckResourceAttr(resourceFullName, "target", "push"),
722+
resource.TestCheckResourceAttr(resourceFullName, "enforcement", "active"),
723+
resource.TestCheckResourceAttr(resourceFullName, "rules.0.max_file_size.0.max_file_size", "100"),
724+
),
725+
},
726+
},
727+
})
728+
})
729+
730+
t.Run("validates_rules__required_status_checks_block", func(t *testing.T) {
731+
t.Run("required_check__context_block_should_not_be_empty", func(t *testing.T) {
732+
resourceName := "test-required-status-checks-context-is-not-empty"
733+
randomID := acctest.RandString(5)
734+
config := fmt.Sprintf(`
735+
resource "github_organization_ruleset" "%s" {
736+
name = "test-context-is-not-empty-%s"
737+
target = "branch"
738+
enforcement = "active"
739+
740+
conditions {
741+
ref_name {
742+
include = ["~ALL"]
743+
exclude = []
744+
}
745+
repository_name {
746+
include = ["~ALL"]
747+
exclude = []
748+
}
749+
}
750+
751+
rules {
752+
required_status_checks {
753+
required_check {
754+
context = ""
755+
}
756+
}
757+
}
758+
}
759+
`, resourceName, randomID)
760+
761+
resource.Test(t, resource.TestCase{
762+
PreCheck: func() { skipUnlessHasPaidOrgs(t) },
763+
ProviderFactories: providerFactories,
764+
Steps: []resource.TestStep{
765+
{
766+
Config: config,
767+
ExpectError: regexp.MustCompile("expected \"context\" to not be an empty string"),
768+
},
769+
},
770+
})
771+
})
772+
t.Run("required_check_should_be_required_when_strict_required_status_checks_policy_is_set", func(t *testing.T) {
773+
resourceName := "test-required-check-is-required"
774+
randomID := acctest.RandString(5)
775+
config := fmt.Sprintf(`
776+
resource "github_organization_ruleset" "%s" {
777+
name = "test-required-with-%s"
778+
target = "branch"
779+
enforcement = "active"
780+
781+
conditions {
782+
ref_name {
783+
include = ["~ALL"]
784+
exclude = []
785+
}
786+
repository_name {
787+
include = ["~ALL"]
788+
exclude = []
789+
}
790+
}
791+
792+
rules {
793+
required_status_checks {
794+
strict_required_status_checks_policy = true
795+
}
796+
}
797+
}
798+
`, resourceName, randomID)
799+
800+
resource.Test(t, resource.TestCase{
801+
PreCheck: func() { skipUnlessHasPaidOrgs(t) },
802+
ProviderFactories: providerFactories,
803+
Steps: []resource.TestStep{
804+
{
805+
Config: config,
806+
ExpectError: regexp.MustCompile("Insufficient required_check blocks"),
807+
},
808+
},
809+
})
810+
})
811+
})
502812
}
503813

504814
func TestOrganizationPushRulesetSupport(t *testing.T) {
@@ -578,7 +888,7 @@ func TestOrganizationPushRulesetSupport(t *testing.T) {
578888
}
579889

580890
// Test flatten functionality (organization rulesets use org=true)
581-
flattenedResult := flattenRules(expandedRules, true)
891+
flattenedResult := flattenRules(t.Context(), expandedRules, true)
582892

583893
if len(flattenedResult) != 1 {
584894
t.Fatalf("Expected 1 flattened result, got %d", len(flattenedResult))

0 commit comments

Comments
 (0)