From a7247b00aab4ac33c348f3d86be5871dc9bf7ba2 Mon Sep 17 00:00:00 2001 From: Angie Pinilla Date: Sun, 10 May 2020 17:31:04 -0400 Subject: [PATCH 1/3] only enable dismissal restrictions if users/teams set --- github/resource_github_branch_protection.go | 8 ++- .../resource_github_branch_protection_test.go | 49 +++++++++++++++++++ 2 files changed, 55 insertions(+), 2 deletions(-) diff --git a/github/resource_github_branch_protection.go b/github/resource_github_branch_protection.go index a216b3e005..f45af814bd 100644 --- a/github/resource_github_branch_protection.go +++ b/github/resource_github_branch_protection.go @@ -544,9 +544,13 @@ func expandRequiredPullRequestReviews(d *schema.ResourceData) (*github.PullReque m := v.(map[string]interface{}) users := expandNestedSet(m, "dismissal_users") - drr.Users = &users + if len(users) > 0 { + drr.Users = &users + } teams := expandNestedSet(m, "dismissal_teams") - drr.Teams = &teams + if len(teams) > 0 { + drr.Teams = &teams + } rprr.DismissalRestrictionsRequest = drr rprr.DismissStaleReviews = m["dismiss_stale_reviews"].(bool) diff --git a/github/resource_github_branch_protection_test.go b/github/resource_github_branch_protection_test.go index fdd879def4..8417f029cb 100644 --- a/github/resource_github_branch_protection_test.go +++ b/github/resource_github_branch_protection_test.go @@ -218,6 +218,30 @@ func TestAccGithubBranchProtection_emptyItems(t *testing.T) { }) } +func TestAccGithubBranchProtection_emptyDismissalRestrictions(t *testing.T) { + var protection github.Protection + rn := "github_branch_protection.master" + rString := acctest.RandString(5) + repoName := fmt.Sprintf("tf-acc-test-branch-prot-%s", rString) + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + Providers: testAccProviders, + CheckDestroy: testAccGithubBranchProtectionDestroy, + Steps: []resource.TestStep{ + { + Config: testAccGithubBranchProtectionEmptyDismissalsConfig(repoName), + Check: resource.ComposeTestCheckFunc( + testAccCheckGithubProtectedBranchExists("github_branch_protection.master", repoName+":master", &protection), + testAccCheckGithubBranchProtectionPullRequestReviews(&protection, true, []string{}, []string{}, true), + resource.TestCheckResourceAttr(rn, "dismissal_restrictions_users.%", "0"), + resource.TestCheckResourceAttr(rn, "dismissal_restrictions_teams.%", "0"), + ), + }, + }, + }) +} + func testAccCheckGithubProtectedBranchExists(n, id string, protection *github.Protection) resource.TestCheckFunc { return func(s *terraform.State) error { rs, ok := s.RootModule().Resources[n] @@ -307,6 +331,9 @@ func testAccCheckGithubBranchProtectionPullRequestReviews(protection *github.Pro var users, teams []string if reviews.DismissalRestrictions != nil { + if len(expectedUsers) == 0 && len(expectedTeams) == 0 { + return fmt.Errorf("Expected Dismissal Restrictions to be nil but was present") + } for _, u := range reviews.GetDismissalRestrictions().Users { users = append(users, u.GetLogin()) } @@ -384,6 +411,28 @@ func testAccGithubBranchProtectionDestroy(s *terraform.State) error { return nil } +func testAccGithubBranchProtectionEmptyDismissalsConfig(repoName string) string { + return fmt.Sprintf(` + +resource "github_repository" "test" { + name = "%s" + description = "Terraform Acceptance Test %s" + auto_init = true +} + +resource "github_branch_protection" "master" { + repository = "${github_repository.test.name}" + branch = "master" + enforce_admins = true + + required_pull_request_reviews { + dismiss_stale_reviews = true + require_code_owner_reviews = true + } +} +`, repoName, repoName) +} + func testAccGithubBranchProtectionConfig(repoName string) string { return fmt.Sprintf(` resource "github_repository" "test" { From 313b3fdacbc7fcf4d7b7fdd5748bae9d6716c446 Mon Sep 17 00:00:00 2001 From: Angie Pinilla Date: Sun, 10 May 2020 17:41:00 -0400 Subject: [PATCH 2/3] fix resource attributes in test --- .../resource_github_branch_protection_test.go | 51 ++++++++++--------- 1 file changed, 27 insertions(+), 24 deletions(-) diff --git a/github/resource_github_branch_protection_test.go b/github/resource_github_branch_protection_test.go index 8417f029cb..5db0cb5b68 100644 --- a/github/resource_github_branch_protection_test.go +++ b/github/resource_github_branch_protection_test.go @@ -234,8 +234,11 @@ func TestAccGithubBranchProtection_emptyDismissalRestrictions(t *testing.T) { Check: resource.ComposeTestCheckFunc( testAccCheckGithubProtectedBranchExists("github_branch_protection.master", repoName+":master", &protection), testAccCheckGithubBranchProtectionPullRequestReviews(&protection, true, []string{}, []string{}, true), - resource.TestCheckResourceAttr(rn, "dismissal_restrictions_users.%", "0"), - resource.TestCheckResourceAttr(rn, "dismissal_restrictions_teams.%", "0"), + resource.TestCheckResourceAttr(rn, "required_pull_request_reviews.#", "1"), + resource.TestCheckResourceAttr(rn, "required_pull_request_reviews.0.dismiss_stale_reviews", "true"), + resource.TestCheckResourceAttr(rn, "required_pull_request_reviews.0.require_code_owner_reviews", "true"), + resource.TestCheckResourceAttr(rn, "required_pull_request_reviews.0.dismissal_users.#", "0"), + resource.TestCheckResourceAttr(rn, "required_pull_request_reviews.0.dismissal_teams.#", "0"), ), }, }, @@ -411,28 +414,6 @@ func testAccGithubBranchProtectionDestroy(s *terraform.State) error { return nil } -func testAccGithubBranchProtectionEmptyDismissalsConfig(repoName string) string { - return fmt.Sprintf(` - -resource "github_repository" "test" { - name = "%s" - description = "Terraform Acceptance Test %s" - auto_init = true -} - -resource "github_branch_protection" "master" { - repository = "${github_repository.test.name}" - branch = "master" - enforce_admins = true - - required_pull_request_reviews { - dismiss_stale_reviews = true - require_code_owner_reviews = true - } -} -`, repoName, repoName) -} - func testAccGithubBranchProtectionConfig(repoName string) string { return fmt.Sprintf(` resource "github_repository" "test" { @@ -578,3 +559,25 @@ resource "github_branch_protection" "master" { } `, repoName, repoName, user) } + +func testAccGithubBranchProtectionEmptyDismissalsConfig(repoName string) string { + return fmt.Sprintf(` + +resource "github_repository" "test" { + name = "%s" + description = "Terraform Acceptance Test %s" + auto_init = true +} + +resource "github_branch_protection" "master" { + repository = "${github_repository.test.name}" + branch = "master" + enforce_admins = true + + required_pull_request_reviews { + dismiss_stale_reviews = true + require_code_owner_reviews = true + } +} +`, repoName, repoName) +} From caace39cf358ff43af29b05f1bb9e71135bd8694 Mon Sep 17 00:00:00 2001 From: Angie Pinilla Date: Sun, 10 May 2020 17:44:14 -0400 Subject: [PATCH 3/3] add import validation --- github/resource_github_branch_protection_test.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/github/resource_github_branch_protection_test.go b/github/resource_github_branch_protection_test.go index 5db0cb5b68..a13284d130 100644 --- a/github/resource_github_branch_protection_test.go +++ b/github/resource_github_branch_protection_test.go @@ -221,8 +221,7 @@ func TestAccGithubBranchProtection_emptyItems(t *testing.T) { func TestAccGithubBranchProtection_emptyDismissalRestrictions(t *testing.T) { var protection github.Protection rn := "github_branch_protection.master" - rString := acctest.RandString(5) - repoName := fmt.Sprintf("tf-acc-test-branch-prot-%s", rString) + repoName := acctest.RandomWithPrefix("tf-acc-test-branch-prot-") resource.ParallelTest(t, resource.TestCase{ PreCheck: func() { testAccPreCheck(t) }, @@ -241,6 +240,11 @@ func TestAccGithubBranchProtection_emptyDismissalRestrictions(t *testing.T) { resource.TestCheckResourceAttr(rn, "required_pull_request_reviews.0.dismissal_teams.#", "0"), ), }, + { + ResourceName: rn, + ImportState: true, + ImportStateVerify: true, + }, }, }) }