From 1fe84bdd77f1457244236ca4ccd09b65366051f3 Mon Sep 17 00:00:00 2001 From: Keegan Campbell Date: Fri, 9 Apr 2021 04:39:22 -0700 Subject: [PATCH] Visibility lifecycle fixes (#746) * visibility is required on create and can not be updated * add an example * update logic for testing if there is an adequate change * add visibility tests * add testing for creation of private-visibility repos - removes testing of `internal` repositories - we encountered 500s from the API with the following error message: > Only organizations associated with an enterprise can set visibility to internal - a compatible testing environment is needed for automated testing - manual testing may be successful when running a local github instance - removes conflicting use of `private` and `visibility` - added `calculateVisibility` to account for the different combinations of visibility-related configuration that the provider expects - testing revealed different behaviour from the API documentation: > The visibility parameter overrides the private parameter when you use both parameters with the nebula-preview preview header. * add testing for creation of private-visibility repos - removes testing of `internal` repositories - we encountered 500s from the API with the following error message: > Only organizations associated with an enterprise can set visibility to internal - a compatible testing environment is needed for automated testing - manual testing may be successful when running a local github instance - removes conflicting use of `private` and `visibility` - added `calculateVisibility` to account for the different combinations of visibility-related configuration that the provider expects - testing revealed different behaviour from the API documentation: > The visibility parameter overrides the private parameter when you use both parameters with the nebula-preview preview header. * Uncomment tests * Remove specific configuration and instead link to Building the Provider documentation * Renamed example directory to match existing pattern Co-authored-by: Jurgen Weber Co-authored-by: Jeremy Udit --- examples/repository_org_internal/README.md | 20 ++ examples/repository_org_internal/main.tf | 5 + examples/repository_org_internal/outputs.tf | 4 + examples/repository_org_internal/providers.tf | 4 + examples/repository_org_internal/variables.tf | 9 + github/resource_github_repository.go | 36 ++- github/resource_github_repository_test.go | 233 ++++++++++++++++++ 7 files changed, 302 insertions(+), 9 deletions(-) create mode 100644 examples/repository_org_internal/README.md create mode 100644 examples/repository_org_internal/main.tf create mode 100644 examples/repository_org_internal/outputs.tf create mode 100644 examples/repository_org_internal/providers.tf create mode 100644 examples/repository_org_internal/variables.tf diff --git a/examples/repository_org_internal/README.md b/examples/repository_org_internal/README.md new file mode 100644 index 0000000000..20c5fa91e5 --- /dev/null +++ b/examples/repository_org_internal/README.md @@ -0,0 +1,20 @@ +# Repository Visibility with Org, type internal + +This demos various repository [visibility settings](https://help.github.com/en/github/administering-a-repository/setting-repository-visibility) for repositories. + +This example will create a repository in the specified `owner` organization. See https://www.terraform.io/docs/providers/github/index.html for details on configuring [`providers.tf`](./providers.tf) accordingly. + +In order to build the provider for use with this example, see [Building the Provider docs](https://github.com/integrations/terraform-provider-github/blob/master/CONTRIBUTING.md#building-the-provider) + +Alternatively, you may use variables passed via command line: + +```console +export GITHUB_OWNER= +export GITHUB_TOKEN= +``` + +```console +terraform apply \ + -var "owner=${GITHUB_OWNER}" \ + -var "github_token=${GITHUB_TOKEN}" +``` diff --git a/examples/repository_org_internal/main.tf b/examples/repository_org_internal/main.tf new file mode 100644 index 0000000000..873613f80c --- /dev/null +++ b/examples/repository_org_internal/main.tf @@ -0,0 +1,5 @@ +resource github_repository internal { + name = "internal-visibility" + description = "A internal-visible repository created by Terraform" + visibility = "internal" +} diff --git a/examples/repository_org_internal/outputs.tf b/examples/repository_org_internal/outputs.tf new file mode 100644 index 0000000000..cdfa0a8ba9 --- /dev/null +++ b/examples/repository_org_internal/outputs.tf @@ -0,0 +1,4 @@ +output internal_repository { + description = "Example repository JSON blob" + value = github_repository.internal +} diff --git a/examples/repository_org_internal/providers.tf b/examples/repository_org_internal/providers.tf new file mode 100644 index 0000000000..0213bde81b --- /dev/null +++ b/examples/repository_org_internal/providers.tf @@ -0,0 +1,4 @@ +provider github { + owner = var.owner + token = var.github_token +} diff --git a/examples/repository_org_internal/variables.tf b/examples/repository_org_internal/variables.tf new file mode 100644 index 0000000000..36108501a9 --- /dev/null +++ b/examples/repository_org_internal/variables.tf @@ -0,0 +1,9 @@ +variable owner { + description = "GitHub owner used to configure the provider" + type = string +} + +variable github_token { + description = "GitHub access token used to configure the provider" + type = string +} diff --git a/github/resource_github_repository.go b/github/resource_github_repository.go index 682220d6f1..1eca113eda 100644 --- a/github/resource_github_repository.go +++ b/github/resource_github_repository.go @@ -237,13 +237,29 @@ func resourceGithubRepository() *schema.Resource { } } +func calculateVisibility(d *schema.ResourceData) string { + + if value, ok := d.GetOk("visibility"); ok { + return value.(string) + } + + if value, ok := d.GetOk("private"); ok { + if value.(bool) { + return "private" + } else { + return "public" + } + } + + return "public" +} + func resourceGithubRepositoryObject(d *schema.ResourceData) *github.Repository { return &github.Repository{ Name: github.String(d.Get("name").(string)), Description: github.String(d.Get("description").(string)), Homepage: github.String(d.Get("homepage_url").(string)), - Private: github.Bool(d.Get("private").(bool)), - Visibility: github.String(d.Get("visibility").(string)), + Visibility: github.String(calculateVisibility(d)), HasDownloads: github.Bool(d.Get("has_downloads").(bool)), HasIssues: github.Bool(d.Get("has_issues").(bool)), HasProjects: github.Bool(d.Get("has_projects").(bool)), @@ -271,11 +287,6 @@ func resourceGithubRepositoryCreate(d *schema.ResourceData, meta interface{}) er repoReq := resourceGithubRepositoryObject(d) owner := meta.(*Owner).name - // Auth issues (403 You need admin access to the organization before adding a repository to it.) - // are encountered when the resources is created with the visibility parameter. As - // resourceGithubRepositoryUpdate is called immediately after, this is subsequently corrected. - repoReq.Visibility = nil - repoName := repoReq.GetName() ctx := context.Background() @@ -462,8 +473,15 @@ func resourceGithubRepositoryUpdate(d *schema.ResourceData, meta interface{}) er repoReq := resourceGithubRepositoryObject(d) - // The endpoint will throw an error if trying to PATCH with a visibility value that is the same - if !d.HasChange("visibility") { + 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 } diff --git a/github/resource_github_repository_test.go b/github/resource_github_repository_test.go index 26348f2495..60caa774a0 100644 --- a/github/resource_github_repository_test.go +++ b/github/resource_github_repository_test.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "log" + "regexp" "strings" "testing" @@ -735,6 +736,229 @@ func TestAccGithubRepositoryPages(t *testing.T) { } +func TestAccGithubRepositoryVisibility(t *testing.T) { + + randomID := acctest.RandStringFromCharSet(5, acctest.CharSetAlphaNum) + + t.Run("creates repos with private visibility", func(t *testing.T) { + + config := fmt.Sprintf(` + resource "github_repository" "private" { + name = "tf-acc-test-visibility-private-%s" + visibility = "private" + } + `, randomID) + + check := resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr( + "github_repository.private", "visibility", + "private", + ), + ) + + 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) + }) + }) + + t.Run("updates repos to private visibility", func(t *testing.T) { + + config := fmt.Sprintf(` + resource "github_repository" "public" { + name = "tf-acc-test-visibility-public-%s" + visibility = "public" + vulnerability_alerts = false + } + `, randomID) + + checks := map[string]resource.TestCheckFunc{ + "before": resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr( + "github_repository.public", "visibility", + "public", + ), + ), + "after": resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr( + "github_repository.public", "visibility", + "private", + ), + ), + } + + 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: checks["before"], + }, + { + Config: reconfigureVisibility(config, "private"), + Check: checks["after"], + }, + }, + }) + } + + 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) + }) + }) + + t.Run("updates repos to public visibility", func(t *testing.T) { + + config := fmt.Sprintf(` + resource "github_repository" "test" { + name = "tf-acc-test-prv-vuln-%s" + visibility = "private" + } + `, randomID) + + checks := map[string]resource.TestCheckFunc{ + "before": resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr( + "github_repository.test", "vulnerability_alerts", + "false", + ), + ), + "after": resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr( + "github_repository.test", "vulnerability_alerts", + "true", + ), + resource.TestCheckResourceAttr( + "github_repository.test", "visibility", + "private", + ), + ), + } + + 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: checks["before"], + }, + { + Config: strings.Replace(config, + `}`, + "vulnerability_alerts = true\n}", 1), + Check: checks["after"], + }, + }, + }) + } + + 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) + }) + }) + + t.Run("updates repos to internal visibility", func(t *testing.T) { + + config := fmt.Sprintf(` + resource "github_repository" "test" { + name = "tf-acc-test-prv-vuln-%s" + visibility = "private" + } + `, randomID) + + checks := map[string]resource.TestCheckFunc{ + "before": resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr( + "github_repository.test", "vulnerability_alerts", + "false", + ), + ), + "after": resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr( + "github_repository.test", "vulnerability_alerts", + "true", + ), + resource.TestCheckResourceAttr( + "github_repository.test", "visibility", + "private", + ), + ), + } + + 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: checks["before"], + }, + { + Config: strings.Replace(config, + `}`, + "vulnerability_alerts = true\n}", 1), + Check: checks["after"], + }, + }, + }) + } + + 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 { meta, err := sharedConfigForRegion(region) if err != nil { @@ -767,3 +991,12 @@ func init() { F: testSweepRepositories, }) } + +func reconfigureVisibility(config, visibility string) string { + re := regexp.MustCompile(`visibility = "(.*)"`) + newConfig := re.ReplaceAllString( + config, + fmt.Sprintf(`visibility = "%s"`, visibility), + ) + return newConfig +}