-
Notifications
You must be signed in to change notification settings - Fork 732
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add repository_webhook and repository owner attributes #2128
base: main
Are you sure you want to change the base?
feat: add repository_webhook and repository owner attributes #2128
Conversation
Signed-off-by: Tomas Dabasinskas <[email protected]> Took 3 hours 5 minutes
Signed-off-by: Tomas Dabasinskas <[email protected]> Took 13 minutes
Co-authored-by: Keegan Campbell <[email protected]>
…and_webhook_owners
…hub into add_repo_and_webhook_owners
Thank you for picking this up! I like the approach for calculating owner and taking the explicit one (if given) first.
In general, the biggest hangup for me on the tests is the addition of the new required environment variable. Can that be inferred somehow? |
Thanks! I'll have to give @tdabasinskas credit for that one - I just rinsed and repeated his approach. 😄
I got hung up over this one, as well - the problem is that I'd really like the provider's Owner and the With that requirement, I thought about a few other approaches that do not include the new environment variable:
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!
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 down with that approach - I'll update the tests to reflect that and also add an |
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. |
…_repository owner attribute
Added two |
Thanks @avidspartan1! I'm seeing errors on the newly added tests when I run:
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 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 |
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 |
…/terraform-provider-github into add_repo_and_webhook_owners
…hub into add_repo_and_webhook_owners
…hub into add_repo_and_webhook_owners
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 |
…hub into add_repo_and_webhook_owners
@kfcampbell 🙏🏼 |
…hub into add_repo_and_webhook_owners
…hub into add_repo_and_webhook_owners
…/terraform-provider-github into add_repo_and_webhook_owners
…hub into add_repo_and_webhook_owners
@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:
due to the following error:
The way I was running these tests:
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 🎉
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
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 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 |
Hey hey - checking in, @kfcampbell. :) Anything I can do for this PR? |
Resolves:
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!