From 8bed68e93190b42a40d6bb008f38606774fad886 Mon Sep 17 00:00:00 2001 From: georgekaz <1391828+georgekaz@users.noreply.github.com> Date: Fri, 2 Feb 2024 22:05:27 +0000 Subject: [PATCH] Fix restrict pushes on github_branch_protection. Fix branch protection tests (#2045) * Update restrict pushes. Fix branch protection tests Change blocks_creations default to true. Fix broken build. * add state migration * rename push_restrictions to push_allowances * correct state migration issue * add docs clarification * update migration func args * fix test args * cleanup tests * Pass context.Background() in test * fix timestamp fields --------- Co-authored-by: Keegan Campbell --- github/data_source_github_user.go | 6 +- github/data_source_github_user_test.go | 2 +- github/migrate_github_branch_protection.go | 37 +++ github/resource_github_branch_protection.go | 50 ++-- .../resource_github_branch_protection_test.go | 265 +++++++++++++----- github/resource_github_release.go | 4 +- github/util_v4_branch_protection.go | 86 ++++-- github/util_v4_consts.go | 15 +- .../docs/r/branch_protection.html.markdown | 32 ++- 9 files changed, 350 insertions(+), 147 deletions(-) diff --git a/github/data_source_github_user.go b/github/data_source_github_user.go index b969e30b36..541b8f69fa 100644 --- a/github/data_source_github_user.go +++ b/github/data_source_github_user.go @@ -181,13 +181,13 @@ func dataSourceGithubUserRead(d *schema.ResourceData, meta interface{}) error { if err = d.Set("following", user.GetFollowing()); err != nil { return err } - if err = d.Set("created_at", user.GetCreatedAt()); err != nil { + if err = d.Set("created_at", user.GetCreatedAt().String()); err != nil { return err } - if err = d.Set("updated_at", user.GetUpdatedAt()); err != nil { + if err = d.Set("updated_at", user.GetUpdatedAt().String()); err != nil { return err } - if err = d.Set("suspended_at", user.GetSuspendedAt()); err != nil { + if err = d.Set("suspended_at", user.GetSuspendedAt().String()); err != nil { return err } if err = d.Set("node_id", user.GetNodeID()); err != nil { diff --git a/github/data_source_github_user_test.go b/github/data_source_github_user_test.go index d5c8fb7511..68f767b61a 100644 --- a/github/data_source_github_user_test.go +++ b/github/data_source_github_user_test.go @@ -19,7 +19,7 @@ func TestAccGithubUserDataSource(t *testing.T) { `, testOwnerFunc()) check := resource.ComposeAggregateTestCheckFunc( - resource.TestCheckResourceAttrSet("data.github_user.test", "name"), + resource.TestCheckResourceAttrSet("data.github_user.test", "login"), resource.TestCheckResourceAttrSet("data.github_user.test", "id"), ) diff --git a/github/migrate_github_branch_protection.go b/github/migrate_github_branch_protection.go index 7a35d31870..2eccae4992 100644 --- a/github/migrate_github_branch_protection.go +++ b/github/migrate_github_branch_protection.go @@ -42,3 +42,40 @@ func resourceGithubBranchProtectionUpgradeV0(_ context.Context, rawState map[str return rawState, nil } + +func resourceGithubBranchProtectionV1() *schema.Resource { + return &schema.Resource{ + Schema: map[string]*schema.Schema{ + "push_restrictions": { + Type: schema.TypeSet, + Optional: true, + Elem: &schema.Schema{Type: schema.TypeString}, + }, + "blocks_creations": { + Type: schema.TypeBool, + Optional: true, + Default: false, + }, + }, + } +} + +func resourceGithubBranchProtectionUpgradeV1(_ context.Context, rawState map[string]interface{}, meta interface{}) (map[string]interface{}, error) { + var blocksCreations bool = false + + if v, ok := rawState["blocks_creations"]; ok { + blocksCreations = v.(bool) + } + + if v, ok := rawState["push_restrictions"]; ok { + rawState["restrict_pushes"] = []interface{}{map[string]interface{}{ + "blocks_creations": blocksCreations, + "push_allowances": v, + }} + } + + delete(rawState, "blocks_creations") + delete(rawState, "push_restrictions") + + return rawState, nil +} diff --git a/github/resource_github_branch_protection.go b/github/resource_github_branch_protection.go index b96f9446e0..3235aecd89 100644 --- a/github/resource_github_branch_protection.go +++ b/github/resource_github_branch_protection.go @@ -13,7 +13,7 @@ import ( func resourceGithubBranchProtection() *schema.Resource { return &schema.Resource{ - SchemaVersion: 1, + SchemaVersion: 2, Schema: map[string]*schema.Schema{ // Input @@ -40,12 +40,6 @@ func resourceGithubBranchProtection() *schema.Resource { Default: false, Description: "Setting this to 'true' to allow force pushes on the branch.", }, - PROTECTION_BLOCKS_CREATIONS: { - Type: schema.TypeBool, - Optional: true, - Default: false, - Description: "Setting this to 'true' to block creating the branch.", - }, PROTECTION_IS_ADMIN_ENFORCED: { Type: schema.TypeBool, Optional: true, @@ -104,7 +98,7 @@ func resourceGithubBranchProtection() *schema.Resource { Optional: true, Description: "Restrict pull request review dismissals.", }, - PROTECTION_RESTRICTS_REVIEW_DISMISSERS: { + PROTECTION_REVIEW_DISMISSAL_ALLOWANCES: { Type: schema.TypeSet, Optional: true, Description: "The list of actor Names/IDs with dismissal access. If not empty, 'restrict_dismissals' is ignored. Actor names must either begin with a '/' for users or the organization name followed by a '/' for teams.", @@ -116,7 +110,7 @@ func resourceGithubBranchProtection() *schema.Resource { Description: "The list of actor Names/IDs that are allowed to bypass pull request requirements. Actor names must either begin with a '/' for users or the organization name followed by a '/' for teams.", Elem: &schema.Schema{Type: schema.TypeString}, }, - PROTECTION_REQUIRES_LAST_PUSH_APPROVAL: { + PROTECTION_REQUIRE_LAST_PUSH_APPROVAL: { Type: schema.TypeBool, Optional: true, Default: false, @@ -146,10 +140,25 @@ func resourceGithubBranchProtection() *schema.Resource { }, }, PROTECTION_RESTRICTS_PUSHES: { - Type: schema.TypeSet, + Type: schema.TypeList, Optional: true, - Description: "The list of actor Names/IDs that may push to the branch. Actor names must either begin with a '/' for users or the organization name followed by a '/' for teams.", - Elem: &schema.Schema{Type: schema.TypeString}, + Description: "Restrict who can push to matching branches.", + Elem: &schema.Resource{ + Schema: map[string]*schema.Schema{ + PROTECTION_BLOCKS_CREATIONS: { + Type: schema.TypeBool, + Optional: true, + Default: true, + Description: "Restrict pushes that create matching branches.", + }, + PROTECTION_PUSH_ALLOWANCES: { + Type: schema.TypeSet, + Optional: true, + Description: "The list of actor Names/IDs that may push to the branch. Actor names must either begin with a '/' for users or the organization name followed by a '/' for teams.", + Elem: &schema.Schema{Type: schema.TypeString}, + }, + }, + }, }, PROTECTION_FORCE_PUSHES_BYPASSERS: { Type: schema.TypeSet, @@ -174,6 +183,11 @@ func resourceGithubBranchProtection() *schema.Resource { Upgrade: resourceGithubBranchProtectionUpgradeV0, Version: 0, }, + { + Type: resourceGithubBranchProtectionV1().CoreConfigSchema().ImpliedType(), + Upgrade: resourceGithubBranchProtectionUpgradeV1, + Version: 1, + }, }, } } @@ -265,7 +279,6 @@ func resourceGithubBranchProtectionRead(d *schema.ResourceData, meta interface{} variables := map[string]interface{}{ "id": d.Id(), } - ctx := context.WithValue(context.Background(), ctxId, d.Id()) client := meta.(*Owner).v4client err := client.Query(ctx, &query, variables) @@ -278,7 +291,6 @@ func resourceGithubBranchProtectionRead(d *schema.ResourceData, meta interface{} return err } - protection := query.Node.Node err = d.Set(PROTECTION_PATTERN, protection.Pattern) @@ -296,11 +308,6 @@ func resourceGithubBranchProtectionRead(d *schema.ResourceData, meta interface{} log.Printf("[DEBUG] Problem setting '%s' in %s %s branch protection (%s)", PROTECTION_ALLOWS_FORCE_PUSHES, protection.Repository.Name, protection.Pattern, d.Id()) } - err = d.Set(PROTECTION_BLOCKS_CREATIONS, protection.BlocksCreations) - if err != nil { - log.Printf("[DEBUG] Problem setting '%s' in %s %s branch protection (%s)", PROTECTION_BLOCKS_CREATIONS, protection.Repository.Name, protection.Pattern, d.Id()) - } - err = d.Set(PROTECTION_IS_ADMIN_ENFORCED, protection.IsAdminEnforced) if err != nil { log.Printf("[DEBUG] Problem setting '%s' in %s %s branch protection (%s)", PROTECTION_IS_ADMIN_ENFORCED, protection.Repository.Name, protection.Pattern, d.Id()) @@ -350,11 +357,6 @@ func resourceGithubBranchProtectionRead(d *schema.ResourceData, meta interface{} log.Printf("[DEBUG] Problem setting '%s' in %s %s branch protection (%s)", PROTECTION_FORCE_PUSHES_BYPASSERS, protection.Repository.Name, protection.Pattern, d.Id()) } - err = d.Set(PROTECTION_REQUIRES_LAST_PUSH_APPROVAL, protection.RequireLastPushApproval) - if err != nil { - log.Printf("[DEBUG] Problem setting '%s' in %s %s branch protection (%s)", PROTECTION_REQUIRES_LAST_PUSH_APPROVAL, protection.Repository.Name, protection.Pattern, d.Id()) - } - err = d.Set(PROTECTION_LOCK_BRANCH, protection.LockBranch) if err != nil { log.Printf("[DEBUG] Problem setting '%s' in %s %s branch protection (%s)", PROTECTION_LOCK_BRANCH, protection.Repository.Name, protection.Pattern, d.Id()) diff --git a/github/resource_github_branch_protection_test.go b/github/resource_github_branch_protection_test.go index 55be969256..a4872bf114 100644 --- a/github/resource_github_branch_protection_test.go +++ b/github/resource_github_branch_protection_test.go @@ -1,7 +1,9 @@ package github import ( + "context" "fmt" + "reflect" "regexp" "testing" @@ -10,11 +12,10 @@ import ( "github.com/hashicorp/terraform-plugin-sdk/v2/terraform" ) -func TestAccGithubBranchProtection(t *testing.T) { - - randomID := acctest.RandStringFromCharSet(5, acctest.CharSetAlphaNum) +func TestAccGithubBranchProtectionV4(t *testing.T) { t.Run("configures default settings when empty", func(t *testing.T) { + randomID := acctest.RandStringFromCharSet(5, acctest.CharSetAlphaNum) config := fmt.Sprintf(` @@ -25,8 +26,8 @@ func TestAccGithubBranchProtection(t *testing.T) { resource "github_branch_protection" "test" { - repository_id = github_repository.test.node_id - pattern = "main" + repository_id = github_repository.test.node_id + pattern = "main" } @@ -49,7 +50,7 @@ func TestAccGithubBranchProtection(t *testing.T) { "github_branch_protection.test", "required_pull_request_reviews.#", "0", ), resource.TestCheckResourceAttr( - "github_branch_protection.test", "push_restrictions.#", "0", + "github_branch_protection.test", "restrict_pushes.#", "0", ), resource.TestCheckResourceAttr( "github_branch_protection.test", "lock_branch", "false", @@ -77,7 +78,7 @@ func TestAccGithubBranchProtection(t *testing.T) { ResourceName: "github_branch_protection.test", ImportState: true, ExpectError: regexp.MustCompile( - `Could not find a branch protection rule with the pattern 'no-such-pattern'\.`, + `could not find a branch protection rule with the pattern 'no-such-pattern'`, ), ImportStateIdFunc: importBranchProtectionByRepoName( fmt.Sprintf("tf-acc-test-%s", randomID), "no-such-pattern", @@ -102,6 +103,7 @@ func TestAccGithubBranchProtection(t *testing.T) { }) t.Run("configures default settings when conversation resolution is true", func(t *testing.T) { + randomID := acctest.RandStringFromCharSet(5, acctest.CharSetAlphaNum) config := fmt.Sprintf(` @@ -112,8 +114,8 @@ func TestAccGithubBranchProtection(t *testing.T) { resource "github_branch_protection" "test" { - repository_id = github_repository.test.node_id - pattern = "main" + repository_id = github_repository.test.node_id + pattern = "main" require_conversation_resolution = true } @@ -140,7 +142,7 @@ func TestAccGithubBranchProtection(t *testing.T) { "github_branch_protection.test", "required_pull_request_reviews.#", "0", ), resource.TestCheckResourceAttr( - "github_branch_protection.test", "push_restrictions.#", "0", + "github_branch_protection.test", "restrict_pushes.#", "0", ), resource.TestCheckResourceAttr( "github_branch_protection.test", "lock_branch", "false", @@ -168,7 +170,7 @@ func TestAccGithubBranchProtection(t *testing.T) { ResourceName: "github_branch_protection.test", ImportState: true, ExpectError: regexp.MustCompile( - `Could not find a branch protection rule with the pattern 'no-such-pattern'\.`, + `could not find a branch protection rule with the pattern 'no-such-pattern'`, ), ImportStateIdFunc: importBranchProtectionByRepoName( fmt.Sprintf("tf-acc-test-%s", randomID), "no-such-pattern", @@ -193,6 +195,7 @@ func TestAccGithubBranchProtection(t *testing.T) { }) t.Run("configures required status checks", func(t *testing.T) { + randomID := acctest.RandStringFromCharSet(5, acctest.CharSetAlphaNum) config := fmt.Sprintf(` @@ -203,10 +206,10 @@ func TestAccGithubBranchProtection(t *testing.T) { resource "github_branch_protection" "test" { - repository_id = github_repository.test.node_id - pattern = "main" + repository_id = github_repository.test.node_id + pattern = "main" - required_status_checks { + required_status_checks { strict = true contexts = ["github/foo"] } @@ -241,7 +244,7 @@ func TestAccGithubBranchProtection(t *testing.T) { ResourceName: "github_branch_protection.test", ImportState: true, ExpectError: regexp.MustCompile( - `Could not find a branch protection rule with the pattern 'no-such-pattern'\.`, + `could not find a branch protection rule with the pattern 'no-such-pattern'`, ), ImportStateIdFunc: importBranchProtectionByRepoID( "github_repository.test", "no-such-pattern"), @@ -265,6 +268,7 @@ func TestAccGithubBranchProtection(t *testing.T) { }) t.Run("configures required pull request reviews", func(t *testing.T) { + randomID := acctest.RandStringFromCharSet(5, acctest.CharSetAlphaNum) config := fmt.Sprintf(` @@ -275,14 +279,14 @@ func TestAccGithubBranchProtection(t *testing.T) { resource "github_branch_protection" "test" { - repository_id = github_repository.test.node_id - pattern = "main" + repository_id = github_repository.test.node_id + pattern = "main" - required_pull_request_reviews { - dismiss_stale_reviews = true - require_code_owner_reviews = true - require_last_push_approval = true - } + required_pull_request_reviews { + dismiss_stale_reviews = true + require_code_owner_reviews = true + require_last_push_approval = true + } } @@ -334,6 +338,64 @@ func TestAccGithubBranchProtection(t *testing.T) { }) t.Run("configures branch push restrictions", func(t *testing.T) { + randomID := acctest.RandStringFromCharSet(5, acctest.CharSetAlphaNum) + + config := fmt.Sprintf(` + resource "github_repository" "test" { + name = "tf-acc-test-%s" + auto_init = true + } + + resource "github_branch_protection" "test" { + + repository_id = github_repository.test.node_id + pattern = "main" + + restrict_pushes {} + } + `, randomID) + + check := resource.ComposeAggregateTestCheckFunc( + resource.TestCheckResourceAttr( + "github_branch_protection.test", "restrict_pushes.#", "1", + ), + resource.TestCheckResourceAttr( + "github_branch_protection.test", "restrict_pushes.0.blocks_creations", "true", + ), + resource.TestCheckResourceAttr( + "github_branch_protection.test", "restrict_pushes.0.push_allowances.#", "0", + ), + ) + + testCase := func(t *testing.T, mode string) { + resource.Test(t, resource.TestCase{ + PreCheck: func() { skipUnlessMode(t, mode) }, + Providers: testAccProviders, + Steps: []resource.TestStep{ + { + Config: config, + Check: check, + }, + }, + }) + } + + t.Run("with an anonymous account", func(t *testing.T) { + t.Skip("anonymous account not supported for this operation") + }) + + t.Run("with an individual account", func(t *testing.T) { + t.Skip("individual account not supported for this operation") + }) + + t.Run("with an organization account", func(t *testing.T) { + testCase(t, organization) + }) + + }) + + t.Run("configures branch push restrictions with node_id", func(t *testing.T) { + randomID := acctest.RandStringFromCharSet(5, acctest.CharSetAlphaNum) config := fmt.Sprintf(` resource "github_repository" "test" { @@ -347,19 +409,26 @@ func TestAccGithubBranchProtection(t *testing.T) { resource "github_branch_protection" "test" { - repository_id = github_repository.test.name - pattern = "main" - - push_restrictions = [ - data.github_user.test.node_id, - ] + repository_id = github_repository.test.node_id + pattern = "main" + restrict_pushes { + push_allowances = [ + data.github_user.test.node_id, + ] + } } `, randomID, testOwnerFunc()) check := resource.ComposeAggregateTestCheckFunc( resource.TestCheckResourceAttr( - "github_branch_protection.test", "push_restrictions.#", "1", + "github_branch_protection.test", "restrict_pushes.#", "1", + ), + resource.TestCheckResourceAttr( + "github_branch_protection.test", "restrict_pushes.0.blocks_creations", "true", + ), + resource.TestCheckResourceAttr( + "github_branch_protection.test", "restrict_pushes.0.push_allowances.#", "1", ), ) @@ -391,6 +460,7 @@ func TestAccGithubBranchProtection(t *testing.T) { }) t.Run("configures branch push restrictions with username", func(t *testing.T) { + randomID := acctest.RandStringFromCharSet(5, acctest.CharSetAlphaNum) user := fmt.Sprintf("/%s", testOwnerFunc()) config := fmt.Sprintf(` @@ -401,19 +471,26 @@ func TestAccGithubBranchProtection(t *testing.T) { resource "github_branch_protection" "test" { - repository_id = github_repository.test.name - pattern = "main" - - push_restrictions = [ - "%s", - ] + repository_id = github_repository.test.node_id + pattern = "main" + restrict_pushes { + push_allowances = [ + "%s", + ] + } } `, randomID, user) check := resource.ComposeAggregateTestCheckFunc( resource.TestCheckResourceAttr( - "github_branch_protection.test", "push_restrictions.#", "1", + "github_branch_protection.test", "restrict_pushes.#", "1", + ), + resource.TestCheckResourceAttr( + "github_branch_protection.test", "restrict_pushes.0.blocks_creations", "true", + ), + resource.TestCheckResourceAttr( + "github_branch_protection.test", "restrict_pushes.0.push_allowances.#", "1", ), ) @@ -444,7 +521,8 @@ func TestAccGithubBranchProtection(t *testing.T) { }) - t.Run("configures force pushes and deletions", func(t *testing.T) { + t.Run("configures branch push restrictions with blocksCreations false", func(t *testing.T) { + randomID := acctest.RandStringFromCharSet(5, acctest.CharSetAlphaNum) config := fmt.Sprintf(` resource "github_repository" "test" { @@ -452,26 +530,26 @@ func TestAccGithubBranchProtection(t *testing.T) { auto_init = true } - data "github_user" "test" { - username = "%s" - } - resource "github_branch_protection" "test" { - repository_id = github_repository.test.name - pattern = "main" - allows_deletions = true - allows_force_pushes = true + repository_id = github_repository.test.node_id + pattern = "main" + restrict_pushes { + blocks_creations = false + } } - `, randomID, testOwnerFunc()) + `, randomID) check := resource.ComposeAggregateTestCheckFunc( resource.TestCheckResourceAttr( - "github_branch_protection.test", "allows_deletions", "true", + "github_branch_protection.test", "restrict_pushes.#", "1", ), resource.TestCheckResourceAttr( - "github_branch_protection.test", "allows_force_pushes", "true", + "github_branch_protection.test", "restrict_pushes.0.blocks_creations", "false", + ), + resource.TestCheckResourceAttr( + "github_branch_protection.test", "restrict_pushes.0.push_allowances.#", "0", ), ) @@ -502,7 +580,8 @@ func TestAccGithubBranchProtection(t *testing.T) { }) - t.Run("configures blocksCreations", func(t *testing.T) { + t.Run("configures force pushes and deletions", func(t *testing.T) { + randomID := acctest.RandStringFromCharSet(5, acctest.CharSetAlphaNum) config := fmt.Sprintf(` resource "github_repository" "test" { @@ -516,16 +595,21 @@ func TestAccGithubBranchProtection(t *testing.T) { resource "github_branch_protection" "test" { - repository_id = github_repository.test.name - pattern = "main" - blocks_creations = true + repository_id = github_repository.test.node_id + pattern = "main" + + allows_deletions = true + allows_force_pushes = true } `, randomID, testOwnerFunc()) check := resource.ComposeAggregateTestCheckFunc( resource.TestCheckResourceAttr( - "github_branch_protection.test", "blocks_creations", "true", + "github_branch_protection.test", "allows_deletions", "true", + ), + resource.TestCheckResourceAttr( + "github_branch_protection.test", "allows_force_pushes", "true", ), ) @@ -557,6 +641,7 @@ func TestAccGithubBranchProtection(t *testing.T) { }) t.Run("configures non-empty list of force push bypassers", func(t *testing.T) { + randomID := acctest.RandStringFromCharSet(5, acctest.CharSetAlphaNum) config := fmt.Sprintf(` @@ -571,12 +656,12 @@ func TestAccGithubBranchProtection(t *testing.T) { resource "github_branch_protection" "test" { - repository_id = github_repository.test.node_id - pattern = "main" + repository_id = github_repository.test.node_id + pattern = "main" - force_push_bypassers = [ - data.github_user.test.node_id - ] + force_push_bypassers = [ + data.github_user.test.node_id + ] } @@ -616,6 +701,7 @@ func TestAccGithubBranchProtection(t *testing.T) { }) t.Run("configures empty list of force push bypassers", func(t *testing.T) { + randomID := acctest.RandStringFromCharSet(5, acctest.CharSetAlphaNum) config := fmt.Sprintf(` @@ -626,10 +712,10 @@ func TestAccGithubBranchProtection(t *testing.T) { resource "github_branch_protection" "test" { - repository_id = github_repository.test.node_id - pattern = "main" + repository_id = github_repository.test.node_id + pattern = "main" - force_push_bypassers = [] + force_push_bypassers = [] } @@ -669,7 +755,9 @@ func TestAccGithubBranchProtection(t *testing.T) { }) t.Run("configures non-empty list of pull request bypassers", func(t *testing.T) { + randomID := acctest.RandStringFromCharSet(5, acctest.CharSetAlphaNum) + user := fmt.Sprintf("/%s", testOwnerFunc()) config := fmt.Sprintf(` resource "github_repository" "test" { @@ -679,26 +767,23 @@ func TestAccGithubBranchProtection(t *testing.T) { resource "github_branch_protection" "test" { - repository_id = github_repository.test.node_id - pattern = "main" + repository_id = github_repository.test.node_id + pattern = "main" - required_pull_request_reviews { - pull_request_bypassers = [ - "1234", - ] - } + required_pull_request_reviews { + pull_request_bypassers = [ + "%s", + ] + } } - `, randomID) + `, randomID, user) check := resource.ComposeAggregateTestCheckFunc( resource.TestCheckResourceAttr( "github_branch_protection.test", "required_pull_request_reviews.0.pull_request_bypassers.#", "1", ), - resource.TestCheckResourceAttr( - "github_branch_protection.test", "required_pull_request_reviews.0.pull_request_bypassers.0", "1234", - ), ) testCase := func(t *testing.T, mode string) { @@ -729,6 +814,7 @@ func TestAccGithubBranchProtection(t *testing.T) { }) t.Run("configures empty list of pull request bypassers", func(t *testing.T) { + randomID := acctest.RandStringFromCharSet(5, acctest.CharSetAlphaNum) config := fmt.Sprintf(` @@ -742,9 +828,9 @@ func TestAccGithubBranchProtection(t *testing.T) { repository_id = github_repository.test.node_id pattern = "main" - required_pull_request_reviews { - pull_request_bypassers = [] - } + required_pull_request_reviews { + pull_request_bypassers = [] + } } @@ -807,3 +893,32 @@ func importBranchProtectionByRepoID(repoLogicalName, pattern string) resource.Im return fmt.Sprintf("%s:%s", repoID, pattern), nil } } + +func testGithubBranchProtectionStateDataV1() map[string]interface{} { + return map[string]interface{}{ + "blocks_creations": true, + "push_restrictions": [...]string{"/example-user"}, + } +} + +func testGithubBranchProtectionStateDataV2() map[string]interface{} { + restrictions := []interface{}{map[string]interface{}{ + "blocks_creations": true, + "push_allowances": [...]string{"/example-user"}, + }} + return map[string]interface{}{ + "restrict_pushes": restrictions, + } +} + +func TestAccGithubBranchProtectionV4StateUpgradeV1(t *testing.T) { + expected := testGithubBranchProtectionStateDataV2() + actual, err := resourceGithubBranchProtectionUpgradeV1(context.Background(), testGithubBranchProtectionStateDataV1(), nil) + if err != nil { + t.Fatalf("error migrating state: %s", err) + } + + if !reflect.DeepEqual(expected, actual) { + t.Fatalf("\n\nexpected:\n\n%#v\n\ngot:\n\n%#v\n\n", expected, actual) + } +} diff --git a/github/resource_github_release.go b/github/resource_github_release.go index a60e0502cc..0fd606ba3c 100644 --- a/github/resource_github_release.go +++ b/github/resource_github_release.go @@ -239,8 +239,8 @@ func transformResponseToResourceData(d *schema.ResourceData, release *github.Rep _ = d.Set("generate_release_notes", release.GetGenerateReleaseNotes()) _ = d.Set("prerelease", release.GetPrerelease()) _ = d.Set("discussion_category_name", release.GetDiscussionCategoryName()) - _ = d.Set("created_at", release.GetCreatedAt()) - _ = d.Set("published_at", release.GetPublishedAt()) + _ = d.Set("created_at", release.GetCreatedAt().String()) + _ = d.Set("published_at", release.GetPublishedAt().String()) _ = d.Set("url", release.GetURL()) _ = d.Set("html_url", release.GetHTMLURL()) _ = d.Set("assets_url", release.GetAssetsURL()) diff --git a/github/util_v4_branch_protection.go b/github/util_v4_branch_protection.go index 2ff0fc92ab..eae3467cda 100644 --- a/github/util_v4_branch_protection.go +++ b/github/util_v4_branch_protection.go @@ -144,10 +144,6 @@ func branchProtectionResourceData(d *schema.ResourceData, meta interface{}) (Bra data.AllowsForcePushes = v.(bool) } - if v, ok := d.GetOk(PROTECTION_BLOCKS_CREATIONS); ok { - data.BlocksCreations = v.(bool) - } - if v, ok := d.GetOk(PROTECTION_IS_ADMIN_ENFORCED); ok { data.IsAdminEnforced = v.(bool) } @@ -190,7 +186,7 @@ func branchProtectionResourceData(d *schema.ResourceData, meta interface{}) (Bra if v, ok := m[PROTECTION_RESTRICTS_REVIEW_DISMISSALS]; ok { data.RestrictsReviewDismissals = v.(bool) } - if v, ok := m[PROTECTION_RESTRICTS_REVIEW_DISMISSERS]; ok { + if v, ok := m[PROTECTION_REVIEW_DISMISSAL_ALLOWANCES]; ok { reviewDismissalActorIDs := make([]string, 0) vL := v.(*schema.Set).List() for _, v := range vL { @@ -211,7 +207,7 @@ func branchProtectionResourceData(d *schema.ResourceData, meta interface{}) (Bra data.BypassPullRequestActorIDs = bypassPullRequestActorIDs } } - if v, ok := m[PROTECTION_REQUIRES_LAST_PUSH_APPROVAL]; ok { + if v, ok := m[PROTECTION_REQUIRE_LAST_PUSH_APPROVAL]; ok { data.RequireLastPushApproval = v.(bool) } } @@ -239,14 +235,32 @@ func branchProtectionResourceData(d *schema.ResourceData, meta interface{}) (Bra } if v, ok := d.GetOk(PROTECTION_RESTRICTS_PUSHES); ok { - pushActorIDs := make([]string, 0) - vL := v.(*schema.Set).List() - for _, v := range vL { - pushActorIDs = append(pushActorIDs, v.(string)) + vL := v.([]interface{}) + if len(vL) > 1 { + return BranchProtectionResourceData{}, + fmt.Errorf("error multiple %s declarations", PROTECTION_RESTRICTS_PUSHES) } - if len(pushActorIDs) > 0 { - data.PushActorIDs = pushActorIDs + for _, v := range vL { + if v == nil { + break + } + data.RestrictsPushes = true + + m := v.(map[string]interface{}) + if v, ok := m[PROTECTION_BLOCKS_CREATIONS]; ok { + data.BlocksCreations = v.(bool) + } + if v, ok := m[PROTECTION_PUSH_ALLOWANCES]; ok { + pushActorIDs := make([]string, 0) + vL := v.(*schema.Set).List() + for _, v := range vL { + pushActorIDs = append(pushActorIDs, v.(string)) + } + if len(pushActorIDs) > 0 { + data.PushActorIDs = pushActorIDs + } + } } } @@ -283,7 +297,7 @@ func branchProtectionResourceDataActors(d *schema.ResourceData, meta interface{} } m := v.(map[string]interface{}) - if v, ok := m[PROTECTION_RESTRICTS_REVIEW_DISMISSERS]; ok { + if v, ok := m[PROTECTION_REVIEW_DISMISSAL_ALLOWANCES]; ok { reviewDismissalActorIDs := make([]string, 0) vL := v.(*schema.Set).List() for _, v := range vL { @@ -308,14 +322,32 @@ func branchProtectionResourceDataActors(d *schema.ResourceData, meta interface{} } if v, ok := d.GetOk(PROTECTION_RESTRICTS_PUSHES); ok { - pushActorIDs := make([]string, 0) - vL := v.(*schema.Set).List() - for _, v := range vL { - pushActorIDs = append(pushActorIDs, v.(string)) + vL := v.([]interface{}) + if len(vL) > 1 { + return BranchProtectionResourceData{}, + fmt.Errorf("error multiple %s declarations", PROTECTION_RESTRICTS_PUSHES) } - if len(pushActorIDs) > 0 { - data.PushActorIDs = pushActorIDs + for _, v := range vL { + if v == nil { + break + } + data.RestrictsPushes = true + + m := v.(map[string]interface{}) + if v, ok := m[PROTECTION_BLOCKS_CREATIONS]; ok { + data.BlocksCreations = v.(bool) + } + if v, ok := m[PROTECTION_PUSH_ALLOWANCES]; ok { + pushActorIDs := make([]string, 0) + vL := v.(*schema.Set).List() + for _, v := range vL { + pushActorIDs = append(pushActorIDs, v.(string)) + } + if len(pushActorIDs) > 0 { + data.PushActorIDs = pushActorIDs + } + } } } @@ -474,9 +506,9 @@ func setApprovingReviews(protection BranchProtectionRule, data BranchProtectionR PROTECTION_REQUIRES_CODE_OWNER_REVIEWS: protection.RequiresCodeOwnerReviews, PROTECTION_DISMISSES_STALE_REVIEWS: protection.DismissesStaleReviews, PROTECTION_RESTRICTS_REVIEW_DISMISSALS: protection.RestrictsReviewDismissals, - PROTECTION_RESTRICTS_REVIEW_DISMISSERS: dismissalActors, + PROTECTION_REVIEW_DISMISSAL_ALLOWANCES: dismissalActors, PROTECTION_PULL_REQUESTS_BYPASSERS: bypassPullRequestActors, - PROTECTION_REQUIRES_LAST_PUSH_APPROVAL: protection.RequireLastPushApproval, + PROTECTION_REQUIRE_LAST_PUSH_APPROVAL: protection.RequireLastPushApproval, }, } @@ -498,14 +530,22 @@ func setStatusChecks(protection BranchProtectionRule) interface{} { return statusChecks } -func setPushes(protection BranchProtectionRule, data BranchProtectionResourceData, meta interface{}) []string { +func setPushes(protection BranchProtectionRule, data BranchProtectionResourceData, meta interface{}) interface{} { if !protection.RestrictsPushes { return nil } + pushAllowances := protection.PushAllowances.Nodes pushActors := setPushActorIDs(pushAllowances, data, meta) - return pushActors + restrictsPushes := []interface{}{ + map[string]interface{}{ + PROTECTION_BLOCKS_CREATIONS: protection.BlocksCreations, + PROTECTION_PUSH_ALLOWANCES: pushActors, + }, + } + + return restrictsPushes } func setForcePushBypassers(protection BranchProtectionRule, data BranchProtectionResourceData, meta interface{}) []string { diff --git a/github/util_v4_consts.go b/github/util_v4_consts.go index 0d073de51d..43dcf00e8d 100644 --- a/github/util_v4_consts.go +++ b/github/util_v4_consts.go @@ -5,24 +5,25 @@ const ( PROTECTION_ALLOWS_FORCE_PUSHES = "allows_force_pushes" PROTECTION_BLOCKS_CREATIONS = "blocks_creations" PROTECTION_DISMISSES_STALE_REVIEWS = "dismiss_stale_reviews" + PROTECTION_FORCE_PUSHES_BYPASSERS = "force_push_bypassers" PROTECTION_IS_ADMIN_ENFORCED = "enforce_admins" + PROTECTION_LOCK_BRANCH = "lock_branch" PROTECTION_PATTERN = "pattern" + PROTECTION_PULL_REQUESTS_BYPASSERS = "pull_request_bypassers" + PROTECTION_PUSH_ALLOWANCES = "push_allowances" PROTECTION_REQUIRED_APPROVING_REVIEW_COUNT = "required_approving_review_count" PROTECTION_REQUIRED_STATUS_CHECK_CONTEXTS = "contexts" PROTECTION_REQUIRES_APPROVING_REVIEWS = "required_pull_request_reviews" PROTECTION_REQUIRES_CODE_OWNER_REVIEWS = "require_code_owner_reviews" PROTECTION_REQUIRES_COMMIT_SIGNATURES = "require_signed_commits" - PROTECTION_REQUIRES_LINEAR_HISTORY = "required_linear_history" PROTECTION_REQUIRES_CONVERSATION_RESOLUTION = "require_conversation_resolution" + PROTECTION_REQUIRES_LINEAR_HISTORY = "required_linear_history" PROTECTION_REQUIRES_STATUS_CHECKS = "required_status_checks" PROTECTION_REQUIRES_STRICT_STATUS_CHECKS = "strict" - PROTECTION_RESTRICTS_PUSHES = "push_restrictions" + PROTECTION_REQUIRE_LAST_PUSH_APPROVAL = "require_last_push_approval" + PROTECTION_RESTRICTS_PUSHES = "restrict_pushes" PROTECTION_RESTRICTS_REVIEW_DISMISSALS = "restrict_dismissals" - PROTECTION_RESTRICTS_REVIEW_DISMISSERS = "dismissal_restrictions" - PROTECTION_FORCE_PUSHES_BYPASSERS = "force_push_bypassers" - PROTECTION_PULL_REQUESTS_BYPASSERS = "pull_request_bypassers" - PROTECTION_LOCK_BRANCH = "lock_branch" - PROTECTION_REQUIRES_LAST_PUSH_APPROVAL = "require_last_push_approval" + PROTECTION_REVIEW_DISMISSAL_ALLOWANCES = "dismissal_restrictions" REPOSITORY_ID = "repository_id" ) diff --git a/website/docs/r/branch_protection.html.markdown b/website/docs/r/branch_protection.html.markdown index a9c1dcc2be..f80d45114d 100644 --- a/website/docs/r/branch_protection.html.markdown +++ b/website/docs/r/branch_protection.html.markdown @@ -11,7 +11,7 @@ Protects a GitHub branch. This resource allows you to configure branch protection for repositories in your organization. When applied, the branch will be protected from forced pushes and deletion. Additional constraints, such as required status checks or restrictions on users, teams, and apps, can also be configured. -Note: for the `push_restrictions` a given user or team must have specific write access to the repository. If specific write access not provided, github will reject the given actor, which will be the cause of terraform drift. +Note: for the `push_allowances` a given user or team must have specific write access to the repository. If specific write access not provided, github will reject the given actor, which will be the cause of terraform drift. ## Example Usage @@ -45,15 +45,17 @@ resource "github_branch_protection" "example" { ] } - push_restrictions = [ - data.github_user.example.node_id, - "/exampleuser", - "exampleorganization/exampleteam", - # you can have more than one type of restriction (teams + users). If you use - # more than one type, you must use node_ids of each user and each team. - # github_team.example.node_id - # github_user.example-2.node_id - ] + restrict_pushes { + push_allowances = [ + data.github_user.example.node_id, + "/exampleuser", + "exampleorganization/exampleteam", + # you can have more than one type of restriction (teams + users). If you use + # more than one type, you must use node_ids of each user and each team. + # github_team.example.node_id + # github_user.example-2.node_id + ] + } force_push_bypassers = [ data.github_user.example.node_id, @@ -97,11 +99,10 @@ The following arguments are supported: * `require_conversation_resolution` - (Optional) Boolean, setting this to `true` requires all conversations on code must be resolved before a pull request can be merged. * `required_status_checks` - (Optional) Enforce restrictions for required status checks. See [Required Status Checks](#required-status-checks) below for details. * `required_pull_request_reviews` - (Optional) Enforce restrictions for pull request reviews. See [Required Pull Request Reviews](#required-pull-request-reviews) below for details. -* `push_restrictions` - (Optional) The list of actor Names/IDs that may push to the branch. Actor names must either begin with a "/" for users or the organization name followed by a "/" for teams. +* `restrict_pushes` - (Optional) Restrict pushes to matching branches. See [Restrict Pushes](#restrict-pushes) below for details. * `force_push_bypassers` - (Optional) The list of actor Names/IDs that are allowed to bypass force push restrictions. Actor names must either begin with a "/" for users or the organization name followed by a "/" for teams. * `allows_deletions` - (Optional) Boolean, setting this to `true` to allow the branch to be deleted. * `allows_force_pushes` - (Optional) Boolean, setting this to `true` to allow force pushes on the branch. -* `blocks_creations` - (Optional) Boolean, setting this to `true` to block creating the branch. * `lock_branch` - (Optional) Boolean, Setting this to `true` will make the branch read-only and preventing any pushes to it. Defaults to `false` ### Required Status Checks @@ -129,6 +130,13 @@ For workflows that use reusable workflows, the pattern is `