From 9f4119631a18dc2b0a02ab520ce4eaf43a02b5b0 Mon Sep 17 00:00:00 2001 From: Keegan Campbell Date: Sun, 18 Apr 2021 06:08:50 -0700 Subject: [PATCH] Visibility bugfix exploration (#761) * Initial commit of bugfix exploration * add test for repository visibility when created by a template * Fix test errors * Simplify visibility logic a bit * Clarify logic a bit more * Cherry-pick commit from @jcudit * Fix linting error * Fix cases switching back and forth between visibility and private flags Co-authored-by: Jeremy Udit --- github/resource_github_repository.go | 78 +++++++++++++++++------ github/resource_github_repository_test.go | 50 +++++++++++++++ 2 files changed, 110 insertions(+), 18 deletions(-) diff --git a/github/resource_github_repository.go b/github/resource_github_repository.go index 4f4853bac3..71aeafd013 100644 --- a/github/resource_github_repository.go +++ b/github/resource_github_repository.go @@ -7,6 +7,7 @@ import ( "log" "net/http" "regexp" + "strings" "github.com/google/go-github/v32/github" "github.com/hashicorp/terraform-plugin-sdk/helper/schema" @@ -291,6 +292,29 @@ func resourceGithubRepositoryCreate(d *schema.ResourceData, meta interface{}) er log.Printf("[DEBUG] Creating repository: %s/%s", owner, repoName) + // determine if repository should be private. assume public to start + isPrivate := false + + // prefer visibility to private flag since private flag is deprecated + privateKeyword, ok := d.Get("private").(bool) + if ok { + isPrivate = privateKeyword + } + + visibility, ok := d.Get("visibility").(string) + if ok { + if visibility == "private" { + isPrivate = true + } + } + + repoReq.Private = github.Bool(isPrivate) + if isPrivate { + repoReq.Visibility = github.String("private") + } else { + repoReq.Visibility = github.String("public") + } + if template, ok := d.GetOk("template"); ok { templateConfigBlocks := template.([]interface{}) @@ -302,11 +326,12 @@ func resourceGithubRepositoryCreate(d *schema.ResourceData, meta interface{}) er templateRepo := templateConfigMap["repository"].(string) templateRepoOwner := templateConfigMap["owner"].(string) + templateRepoReq := github.TemplateRepoRequest{ Name: &repoName, Owner: &owner, Description: github.String(d.Get("description").(string)), - Private: github.Bool(d.Get("private").(bool)), + Private: github.Bool(isPrivate), } repo, _, err := client.Repositories.CreateFromTemplate(ctx, @@ -344,17 +369,15 @@ func resourceGithubRepositoryCreate(d *schema.ResourceData, meta interface{}) er } } - var alerts, private bool + var alerts bool if a, ok := d.GetOk("vulnerability_alerts"); ok { alerts = a.(bool) } - if p, ok := d.GetOk("private"); ok { - private = p.(bool) - } + var createVulnerabilityAlerts func(context.Context, string, string) (*github.Response, error) - if private && alerts { + if isPrivate && alerts { createVulnerabilityAlerts = client.Repositories.EnableVulnerabilityAlerts - } else if !private && !alerts { + } else if !isPrivate && !alerts { createVulnerabilityAlerts = client.Repositories.DisableVulnerabilityAlerts } if createVulnerabilityAlerts != nil { @@ -472,17 +495,8 @@ func resourceGithubRepositoryUpdate(d *schema.ResourceData, meta interface{}) er repoReq := resourceGithubRepositoryObject(d) - if d.HasChange("visibility") { - // The endpoint will throw an error if this repo is being created and the old value is "" - o, n := d.GetChange("visibility") - log.Printf("[DEBUG] Old Value %v New Value %v", o, n) - if o.(string) == "" { - repoReq.Visibility = nil - } - } else { - // The endpoint will throw an error if trying to PATCH with a visibility value that is the same - repoReq.Visibility = nil - } + // handle visibility updates separately from other fields + repoReq.Visibility = nil // The documentation for `default_branch` states: "This can only be set // after a repository has already been created". However, for backwards @@ -547,6 +561,34 @@ func resourceGithubRepositoryUpdate(d *schema.ResourceData, meta interface{}) er } } + if d.HasChange("visibility") { + o, n := d.GetChange("visibility") + repoReq.Visibility = github.String(n.(string)) + log.Printf("[DEBUG] <<<<<<<<<<<<< Updating repository visibility from %s to %s", o, n) + _, _, err = client.Repositories.Edit(ctx, owner, repoName, repoReq) + if err != nil { + if !strings.Contains(err.Error(), "422 Visibility is already private") { + return err + } + } + } else { + log.Printf("[DEBUG] <<<<<<<<<< no visibility update required. visibility: %s", d.Get("visibility")) + } + + if d.HasChange("private") { + o, n := d.GetChange("private") + repoReq.Private = github.Bool(n.(bool)) + log.Printf("[DEBUG] <<<<<<<<<<<<< Updating repository privacy from %v to %v", o, n) + _, _, err = client.Repositories.Edit(ctx, owner, repoName, repoReq) + if err != nil { + if !strings.Contains(err.Error(), "422 Privacy is already set") { + return err + } + } + } else { + log.Printf("[DEBUG] <<<<<<<<<< no privacy update required. private: %v", d.Get("private")) + } + return resourceGithubRepositoryRead(d, meta) } diff --git a/github/resource_github_repository_test.go b/github/resource_github_repository_test.go index 1977e82c9d..2a0087ebdb 100644 --- a/github/resource_github_repository_test.go +++ b/github/resource_github_repository_test.go @@ -1019,6 +1019,56 @@ func TestAccGithubRepositoryVisibility(t *testing.T) { }) }) + t.Run("sets private visibility for repositories created by a template", func(t *testing.T) { + + config := fmt.Sprintf(` + resource "github_repository" "private" { + name = "tf-acc-test-visibility-private-%s" + visibility = "private" + template { + owner = "%s" + repository = "%s" + } + } + `, randomID, testOrganization, "terraform-template-module") + + check := resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr( + "github_repository.private", "visibility", + "private", + ), + resource.TestCheckResourceAttr( + "github_repository.private", "private", + "true", + ), + ) + + 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) { + testCase(t, individual) + }) + + t.Run("with an organization account", func(t *testing.T) { + testCase(t, organization) + }) + }) + } func testSweepRepositories(region string) error {