feat: add repository_webhook and repository owner attributes

[avidspartan1]

Resolves:

• [FEAT]: allow to set repository owner in resource block #1436
• [FEAT]: github_repository select organization to use in Enterprise #1702
• [FEAT]: github_repository_webhook owner attribute #2127

Please note: I'm looking for feedback on the adjustments / additions to the repository_webhook tests. I'm new to Golang and Terraform providers, so I didn't want to go add to the repository tests until these are deemed passable. 😄

This is a continuation of #1832 (thanks, @tdabasinskas!), which implemented the owner attribute for github_repository.

---

Before the change?

• Could not set owner attribute on either github_repository or github_repository_webhook resources (the former required in order for the latter's tests to pass).

After the change?

• Can now explicitly set owner for repositories and webhooks.

Pull request checklist

• Tests for the changes have been added (for bug fixes / features)
  • Tests for repository_webhook
  • Tests for repository
• Docs have been reviewed and added / updated if needed (for bug fixes / features)

Does this introduce a breaking change?

Please see our docs on breaking changes to help!

• No

---

[kfcampbell]

Thank you for picking this up! I like the approach for calculating owner and taking the explicit one (if given) first.

I'm looking for feedback on the adjustments / additions to the repository_webhook tests.

In general, the biggest hangup for me on the tests is the addition of the new required environment variable. Can that be inferred somehow?

[avidspartan1]

Thank you for picking this up! I like the approach for calculating owner and taking the explicit one (if given) first.

Thanks! I'll have to give @tdabasinskas credit for that one - I just rinsed and repeated his approach. 😄

In general, the biggest hangup for me on the tests is the addition of the new required environment variable. Can that be inferred somehow?

I got hung up over this one, as well - the problem is that I'd really like the provider's Owner and the owner specified for the resource, to be different, so that I can confirm the new attribute is truly working as intended by checking the final owner value.

With that requirement, I thought about a few other approaches that do not include the new environment variable:

1. Create a new GitHub organization as part of the test and reference that (so the org will always be different from the provider's Owner). That felt a bit complex for just testing a webhook owner, especially since one of the documented requirements for testing this provider is creating your own org to create/destroy repositories in.
2. Use GITHUB_ORGANIZATION, an already-required environment variable according to the testing docs, to infer the organization. The issue here is that the provider already takes this environment variable as one of the (deprecated) ways to set the overall owner for the provider. If it's both the "resource owner" and the "provider owner", my test for those to be different, fails.
3. Use GITHUB_ORGANIZATION (at the provider level) and GITHUB_OWNER (at the test level) to infer the organization (the former taking precedence for the provider's owner). This method is the most plausible of the three (to me, at least), though it could be a bit confusing for anyone looking at these tests after me: they would need to explicitly set GITHUB_ORGANIZATION to the owner of their token so that it gets picked up by the provider, but then set GITHUB_OWNER to the org they want the repo and webhook owned by for the withOwner tests. It works, but... feels bad. 😆

I'm curious if any of those approaches seems better to you than my current one (a new, explicit environment variable for the owner tests), or if my initial requirement is unnecessary (having the provider's owner and resource owner be different values).

Thanks for taking the time to review the PR!

[kfcampbell]

Thank you for the thoughtful response!

I'm curious if any of those approaches seems better to you than my current one (a new, explicit environment variable for the owner tests), or if my initial requirement is unnecessary (having the provider's owner and resource owner be different values).

None of them do...there's not a lot of appetizing options here. I'm tentatively in favor of using the same owner in nearly all of the integration tests, and then having a single scoped test for the separate owner attribute that requires the different environment variable and setup. That way, at least most tests will require fewer permissions to run. Thoughts?

[avidspartan1]

I'm tentatively in favor of using the same owner in nearly all of the integration tests, and then having a single scoped test for the separate owner attribute that requires the different environment variable and setup. That way, at least most tests will require fewer permissions to run. Thoughts?

I'm down with that approach - I'll update the tests to reflect that and also add an owner-related test for the github_repository resource that uses the same environment variable, if you're good with that. Thanks!

[avidspartan1]

I've fixed up the webhook tests - if you could take a look and let me know if that's what you were thinking, I'd appreciate it! I'll work on getting an owner-specific repository test or two up soon.

[avidspartan1]

Added two github_repository tests that leverage the new owner attribute and exercise the new github_repository import option - ready for re-review, @kfcampbell!

[kfcampbell]

Thanks @avidspartan1! I'm seeing errors on the newly added tests when I run:

    testing.go:705: Step 1 error: Check failed: Check 1/1 error: github_repository.test: Attribute 'primary_language' expected "Go", got ""
=== RUN   TestAccGithubRepositories/creates_and_updates_repositories_with_owner_without_error
=== RUN   TestAccGithubRepositories/creates_and_updates_repositories_with_owner_without_error/with_a_unique_resource-specific_owner

Do these pass for you? Is there setup I'm missing?

[avidspartan1]

Thanks @avidspartan1! I'm seeing errors on the newly added tests when I run:

    testing.go:705: Step 1 error: Check failed: Check 1/1 error: github_repository.test: Attribute 'primary_language' expected "Go", got ""
=== RUN   TestAccGithubRepositories/creates_and_updates_repositories_with_owner_without_error
=== RUN   TestAccGithubRepositories/creates_and_updates_repositories_with_owner_without_error/with_a_unique_resource-specific_owner

Do these pass for you? Is there setup I'm missing?

@kfcampbell, I'm wondering if the failed check you're seeing is the check for the create a repository with go as primary_language test, since the error you're seeing says Check 1/1 (the two new TestAccGithubRepositories tests I added have 6 and 2 checks, respectively, so I would have expected to see Check #/6 or Check #/2 if one of those ran and failed).

Here's how I'm getting the new tests to run:

export GITHUB_TOKEN=<my personal token>
export GITHUB_RESOURCE_OWNER=<my testing org>. # in my case, tf-github-provider-testing-avidspartan1
TF_LOG=DEBUG TF_ACC=1 go test -v ./... -run ^TestAccGithubRepositories
TF_LOG=DEBUG TF_ACC=1 go test -v ./... -run ^TestAccGithubRepositoryWebhook

Now, if I set GITHUB_ORGANIZATION instead of GITHUB_RESOURCE_OWNER, I get the error you see above re: Go as the primary language, since the primary language test is no longer skipped. I also see this error on the main branch of the provider, however. Maybe something has changed with how GitHub calculates the primary language of a repo?

[avidspartan1]

I think that the time it takes for GitHub to figure out the primary language is sometimes longer than the time between the first and second applies in the primary_language test - I've added a Sleep to the test and it seems to be working consistently now

[avidspartan1]

Hey @kfcampbell, just wanted to give this a bump - let me know what else I can do to get this PR where it needs to be

[avidspartan1]

@kfcampbell 🙏🏼

[avidspartan1]

@kfcampbell 👋🏼 anything I can do to get this PR moving?

[kfcampbell]

@avidspartan1 apologies for the delay. My main concern here is around testing, I was struggling to make the new integration tests pass:

    --- FAIL: TestAccGithubRepositories/creates_and_updates_repositories_with_owner_without_error (0.91s)
        --- FAIL: TestAccGithubRepositories/creates_and_updates_repositories_with_owner_without_error/with_a_unique_resource-specific_owner (0.91s)

due to the following error:

2024-04-01T15:42:11.208-0700 [ERROR] sdk.helper_resource: Unexpected error: test_name=TestAccGithubRepositories/creates_and_updates_repositories_with_owner_without_error/with_a_unique_resource-specific_owner test_terraform_path=/usr/bin/terraform test_working_directory=/tmp/plugintest3784674272 test_step_number=1
  error=
  | Error running apply: exit status 1
  | 
  | Error: POST https://api.github.com/orgs/kfcampbell/repos: 404 Not Found []
  | 
  |   with github_repository.test,
  |   on terraform_plugin_test.tf line 3, in resource "github_repository" "test":
  |    3: \t\t\tresource "github_repository" "test" {
  | 

The way I was running these tests:

export GITHUB_TOKEN={my account's (@kfcampbell's) token}
export GITHUB_RESOURCE_OWNER={kfcampbell}
export GITHUB_OWNER={my testing account org, kfcampbell-terraform-provider}
TF_LOG=DEBUG TF_ACC=1 go test -v ./... -run ^TestAccGithubRepositories

I noticed that you had your org set to be the resource owner while presumably your personal account was the provider owner. So I changed my configuration to set the owner of the token and the provider to myself, while the resource owner was set to my testing org (as shown below). The result is passing on the newly-added tests 🎉

export GITHUB_TOKEN={my account's (@kfcampbell's) token}
export GITHUB_RESOURCE_OWNER={my testing account org, kfcampbell-terraform-provider}
export GITHUB_OWNER={kfcampbell}
TF_LOG=DEBUG TF_ACC=1 go test -v ./... -run ^TestAccGithubRepositories

Is there a limitation that the resource owner cannot be an individual and must be an org account?

[avidspartan1]

No worries, @kfcampbell - thanks for taking another go at the tests. 😄 The way I had been testing was to only set the following GITHUB-related environment variables:

GITHUB_TOKEN={my token}
GITHUB_RESOURCE_OWNER={my testing org}

The reason I had the resource owner set to my testing org was because the "default" owner for the provider, unless otherwise set, is automatically set to the token's owner. I think I tried GITHUB_OWNER at some point, but I can't remember what my issue was. Maybe not being able to isolate the new tests?

Either way - if it's working for you with GITHUB_OWNER, great. I don't have a specific reason to not use that env var.

[avidspartan1]

Hey, @kfcampbell! Circling back here. It sounds like we're set with testing, since you were able to get it working by specifying both GITHUB_RESOURCE_OWNER and GITHUB_OWNER. Are there any further changes you're looking for in this PR?