Visibility bugfix exploration

[kfcampbell]

The following code appears to fix the bug, although it's heavily reliant on printline debugging and may not be the permanent solution we desire.

Terraform configuration used for testing:

terraform {
  required_providers {
    github = {
      source  = "integrations/github"
      version = "4.8.0"
    }
  }
  required_version = ">= 0.14"
}

locals {
  owner    = "kfcampbell-terraform-provider"
  repo     = "tf-test-repo-issue-758-013"
  template = "terraform-template-module"
}

provider "github" {
  owner = local.owner
}

resource "github_repository" "repo" {
  name        = local.repo
  description = "Github Provider Test"

  visibility = "private"

  template {
    owner      = local.owner
    repository = local.template
  }
}

[jcudit]

Added a test in 222fc66 if you want to cherry-pick it in. Our fix seems to have tripped a subset of existing tests though 🤔

18:25:39 [jcudit:~/integrations/terraform-provider-github] test/pull/761+* 7m44s ± TF_ACC=1  go test -v   ./... -run ^TestAccGithubRepositoryVisibility
?       github.com/terraform-providers/terraform-provider-github        [no test files]
=== RUN   TestAccGithubRepositoryVisibility
=== RUN   TestAccGithubRepositoryVisibility/creates_repos_with_private_visibility
=== RUN   TestAccGithubRepositoryVisibility/creates_repos_with_private_visibility/with_an_anonymous_account
    resource_github_repository_test.go:773: anonymous account not supported for this operation
=== RUN   TestAccGithubRepositoryVisibility/creates_repos_with_private_visibility/with_an_individual_account
    provider_utils.go:55: GITHUB_TOKEN and GITHUB_OWNER environment variables should be set
    provider_utils.go:65: Skipping TestAccGithubRepositoryVisibility/creates_repos_with_private_visibility/with_an_individual_account which requires individual mode
=== RUN   TestAccGithubRepositoryVisibility/creates_repos_with_private_visibility/with_an_organization_account
    testing.go:654: Step 0 error: errors during apply:
        
        Error: PATCH https://api.github.com/repos/terraformtesting/tf-acc-test-visibility-private-mrqhu: 422 Visibility is already private. []
        
          on /var/folders/t1/p9cyhv6s2wg4g8s_x6zkpz_m0000gn/T/tf-test281982068/main.tf line 2:
          (source code not available)
        
        
=== RUN   TestAccGithubRepositoryVisibility/updates_repos_to_private_visibility
=== RUN   TestAccGithubRepositoryVisibility/updates_repos_to_private_visibility/with_an_anonymous_account
    resource_github_repository_test.go:828: anonymous account not supported for this operation
=== RUN   TestAccGithubRepositoryVisibility/updates_repos_to_private_visibility/with_an_individual_account
    provider_utils.go:55: GITHUB_TOKEN and GITHUB_OWNER environment variables should be set
    provider_utils.go:65: Skipping TestAccGithubRepositoryVisibility/updates_repos_to_private_visibility/with_an_individual_account which requires individual mode
=== RUN   TestAccGithubRepositoryVisibility/updates_repos_to_private_visibility/with_an_organization_account
=== RUN   TestAccGithubRepositoryVisibility/updates_repos_to_public_visibility
=== RUN   TestAccGithubRepositoryVisibility/updates_repos_to_public_visibility/with_an_anonymous_account
    resource_github_repository_test.go:888: anonymous account not supported for this operation
=== RUN   TestAccGithubRepositoryVisibility/updates_repos_to_public_visibility/with_an_individual_account
    provider_utils.go:55: GITHUB_TOKEN and GITHUB_OWNER environment variables should be set
    provider_utils.go:65: Skipping TestAccGithubRepositoryVisibility/updates_repos_to_public_visibility/with_an_individual_account which requires individual mode
=== RUN   TestAccGithubRepositoryVisibility/updates_repos_to_public_visibility/with_an_organization_account
    testing.go:654: Step 0 error: errors during apply:
        
        Error: PATCH https://api.github.com/repos/terraformtesting/tf-acc-test-prv-vuln-mrqhu: 422 Visibility is already private. []
        
          on /var/folders/t1/p9cyhv6s2wg4g8s_x6zkpz_m0000gn/T/tf-test304551719/main.tf line 2:
          (source code not available)
        
        
=== RUN   TestAccGithubRepositoryVisibility/updates_repos_to_internal_visibility
=== RUN   TestAccGithubRepositoryVisibility/updates_repos_to_internal_visibility/with_an_anonymous_account
    resource_github_repository_test.go:948: anonymous account not supported for this operation
=== RUN   TestAccGithubRepositoryVisibility/updates_repos_to_internal_visibility/with_an_individual_account
    provider_utils.go:55: GITHUB_TOKEN and GITHUB_OWNER environment variables should be set
    provider_utils.go:65: Skipping TestAccGithubRepositoryVisibility/updates_repos_to_internal_visibility/with_an_individual_account which requires individual mode
=== RUN   TestAccGithubRepositoryVisibility/updates_repos_to_internal_visibility/with_an_organization_account
    testing.go:654: Step 0 error: errors during apply:
        
        Error: PATCH https://api.github.com/repos/terraformtesting/tf-acc-test-prv-vuln-mrqhu: 422 Visibility is already private. []
        
          on /var/folders/t1/p9cyhv6s2wg4g8s_x6zkpz_m0000gn/T/tf-test582203889/main.tf line 2:
          (source code not available)
        
        
=== RUN   TestAccGithubRepositoryVisibility/sets_private_visibility_for_repositories_created_by_a_template
=== RUN   TestAccGithubRepositoryVisibility/sets_private_visibility_for_repositories_created_by_a_template/with_an_anonymous_account
    resource_github_repository_test.go:998: anonymous account not supported for this operation
=== RUN   TestAccGithubRepositoryVisibility/sets_private_visibility_for_repositories_created_by_a_template/with_an_individual_account
    provider_utils.go:55: GITHUB_TOKEN and GITHUB_OWNER environment variables should be set
    provider_utils.go:65: Skipping TestAccGithubRepositoryVisibility/sets_private_visibility_for_repositories_created_by_a_template/with_an_individual_account which requires individual mode
=== RUN   TestAccGithubRepositoryVisibility/sets_private_visibility_for_repositories_created_by_a_template/with_an_organization_account
--- FAIL: TestAccGithubRepositoryVisibility (32.04s)
    --- FAIL: TestAccGithubRepositoryVisibility/creates_repos_with_private_visibility (5.50s)
        --- SKIP: TestAccGithubRepositoryVisibility/creates_repos_with_private_visibility/with_an_anonymous_account (0.00s)
        --- SKIP: TestAccGithubRepositoryVisibility/creates_repos_with_private_visibility/with_an_individual_account (0.00s)
        --- FAIL: TestAccGithubRepositoryVisibility/creates_repos_with_private_visibility/with_an_organization_account (5.50s)
    --- PASS: TestAccGithubRepositoryVisibility/updates_repos_to_private_visibility (9.79s)
        --- SKIP: TestAccGithubRepositoryVisibility/updates_repos_to_private_visibility/with_an_anonymous_account (0.00s)
        --- SKIP: TestAccGithubRepositoryVisibility/updates_repos_to_private_visibility/with_an_individual_account (0.00s)
        --- PASS: TestAccGithubRepositoryVisibility/updates_repos_to_private_visibility/with_an_organization_account (9.79s)
    --- FAIL: TestAccGithubRepositoryVisibility/updates_repos_to_public_visibility (4.68s)
        --- SKIP: TestAccGithubRepositoryVisibility/updates_repos_to_public_visibility/with_an_anonymous_account (0.00s)
        --- SKIP: TestAccGithubRepositoryVisibility/updates_repos_to_public_visibility/with_an_individual_account (0.00s)
        --- FAIL: TestAccGithubRepositoryVisibility/updates_repos_to_public_visibility/with_an_organization_account (4.68s)
    --- FAIL: TestAccGithubRepositoryVisibility/updates_repos_to_internal_visibility (4.82s)
        --- SKIP: TestAccGithubRepositoryVisibility/updates_repos_to_internal_visibility/with_an_anonymous_account (0.00s)
        --- SKIP: TestAccGithubRepositoryVisibility/updates_repos_to_internal_visibility/with_an_individual_account (0.00s)
        --- FAIL: TestAccGithubRepositoryVisibility/updates_repos_to_internal_visibility/with_an_organization_account (4.82s)
    --- PASS: TestAccGithubRepositoryVisibility/sets_private_visibility_for_repositories_created_by_a_template (7.25s)
        --- SKIP: TestAccGithubRepositoryVisibility/sets_private_visibility_for_repositories_created_by_a_template/with_an_anonymous_account (0.00s)
        --- SKIP: TestAccGithubRepositoryVisibility/sets_private_visibility_for_repositories_created_by_a_template/with_an_individual_account (0.00s)
        --- PASS: TestAccGithubRepositoryVisibility/sets_private_visibility_for_repositories_created_by_a_template/with_an_organization_account (7.25s)
FAIL
FAIL    github.com/terraform-providers/terraform-provider-github/github 32.624s
FAIL

[kfcampbell]

Thank you for providing the test! So they have...what I'm seeing is when we create a repo, we immediately also call Update on that repo. The Update function is somehow picking up an old state visibility value of "" and a new state visibility value of "private". When it tries to set a new visibility value of private, the GitHub API complains that it's already set to private.

I'll need to look into this further to try to figure out the discrepancy between the local and GitHub states here.

[kfcampbell]

The more I work with this, the more I think the visibility not being updated is expected and we should revert this PR.

The workflow of creating and immediately updating a release seems to introduce an expected change of the visibility value from "" to whatever it was set to. Our code in the past attempted to set the value earlier, which resulted in the 422 patch errors. This line was an attempt to deal with that issue, but doing so breaks the case in which you'd switch from using visibility to private (or vice versa), as well as the initial case of creating a private repository, which is the big scary one.

I'm struggling to find changes which can satisfy your new test case as well as the existing ones. I don't understand why between the initial create/update on v4.6 or earlier, the visibility shows as changing from "" to whatever value it was set to. If I could understand why that occurs, perhaps that would lead the way to correcting this behavior.

@jcudit please let me know if you want to pair on this at some point tomorrow!

[jcudit]

@kfcampbell take a look at 37d3ff8 which seems to have gotten the tests passing.

The approach here decouples updating visibility from the rest of the fields that get updated by the single call to client.Repositories.Edit. We now have a second call to client.Repositories.Edit to specifically handle visibility and allow for receiving the 422 Visibility is already ... class of errors, which are harmless. We spend more of our API quota here, but it makes for handling visibility much easier now that it is isolated.

Feel free to cherry-pick this in and play around. If it seems like an accurate approach, lets get this PR prepped to go out with the next release.

[kfcampbell]

Thanks a bunch @jcudit! I've tested this a couple different ways, and it works with the visibility flag exactly as we'd expect.

There's a little bit of trouble with the private boolean flag. Setting it to true or false works as expected the first run, but after that I am unable to flip the private flag.

I've added 30c988c which duplicates the handling you added for visibility except with privacy. After that update, I am able to make all my manual test cases pass:

• starting a repo with visibility public and switching it to private
• starting a repo with visibility private and switching it to public
• starting a repo with visibility private and switching private to false (removing visibility)
• starting a repo with visibility public and switching private to true (removing visibility)
• starting a repo with private true and switching visibility to public (removing private)
• starting a repo with private false and switching visibility to private (removing private)

We'll pay a little bit of an extra tax in calls, but I think it's worth it to match the previous behavior. What do you think?

I've marked this pull request as "ready for review" now since I'm happy with the test cases.

[jcudit]

Looking good! Thanks for running through all those scenarios manually. Will get this out shortly.