From 05a8875f0a3912d7231b06cfe5dc5d55bbc1dccc Mon Sep 17 00:00:00 2001 From: Keegan Campbell Date: Thu, 13 Oct 2022 16:23:42 -0700 Subject: [PATCH] Revert PR #1020 to undo performance regression as reported in #1328 (#1330) This reverts commit 553785b7bcb82027ee56b675ff4fe0fc1108b289. --- github/resource_github_branch_protection.go | 53 +---- .../resource_github_branch_protection_test.go | 54 ----- .../resource_github_repository_environment.go | 8 +- github/util_v4_branch_protection.go | 219 ++++-------------- .../docs/r/branch_protection.html.markdown | 10 +- 5 files changed, 49 insertions(+), 295 deletions(-) diff --git a/github/resource_github_branch_protection.go b/github/resource_github_branch_protection.go index 4e9c48bae8..73162cdc65 100644 --- a/github/resource_github_branch_protection.go +++ b/github/resource_github_branch_protection.go @@ -154,27 +154,6 @@ func resourceGithubBranchProtectionCreate(d *schema.ResourceData, meta interface if err != nil { return err } - - var reviewIds, pushIds, bypassIds []string - reviewIds, err = getActorIds(data.ReviewDismissalActorIDs, meta) - if err != nil { - return err - } - - pushIds, err = getActorIds(data.PushActorIDs, meta) - if err != nil { - return err - } - - bypassIds, err = getActorIds(data.BypassPullRequestActorIDs, meta) - if err != nil { - return err - } - - data.PushActorIDs = pushIds - data.ReviewDismissalActorIDs = reviewIds - data.BypassPullRequestActorIDs = bypassIds - input := githubv4.CreateBranchProtectionRuleInput{ AllowsDeletions: githubv4.NewBoolean(githubv4.Boolean(data.AllowsDeletions)), AllowsForcePushes: githubv4.NewBoolean(githubv4.Boolean(data.AllowsForcePushes)), @@ -276,11 +255,7 @@ func resourceGithubBranchProtectionRead(d *schema.ResourceData, meta interface{} log.Printf("[DEBUG] Problem setting '%s' in %s %s branch protection (%s)", PROTECTION_REQUIRES_CONVERSATION_RESOLUTION, protection.Repository.Name, protection.Pattern, d.Id()) } - approvingReviews, err := setApprovingReviews(protection, d, meta) - if err != nil { - return err - } - + approvingReviews := setApprovingReviews(protection) err = d.Set(PROTECTION_REQUIRES_APPROVING_REVIEWS, approvingReviews) if err != nil { log.Printf("[DEBUG] Problem setting '%s' in %s %s branch protection (%s)", PROTECTION_REQUIRES_APPROVING_REVIEWS, protection.Repository.Name, protection.Pattern, d.Id()) @@ -292,10 +267,7 @@ func resourceGithubBranchProtectionRead(d *schema.ResourceData, meta interface{} log.Printf("[DEBUG] Problem setting '%s' in %s %s branch protection (%s)", PROTECTION_REQUIRES_STATUS_CHECKS, protection.Repository.Name, protection.Pattern, d.Id()) } - restrictsPushes, err := setPushes(protection, d, meta) - if err != nil { - return err - } + restrictsPushes := setPushes(protection) err = d.Set(PROTECTION_RESTRICTS_PUSHES, restrictsPushes) if err != nil { log.Printf("[DEBUG] Problem setting '%s' in %s %s branch protection (%s)", PROTECTION_RESTRICTS_PUSHES, protection.Repository.Name, protection.Pattern, d.Id()) @@ -316,27 +288,6 @@ func resourceGithubBranchProtectionUpdate(d *schema.ResourceData, meta interface if err != nil { return err } - - var reviewIds, pushIds, bypassIds []string - reviewIds, err = getActorIds(data.ReviewDismissalActorIDs, meta) - if err != nil { - return err - } - - pushIds, err = getActorIds(data.PushActorIDs, meta) - if err != nil { - return err - } - - bypassIds, err = getActorIds(data.BypassPullRequestActorIDs, meta) - if err != nil { - return err - } - - data.PushActorIDs = pushIds - data.ReviewDismissalActorIDs = reviewIds - data.BypassPullRequestActorIDs = bypassIds - input := githubv4.UpdateBranchProtectionRuleInput{ BranchProtectionRuleID: d.Id(), AllowsDeletions: githubv4.NewBoolean(githubv4.Boolean(data.AllowsDeletions)), diff --git a/github/resource_github_branch_protection_test.go b/github/resource_github_branch_protection_test.go index 91f1d22954..e46cf6c53f 100644 --- a/github/resource_github_branch_protection_test.go +++ b/github/resource_github_branch_protection_test.go @@ -380,60 +380,6 @@ func TestAccGithubBranchProtection(t *testing.T) { }) - t.Run("configures branch push restrictions with username", func(t *testing.T) { - - user := fmt.Sprintf("/%s", testOwnerFunc()) - 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.name - pattern = "main" - - push_restrictions = [ - "%s", - ] - - } - `, randomID, user) - - check := resource.ComposeAggregateTestCheckFunc( - resource.TestCheckResourceAttr( - "github_branch_protection.test", "push_restrictions.#", "1", - ), - ) - - 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 force pushes and deletions", func(t *testing.T) { config := fmt.Sprintf(` diff --git a/github/resource_github_repository_environment.go b/github/resource_github_repository_environment.go index 9c460efabf..90e33a29f8 100644 --- a/github/resource_github_repository_environment.go +++ b/github/resource_github_repository_environment.go @@ -134,13 +134,9 @@ func resourceGithubRepositoryEnvironmentRead(d *schema.ResourceData, meta interf for _, r := range pr.Reviewers { switch *r.Type { case "Team": - if r.Reviewer.(*github.Team).ID != nil { - teams = append(teams, *r.Reviewer.(*github.Team).ID) - } + teams = append(teams, *r.Reviewer.(*github.Team).ID) case "User": - if r.Reviewer.(*github.User).ID != nil { - users = append(users, *r.Reviewer.(*github.User).ID) - } + users = append(users, *r.Reviewer.(*github.User).ID) } } d.Set("reviewers", []interface{}{ diff --git a/github/util_v4_branch_protection.go b/github/util_v4_branch_protection.go index 0b17a81727..bf0df51e49 100644 --- a/github/util_v4_branch_protection.go +++ b/github/util_v4_branch_protection.go @@ -3,8 +3,6 @@ package github import ( "context" "fmt" - "log" - "strings" "github.com/hashicorp/terraform-plugin-sdk/helper/schema" "github.com/shurcooL/githubv4" @@ -13,34 +11,27 @@ import ( type Actor struct { ID githubv4.ID Name githubv4.String - Slug githubv4.String -} - -type ActorUser struct { - ID githubv4.ID - Name githubv4.String - Login githubv4.String } type DismissalActorTypes struct { Actor struct { - Team Actor `graphql:"... on Team"` - User ActorUser `graphql:"... on User"` + Team Actor `graphql:"... on Team"` + User Actor `graphql:"... on User"` } } type BypassPullRequestActorTypes struct { Actor struct { - Team Actor `graphql:"... on Team"` - User ActorUser `graphql:"... on User"` + Team Actor `graphql:"... on Team"` + User Actor `graphql:"... on User"` } } type PushActorTypes struct { Actor struct { - App Actor `graphql:"... on App"` - Team Actor `graphql:"... on Team"` - User ActorUser `graphql:"... on User"` + App Actor `graphql:"... on App"` + Team Actor `graphql:"... on Team"` + User Actor `graphql:"... on User"` } } @@ -232,116 +223,60 @@ func branchProtectionResourceData(d *schema.ResourceData, meta interface{}) (Bra return data, nil } -func setDismissalActorIDs(actors []DismissalActorTypes, d *schema.ResourceData, meta interface{}) ([]string, error) { - dismissalActors := make([]string, 0, len(actors)) - data, err := branchProtectionResourceData(d, meta) - if err != nil { - return []string{}, err - } - orgName := meta.(*Owner).name - +func setDismissalActorIDs(actors []DismissalActorTypes) []string { + pushActors := make([]string, 0, len(actors)) for _, a := range actors { - IsID := false - for _, v := range data.ReviewDismissalActorIDs { - if (a.Actor.Team.ID != nil && a.Actor.Team.ID.(string) == v) || (a.Actor.User.ID != nil && a.Actor.User.ID.(string) == v) { - dismissalActors = append(dismissalActors, v) - IsID = true - break - } + if a.Actor.Team != (Actor{}) { + pushActors = append(pushActors, a.Actor.Team.ID.(string)) } - if !IsID { - if a.Actor.Team.Slug != "" { - dismissalActors = append(dismissalActors, orgName+"/"+string(a.Actor.Team.Slug)) - continue - } - if a.Actor.User.Login != "" { - dismissalActors = append(dismissalActors, "/"+string(a.Actor.User.Login)) - } + if a.Actor.User != (Actor{}) { + pushActors = append(pushActors, a.Actor.User.ID.(string)) } } - return dismissalActors, nil + + return pushActors } -func setBypassPullRequestActorIDs(actors []BypassPullRequestActorTypes, d *schema.ResourceData, meta interface{}) ([]string, error) { +func setBypassPullRequestActorIDs(actors []BypassPullRequestActorTypes) []string { bypassActors := make([]string, 0, len(actors)) - data, err := branchProtectionResourceData(d, meta) - if err != nil { - return []string{}, err - } - orgName := meta.(*Owner).name - for _, a := range actors { - IsID := false - for _, v := range data.BypassPullRequestActorIDs { - if (a.Actor.Team.ID != nil && a.Actor.Team.ID.(string) == v) || (a.Actor.User.ID != nil && a.Actor.User.ID.(string) == v) { - bypassActors = append(bypassActors, v) - IsID = true - break - } + if a.Actor.Team != (Actor{}) { + bypassActors = append(bypassActors, a.Actor.Team.ID.(string)) } - if !IsID { - if a.Actor.Team.Slug != "" { - bypassActors = append(bypassActors, orgName+"/"+string(a.Actor.Team.Slug)) - continue - } - if a.Actor.User.Login != "" { - bypassActors = append(bypassActors, "/"+string(a.Actor.User.Login)) - } + if a.Actor.User != (Actor{}) { + bypassActors = append(bypassActors, a.Actor.User.ID.(string)) } } - return bypassActors, nil + + return bypassActors } -func setPushActorIDs(actors []PushActorTypes, d *schema.ResourceData, meta interface{}) ([]string, error) { +func setPushActorIDs(actors []PushActorTypes) []string { pushActors := make([]string, 0, len(actors)) - data, err := branchProtectionResourceData(d, meta) - if err != nil { - return []string{}, err - } - orgName := meta.(*Owner).name - for _, a := range actors { - IsID := false - for _, v := range data.PushActorIDs { - if (a.Actor.Team.ID != nil && a.Actor.Team.ID.(string) == v) || (a.Actor.User.ID != nil && a.Actor.User.ID.(string) == v) || (a.Actor.App.ID != nil && a.Actor.App.ID.(string) == v) { - pushActors = append(pushActors, v) - IsID = true - break - } + if a.Actor.Team != (Actor{}) { + pushActors = append(pushActors, a.Actor.Team.ID.(string)) } - if !IsID { - if a.Actor.Team.Slug != "" { - pushActors = append(pushActors, orgName+"/"+string(a.Actor.Team.Slug)) - continue - } - if a.Actor.User.Login != "" { - pushActors = append(pushActors, "/"+string(a.Actor.User.Login)) - continue - } - if a.Actor.App != (Actor{}) { - pushActors = append(pushActors, a.Actor.App.ID.(string)) - } + if a.Actor.User != (Actor{}) { + pushActors = append(pushActors, a.Actor.User.ID.(string)) + } + if a.Actor.App != (Actor{}) { + pushActors = append(pushActors, a.Actor.App.ID.(string)) } } - return pushActors, nil + + return pushActors } -func setApprovingReviews(protection BranchProtectionRule, d *schema.ResourceData, meta interface{}) (interface{}, error) { +func setApprovingReviews(protection BranchProtectionRule) interface{} { if !protection.RequiresApprovingReviews { - return nil, nil + return nil } dismissalAllowances := protection.ReviewDismissalAllowances.Nodes - dismissalActors, err := setDismissalActorIDs(dismissalAllowances, d, meta) - if err != nil { - return nil, err - } + dismissalActors := setDismissalActorIDs(dismissalAllowances) bypassPullRequestAllowances := protection.BypassPullRequestAllowances.Nodes - bypassPullRequestActors, err := setBypassPullRequestActorIDs(bypassPullRequestAllowances, d, meta) - if err != nil { - return nil, err - } - + bypassPullRequestActors := setBypassPullRequestActorIDs(bypassPullRequestAllowances) approvalReviews := []interface{}{ map[string]interface{}{ PROTECTION_REQUIRED_APPROVING_REVIEW_COUNT: protection.RequiredApprovingReviewCount, @@ -353,7 +288,7 @@ func setApprovingReviews(protection BranchProtectionRule, d *schema.ResourceData }, } - return approvalReviews, nil + return approvalReviews } func setStatusChecks(protection BranchProtectionRule) interface{} { @@ -371,16 +306,14 @@ func setStatusChecks(protection BranchProtectionRule) interface{} { return statusChecks } -func setPushes(protection BranchProtectionRule, d *schema.ResourceData, meta interface{}) ([]string, error) { +func setPushes(protection BranchProtectionRule) []string { if !protection.RestrictsPushes { - return nil, nil + return nil } pushAllowances := protection.PushAllowances.Nodes - pushActors, err := setPushActorIDs(pushAllowances, d, meta) - if err != nil { - return nil, err - } - return pushActors, nil + pushActors := setPushActorIDs(pushAllowances) + + return pushActors } func getBranchProtectionID(repoID githubv4.ID, pattern string, meta interface{}) (githubv4.ID, error) { @@ -433,71 +366,3 @@ func getBranchProtectionID(repoID githubv4.ID, pattern string, meta interface{}) return nil, fmt.Errorf("could not find a branch protection rule with the pattern '%s'.", pattern) } - -func getActorIds(data []string, meta interface{}) ([]string, error) { - var actors []string - for _, v := range data { - id, err := getNodeIDv4(v, meta) - if err != nil { - return []string{}, err - } - log.Printf("[DEBUG] Retrieved node ID for user/team : %s - node ID : %s", v, id) - actors = append(actors, id) - } - - return actors, nil -} - -// Given a string that is either a username or team slug, return the -// node id of the user or team it is referring to. Team slugs must be provided -// with the organization name as prefix (Ex.: exampleorg/exampleteam). Usernames -// must be provided with the "/" prefix otherwise getNodeIDv4 assumes that -// the provided string is a node ID. -func getNodeIDv4(userOrSlug string, meta interface{}) (string, error) { - orgName := meta.(*Owner).name - ctx := context.Background() - client := meta.(*Owner).v4client - - if strings.HasPrefix(userOrSlug, orgName+"/") { - var queryTeam struct { - Organization struct { - Team struct { - ID string - } `graphql:"team(slug: $slug)"` - } `graphql:"organization(login: $organization)"` - } - teamName := strings.TrimPrefix(userOrSlug, orgName+"/") - variablesTeam := map[string]interface{}{ - "slug": githubv4.String(teamName), - "organization": githubv4.String(orgName), - } - - err := client.Query(ctx, &queryTeam, variablesTeam) - if err != nil { - return "", err - } - log.Printf("[DEBUG] Retrieved node ID for team %s. ID is %s", userOrSlug, queryTeam.Organization.Team.ID) - return queryTeam.Organization.Team.ID, nil - } else if strings.HasPrefix(userOrSlug, "/") { - // The "/" prefix indicates a username - var queryUser struct { - User struct { - ID string - } `graphql:"user(login: $user)"` - } - userName := strings.TrimPrefix(userOrSlug, "/") - variablesUser := map[string]interface{}{ - "user": githubv4.String(userName), - } - - err := client.Query(ctx, &queryUser, variablesUser) - if err != nil { - return "", err - } - log.Printf("[DEBUG] Retrieved node ID for user %s. ID is %s", userOrSlug, queryUser.User.ID) - return queryUser.User.ID, nil - } else { - // If userOrSlug does not contain the team or username prefix, assume it is a node ID - return userOrSlug, nil - } -} diff --git a/website/docs/r/branch_protection.html.markdown b/website/docs/r/branch_protection.html.markdown index c6310c0d58..a2b6d8dddf 100644 --- a/website/docs/r/branch_protection.html.markdown +++ b/website/docs/r/branch_protection.html.markdown @@ -38,15 +38,11 @@ resource "github_branch_protection" "example" { dismissal_restrictions = [ data.github_user.example.node_id, github_team.example.node_id, - "/exampleuser", - "exampleorganization/exampleteam", ] } push_restrictions = [ data.github_user.example.node_id, - "/exampleuser", - "exampleorganization/exampleteam", # limited to a list of one type of restriction (user, team, app) # github_team.example.node_id ] @@ -84,7 +80,7 @@ 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. +* `push_restrictions` - (Optional) The list of actor IDs that may push to the branch. * `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. @@ -102,8 +98,8 @@ The following arguments are supported: * `dismiss_stale_reviews`: (Optional) Dismiss approved reviews automatically when a new commit is pushed. Defaults to `false`. * `restrict_dismissals`: (Optional) Restrict pull request review dismissals. -* `dismissal_restrictions`: (Optional) 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. -* `pull_request_bypassers`: (Optional) 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. +* `dismissal_restrictions`: (Optional) The list of actor IDs with dismissal access. If not empty, `restrict_dismissals` is ignored. +* `pull_request_bypassers`: (Optional) The list of actor IDs that are allowed to bypass pull request requirements. * `require_code_owner_reviews`: (Optional) Require an approved review in pull requests including files with a designated code owner. Defaults to `false`. * `required_approving_review_count`: (Optional) Require x number of approvals to satisfy branch protection requirements. If this is specified it must be a number between 0-6. This requirement matches GitHub's API, see the upstream [documentation](https://developer.github.com/v3/repos/branches/#parameters-1) for more information.