github/provider: introduce owner #464

anGie44 · 2020-05-17T17:53:18Z

Continuing the work done by @elislusarczyk in #96 to support individual github accounts for repos

Changes here address last review comments in #96 :

transition off of GITHUB_ORGANIZATION to GITHUB_OWNER (across acctests as well)
check whether the owner is org and if not, return a user-friendly error (#checkOrganization inutil.go)
deprecate organization in provider.go
skip acctests where organization is needed
add IsOrganization to Owner struct

Output of acceptance tests with owner set as Github User:

--- PASS: TestEtagTransport (0.00s)
--- PASS: TestRateLimitTransport_abuseLimit_get (0.00s)
--- PASS: TestRateLimitTransport_abuseLimit_post (0.00s)
--- PASS: TestRateLimitTransport_abuseLimit_post_error (0.00s)
--- PASS: TestAccValidateTeamIDFunc (0.00s)
--- PASS: TestAccGithubUtilRole_validation (0.00s)
--- PASS: TestAccGithubUtilTwoPartID (0.00s)
--- PASS: TestAccGithubReleaseDataSource_invalidRetrieveMethodReturnsError (0.03s)
--- PASS: TestAccGithubReleaseDataSource_fetchByTagNoTagReturnsError (1.57s)
--- PASS: TestAccGithubReleaseDataSource_fetchByIdWithNoIdReturnsError (1.62s)
--- PASS: TestAccGithubUserDataSource_noMatchReturnsError (1.72s)
--- PASS: TestAccGithubReleaseDataSource_fetchByLatestNoReleaseReturnsError (2.01s)
--- PASS: TestAccGithubMembershipDataSource_noMatchReturnsError (2.06s)
--- PASS: TestAccGithubIpRangesDataSource_existing (8.22s)
--- PASS: TestAccGithubUserGpgKey_basic (11.04s)
--- PASS: TestAccGithubUserSshKey_basic (11.17s)
--- PASS: TestAccGithubUserDataSource_existing (12.14s)
--- PASS: TestAccProvider_individual (12.56s)

* remaining tests are skipped

jcudit · 2020-05-19T00:08:37Z

Gave this one a skim and am 👍 on the approach. It is a large diff so I plan on reading it over the next few days. Excited to see this 🚢 !

jcudit · 2020-05-20T11:56:21Z

github/provider.go

 			"organization": {
 				Type:        schema.TypeString,
 				Optional:    true,
 				DefaultFunc: schema.EnvDefaultFunc("GITHUB_ORGANIZATION", nil),
 				Description: descriptions["organization"],
+				Deprecated:  "Use owner field (or GITHUB_OWNER ENV variable)",


jcudit · 2020-05-20T12:16:28Z

website/docs/index.html.markdown

-* `individual`: (Optional) Run outside an organization.  When `individual` is true, the provider will run outside
-  the scope of an organization. Defaults to `false`.
-
-* `anonymous`: (Optional) Authenticate without a token.  When `anonymous` is true, the provider will not be able to


We should call out this removal explicitly when we update the CHANGELOG for the release this goes out in.

why this was removed and not added to the docs ?, I had to come here to actually find out why, a note could have been usefull for people using the anonymous flag

Damn, yes, it took me 2 hours to find out, and now I'm hardcoding the old plugin version. I need anonymous functionality to just pull the GitHub API IPs. That should never require tokens.

The CHANGELOG has been retroactively updated to help save time for anyone else affected.

Thanks for being quick to call this out.

@MarcoDalco open a bug report to make sure such kind of requests are possible? The advantage of token is that it probably raises from GitHub API limits, but I haven't checked. A list of resources that one can use without token would be handy.

@abitrolly https://api.github.com/meta?q=hooks "You can add the IP address we use when delivering hooks to your server's allow list. To ensure that you're always checking the right IP address, you can use the /meta endpoint to find the address we use." https://developer.github.com/v3/guides/best-practices-for-integrators/ . Having to periodically update the whitelisted IPs is not great, but it's what seems to be suggested, at the moment. Then we could implement step 3. Currently we don't, but it would be good, but from a security perspective, in order to reduce the workload of reverse proxies, to reduce the impact of DDOS attacks, you should filter by public IP (step 2) before checking the secret, which happens after the TLS handshake (step 3).

abitrolly · 2020-06-09T07:10:37Z

Looks like owner is not needed at all - only token matters https://developer.github.com/v4/guides/forming-calls/#authenticating-with-graphql

For example GitLab doesn't use any kind of username for API access https://github.com/terraform-providers/terraform-provider-gitlab/blob/74c5685c69a9926c39c6beccebc89e29217494b0/gitlab/config.go#L14

jcudit · 2020-06-09T18:23:11Z

@abitrolly this is good news, but may cause us to take a step back and re-evaluate how best to accomplish things. We are currently trying to leverage the v3 API and is unclear to me whether relying on the token is available outside of v4. My aim is to read more but welcome a more thorough explanation of what could be changed.

jcudit · 2020-06-09T18:25:48Z

@anGie44 - I've gotten this branch into a strange state by fixing conflicts using the web UI 😕.

Also, I am now realizing that local test runs with the configured user account are incompatible:

--- SKIP: TestAccGithubRepositoryWebhook_secret (0.50s)
    resource_github_repository_webhook_test.go:73: Skipping because GITHUB_OWNER "github-terraform-test-user" is a user, not an organization.
PASS
ok  	github.com/terraform-providers/terraform-provider-github/github	0.867s

Should a non-organization user be able to run acceptance tests? My original assumption was that owner was an abstraction of organization and personal-account. Will give this another read through to better understand things, but please clarify if I've misunderstood.

anGie44 · 2020-06-10T06:52:54Z

Yeah ideally we'd want an owner set with an individual user to be able to run all tests but that would require that we remove the checkOrganization method calls in a lot of the resources/data_sources and also adjust acctests where certain values are only available for organizations vs. users (e.g. branch protection, repo collaborator permissions) ..i remember going about this but held back as that would be a lot to review in one go. instead I was thinking of partitioning out changes like in #465 by resource and/or datasource.

emmahsax · 2020-06-25T21:56:45Z

Is there a new Terraform provider version with these changes yet? I'm running into the same issue:

module.example_repo.github_repository.repo: Creating...

Error: This resource requires GitHub organization to be set on the provider.

anGie44 · 2020-06-25T22:03:40Z

hi @emmasax4 👋 ! Not yet, but will be part of the next provider milestone which is the release of v2.9.0 😃

emmahsax · 2020-06-25T22:44:09Z

@anGie44 Awesome! Do we have any idea yet of when that next release will happen?

mariux · 2020-06-29T15:21:07Z

we were facing some issues.. will dig deeper before re posting my deleted post... sorry for the noise

anGie44 · 2020-06-29T15:31:43Z

@mariux no worries! I believe you should still be able to source your token from an environment variable and initialize the provider as usual however maybe we could reconsider moving this back to Optional @jcudit ? at one point the token attribute was required and then changed to Optional to support the anonymous OAuth, though now removed in this version.

abitrolly · 2020-06-29T15:52:40Z

I was going to ask if the next version should be 3.0.0 because of potential compatibility break, but got lazy. :D

jcudit · 2020-06-29T15:54:38Z

😄 was considering a major version bump as well. Hoping the community is forthcoming with bug reports so we can mop them up during v2.9.1.

mariux · 2020-06-29T16:24:48Z

Seems to be only terraform validate to be affected.. will get back, once confirmed.

mariux · 2020-06-29T17:21:10Z

I can now confirm that with provider 2.8.0 terraform validate runs fine without a token and with 2.9.0 terraform validate needs a token to run.

specifying any token (token does not need to be valid) via GITHUB_TOKEN environment variable fixes it.

I am not sure if this is worth to be called a full-blown regression as we can easily fix this in our CI by providing the required env variable. (we run automated tests for our open source github modules)

Should I open a bug report anyway?

abitrolly · 2020-06-29T18:28:10Z

@mariux what could you do with anonymous GitHub provider before?

jcudit · 2020-06-29T18:50:22Z

Should I open a bug report anyway?

@mariux yes, please. Capturing this is valuable.

jamengual · 2020-06-29T18:54:58Z

github/provider.go

@@ -97,41 +87,33 @@ func init() {
 		"token": "The OAuth token used to connect to GitHub. " +
 			"If `anonymous` is false, `token` is required.",


Anonymous is mention here although it has been completely removed

I can now confirm that with provider 2.8.0 terraform validate runs fine without a token and with 2.9.0 terraform validate needs a token to run.

specifying any token (token does not need to be valid) via GITHUB_TOKEN environment variable fixes it.

I am not sure if this is worth to be called a full-blown regression as we can easily fix this in our CI by providing the required env variable. (we run automated tests for our open source github modules)

Should I open a bug report anyway?

same thing here, we have many automated tests now failing

scottb-isp · 2020-06-30T01:08:51Z

I just started receiving this Warning which seems spurious because I'm not using or declaring Organization - I already use Owner. Is this Warning always emitted intentionally?

Warning: "organization": [DEPRECATED] Use owner field (or GITHUB_OWNER ENV variable)

My codebuild github config looks like this

action {
  category         = "Source"
  ~ configuration    = {
    "Branch"               = "***my branch***"
    + "OAuthToken"           = "*******"
    "Owner"                = "***my github company org***"
    "PollForSourceChanges" = "false"
    "Repo"                 = "********removed********"
  }

Initializing provider plugins...
- Checking for available provider plugins...
- Downloading plugin for provider "github" (hashicorp/github) 2.9.0...
- Downloading plugin for provider "aws" (hashicorp/aws) 2.68.0...

mariux · 2020-06-30T08:16:18Z

@mariux what could you do with anonymous GitHub provider before?

We provide a module and run automated tests for static analysis and unit testing. The static analysis runs tools like tflint and terraform validate. terraform validate started failing with 2.9.0 if no token is provided. As this is a module we do not configure the provider at all but just have a version constraint defined:

terraform {
  required_version = "~> 0.12.9"

  required_providers {
    github = "~> 2.4"
  }
}

ghost added the size/XXL label May 17, 2020

anGie44 linked an issue May 17, 2020 that may be closed by this pull request

Feature Request: Add support for individuals #371

Closed

anGie44 marked this pull request as ready for review May 17, 2020 18:01

anGie44 mentioned this pull request May 17, 2020

github/repository*: support individual user for resource management (limited to authenticated user) #465

Merged

jcudit approved these changes May 20, 2020

View reviewed changes

jcudit added this to the v2.9.0 milestone Jun 3, 2020

anGie44 force-pushed the ap_support_user_repos branch from 63ee66e to 86336ec Compare June 10, 2020 04:38

anGie44 added 5 commits June 17, 2020 13:50

fix merge conflicts and updates based on review

fa6bf79

rever variabe name change and update provider attribute description

024b4e6

discover authenticated user to populate owner.name

49c082d

remove env var reference

67ec96c

remove org var reference

3e1f2ce

anGie44 force-pushed the ap_support_user_repos branch from c268d2e to 3e1f2ce Compare June 17, 2020 17:50

update to Owner reference

7392526

jcudit merged commit 9dd0b20 into master Jun 17, 2020

anGie44 deleted the ap_support_user_repos branch June 17, 2020 21:08

jcudit mentioned this pull request Jun 19, 2020

provider: Add the possibility to create repositories for non-organization accounts #96

Closed

abitrolly mentioned this pull request Jun 19, 2020

Rebased #96: Create repos for non-organizations #322

Closed

mariux mentioned this pull request Jun 29, 2020

README.md Markdown link typo fix mineiros-io/terraform-github-team#20

Merged

jamengual reviewed Jun 29, 2020

View reviewed changes

glenwinters mentioned this pull request Jun 30, 2020

Anonymous access flag deprecation? #502

Closed

mariux mentioned this pull request Jun 30, 2020

Breaking behavior in 2.9.0 release: terraform validate fails because of missing token #503

Closed

fmasuhr mentioned this pull request Jun 30, 2020

raise error if organization can not be validated babbel/terraform-provider-github#22

Merged

anGie44 mentioned this pull request Jul 1, 2020

Support for github_actions_secret for personal repo #422

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

github/provider: introduce owner #464

github/provider: introduce owner #464

anGie44 commented May 17, 2020 •

edited

Loading

jcudit commented May 19, 2020

jcudit May 20, 2020

jcudit May 20, 2020

jamengual Jun 29, 2020 •

edited

Loading

MarcoDalco Jul 1, 2020

jcudit Jul 1, 2020

abitrolly Jul 2, 2020

MarcoDalco Jul 3, 2020 •

edited

Loading

abitrolly commented Jun 9, 2020

jcudit commented Jun 9, 2020

jcudit commented Jun 9, 2020

anGie44 commented Jun 10, 2020

emmahsax commented Jun 25, 2020

anGie44 commented Jun 25, 2020

emmahsax commented Jun 25, 2020

mariux commented Jun 29, 2020

anGie44 commented Jun 29, 2020

abitrolly commented Jun 29, 2020

jcudit commented Jun 29, 2020

mariux commented Jun 29, 2020 •

edited

Loading

mariux commented Jun 29, 2020

abitrolly commented Jun 29, 2020

jcudit commented Jun 29, 2020

jamengual Jun 29, 2020

jamengual Jun 29, 2020

scottb-isp commented Jun 30, 2020

mariux commented Jun 30, 2020

		@@ -97,41 +87,33 @@ func init() {
		"token": "The OAuth token used to connect to GitHub. " +
		"If `anonymous` is false, `token` is required.",

github/provider: introduce owner #464

github/provider: introduce owner #464

Conversation

anGie44 commented May 17, 2020 • edited Loading

jcudit commented May 19, 2020

jcudit May 20, 2020

Choose a reason for hiding this comment

jcudit May 20, 2020

Choose a reason for hiding this comment

jamengual Jun 29, 2020 • edited Loading

Choose a reason for hiding this comment

MarcoDalco Jul 1, 2020

Choose a reason for hiding this comment

jcudit Jul 1, 2020

Choose a reason for hiding this comment

abitrolly Jul 2, 2020

Choose a reason for hiding this comment

MarcoDalco Jul 3, 2020 • edited Loading

Choose a reason for hiding this comment

abitrolly commented Jun 9, 2020

jcudit commented Jun 9, 2020

jcudit commented Jun 9, 2020

anGie44 commented Jun 10, 2020

emmahsax commented Jun 25, 2020

anGie44 commented Jun 25, 2020

emmahsax commented Jun 25, 2020

mariux commented Jun 29, 2020

anGie44 commented Jun 29, 2020

abitrolly commented Jun 29, 2020

jcudit commented Jun 29, 2020

mariux commented Jun 29, 2020 • edited Loading

mariux commented Jun 29, 2020

abitrolly commented Jun 29, 2020

jcudit commented Jun 29, 2020

jamengual Jun 29, 2020

Choose a reason for hiding this comment

jamengual Jun 29, 2020

Choose a reason for hiding this comment

scottb-isp commented Jun 30, 2020

mariux commented Jun 30, 2020

anGie44 commented May 17, 2020 •

edited

Loading

jamengual Jun 29, 2020 •

edited

Loading

MarcoDalco Jul 3, 2020 •

edited

Loading

mariux commented Jun 29, 2020 •

edited

Loading