Visibility

[patrickmarabeas]

Adds the additional visibility parameter allowing for enterprise
accounts to set the repository to be only internally visible.

NOTE: This is dependant on #424 and should be rebased on master once this has been merged.

[anGie44]

@patrickmarabeas this looks great! when you get a chance you can also now rebase to use the updated go-github lib

[patrickmarabeas]

Comments resolved, and commits rebased @anGie44.

[anGie44]

awesome, thanks for pushing up changes @patrickmarabeas! do you mind adding test coverage for the attribute as well? forgot to mention it when I made my initial comments..otherwise changes LGTM 👍

[patrickmarabeas]

@anGie44 I've included mention of the property in the tests, but not sure how much more can be done since testing is only done within public orgs... This looks to have the same degree of "testing" as the private property now. Any additional suggestions?

[anGie44]

@patrickmarabeas thank you! running the tests locally tho, I realized you'll have to also add the Visibility attribute in the Check functions like:

testAccCheckGithubRepositoryAttributes(&repo, &testAccGithubRepositoryExpectedAttributes{
      Visibility: "public",
      ...
}

re: more test coverage, the only edge case i considered "testing" would be to set conflicting params (e.g. visibility=public and private=true) to ensure the visibility param overrides but i don't think it'd add much value as I don't see it being a typical practitioner's user error.

[anGie44]

@patrickmarabeas thank you! running the tests locally tho, I realized you'll have to also add the Visibility attribute in the Check functions like:

testAccCheckGithubRepositoryAttributes(&repo, &testAccGithubRepositoryExpectedAttributes{
      Visibility: "public",
      ...
}

re: more test coverage, the only edge case i considered "testing" would be to set conflicting params (e.g. visibility=public and private=true) to ensure the visibility param overrides but i don't think it'd add much value as I don't see it being a typical practitioner's user error.

review needed

[patrickmarabeas]

@anGie44

Added visibility to the Check functions.

I don't think testing that edge case should be necessary, as that's just testing the API, not the provider.

[anGie44]

thanks so much @patrickmarabeas! changes and test results look good 🚀

Output of (GithubRepository) acceptance tests:

--- PASS: TestAccGithubRepository_hasProjects (6.15s)
--- PASS: TestAccGithubRepository_templates (6.90s)
--- PASS: TestAccGithubRepository_basic (12.63s)
--- PASS: TestAccGithubRepository_defaultBranch (12.70s)
--- PASS: TestAccGithubRepository_archive (12.79s)
--- PASS: TestAccGithubRepository_archiveUpdate (14.82s)
--- PASS: TestAccGithubRepository_createFromTemplate (17.30s)
--- PASS: TestAccGithubRepository_autoInitForceNew (30.04s)
--- PASS: TestAccGithubRepository_topics (33.02s)

[anGie44]

Note: rolling this back as the param will override the private attribute by default (even if omitted from config per https://github.com/terraform-providers/terraform-provider-github/issues/451); we should probably consider overriding only if a user provides a value for visibility @patrickmarabeas -- similar to how the API treats this param; apologies if you might have to submit a new PR after I reverted?

[patrickmarabeas]

Ah true. Would removing the default be enough? That should result in a nil setting.

[patrickmarabeas]

@anGie44 see #454