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 <jurgen@space-marine.org>
Co-authored-by: Jeremy Udit <jcudit@github.com>

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}"
+```

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"
+}

examples/repository_org_internal/outputs.tf

@@ -0,0 +1,4 @@
+output internal_repository {
+  description = "Example repository JSON blob"
+  value       = github_repository.internal
+}

examples/repository_org_internal/providers.tf

@@ -0,0 +1,4 @@
+provider github {
+  owner = var.owner
+  token = var.github_token
+}

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
+}

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
 	}
 

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
+}