Open avidspartan1 opened 7 months ago
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?
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:
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.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!
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?
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!
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.
Added two github_repository
tests that leverage the new owner
attribute and exercise the new github_repository
import option - ready for re-review, @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?
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?
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
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
@kfcampbell 🙏🏼
@kfcampbell 👋🏼 anything I can do to get this PR moving?
@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 :tada:
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?
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.
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?
Hey hey - checking in, @kfcampbell. :) Anything I can do for this PR?
Resolves:
1436
1702
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 forgithub_repository
.Before the change?
owner
attribute on eithergithub_repository
orgithub_repository_webhook
resources (the former required in order for the latter's tests to pass).After the change?
owner
for repositories and webhooks.Pull request checklist
repository_webhook
repository
Does this introduce a breaking change?
Please see our docs on breaking changes to help!