Skip to content
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

Add GraphQL Client #331

Merged
merged 4 commits into from
Apr 15, 2020
Merged

Conversation

patrickmarabeas
Copy link
Contributor

@patrickmarabeas patrickmarabeas commented Jan 29, 2020

Initialize a client for Github GraphQL API calls. This will allow for resources and data sources to be uplifted on a case by case basis.

  • Add shurcool/graphqlv4
  • Initialize a GraphQL client (v4client)
  • Rename client to v3client
  • Update client usage to v3client
  • Update vendor files

@ghost ghost added the size/L label Jan 29, 2020
@kpfleming
Copy link
Contributor

I know this will sound really picky, but I'd prefer to see them named 'v3client' and 'v4client'; the API protocol is just an artifact, it's not really the differentiating factor between them the API version is. Imagine what will happen if GitHub releases as V5 API which is also GraphQL based :-)

@patrickmarabeas
Copy link
Contributor Author

Makes sense.

@patrickmarabeas
Copy link
Contributor Author

  • Clients renamed as per @kpfleming request
  • Vendor files updated for CI to pass

@jcudit jcudit added this to the v2.7.0 milestone Apr 2, 2020
@jcudit
Copy link
Contributor

jcudit commented Apr 2, 2020

@patrickmarabeas - I've got this queued to remove conflicts early next week. Happy to merge if you beat me to it 😄

@patrickmarabeas
Copy link
Contributor Author

Was hoping to get to it today but failed miserably.

@patrickmarabeas patrickmarabeas force-pushed the graphql/client branch 3 times, most recently from 74c54cd to 55b7062 Compare April 4, 2020 13:33
@patrickmarabeas
Copy link
Contributor Author

@jcudit rebased and conflicts resolved.

@jcudit
Copy link
Contributor

jcudit commented Apr 4, 2020

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x100 pc=0x1c945c7]

goroutine 15 [running]:
github.com/terraform-providers/terraform-provider-github/github.(*Config).Clients(0xc000019a28, 0x20469a6, 0x9, 0x1debba0, 0x2d84d60)
	/Users/jcudit/terraform-providers/terraform-provider-github/github/config.go:106 +0x5a7
github.com/terraform-providers/terraform-provider-github/github.providerConfigure.func1(0xc00043c700, 0x0, 0xc0002995a0, 0xc00043c700, 0x0)
	/Users/jcudit/terraform-providers/terraform-provider-github/github/provider.go:125 +0x2bc
github.com/hashicorp/terraform-plugin-sdk/helper/schema.(*Provider).Configure(0xc000044e80, 0xc0001b02d0, 0x1e5b4c0, 0xc0001b0240)
	/Users/jcudit/go/pkg/mod/github.com/hashicorp/terraform-plugin-sdk@v1.7.0/helper/schema/provider.go:275 +0xf6
github.com/hashicorp/terraform-plugin-sdk/internal/helper/plugin.(*GRPCProviderServer).Configure(0xc0007b45e8, 0x23b1a60, 0xc0004d1c80, 0xc0004671c0, 0xc0007b45e8, 0xc0004d1c80, 0xc00045abd0)
	/Users/jcudit/go/pkg/mod/github.com/hashicorp/terraform-plugin-sdk@v1.7.0/internal/helper/plugin/grpc_provider.go:487 +0x2e6
github.com/hashicorp/terraform-plugin-sdk/internal/tfplugin5._Provider_Configure_Handler(0x1fed500, 0xc0007b45e8, 0x23b1a60, 0xc0004d1c80, 0xc0001587e0, 0x0, 0x23b1a60, 0xc0004d1c80, 0xc0005a43c0, 0x51)
	/Users/jcudit/go/pkg/mod/github.com/hashicorp/terraform-plugin-sdk@v1.7.0/internal/tfplugin5/tfplugin5.pb.go:3135 +0x23e
google.golang.org/grpc.(*Server).processUnaryRPC(0xc000444840, 0x23c0ea0, 0xc00055af00, 0xc000452600, 0xc000222300, 0x2df05d8, 0x0, 0x0, 0x0)
	/Users/jcudit/go/pkg/mod/google.golang.org/grpc@v1.23.0/server.go:995 +0x466
google.golang.org/grpc.(*Server).handleStream(0xc000444840, 0x23c0ea0, 0xc00055af00, 0xc000452600, 0x0)
	/Users/jcudit/go/pkg/mod/google.golang.org/grpc@v1.23.0/server.go:1275 +0xda6
google.golang.org/grpc.(*Server).serveStreams.func1.1(0xc0007b8070, 0xc000444840, 0x23c0ea0, 0xc00055af00, 0xc000452600)
	/Users/jcudit/go/pkg/mod/google.golang.org/grpc@v1.23.0/server.go:710 +0x9f
created by google.golang.org/grpc.(*Server).serveStreams.func1
	/Users/jcudit/go/pkg/mod/google.golang.org/grpc@v1.23.0/server.go:708 +0xa1
FAIL	github.com/terraform-providers/terraform-provider-github/github	0.524s
?   	github.com/terraform-providers/terraform-provider-github	[no test files]

Caught a bunch of these when running acceptance tests locally ☝️
Are you getting similar results?

@patrickmarabeas
Copy link
Contributor Author

Soz, assumed CI was running acceptance tests.

Tests no longer panicking, but I am getting some failures - most look to be because I have previously run the tests and things are already in the state it is trying to put them into.

The only error looks to be the following, which is related to my addition of adding v3/ to the BaseURL if it does not match https://api.github.com/ - which is to facilitate a) enterprise installs b) the graphql endpoint (graphql is added).

=== RUN   TestProvider_insecure
--- FAIL: TestProvider_insecure (0.07s)
    testing.go:654: Step 1 error: errors during refresh:

        Error: GET https://localhost:64499/v3/orgs/patrickmarabeas-org: 404  []

          on /var/folders/wq/ztvbmnls1f50rsgvn4z4wtdr0000gn/T/tf-test093003891/main.tf line 1:
          (source code not available)

Is it possible to update the mock API URL to include the /v3/ path?

I'll continue to dig into it later tonight if you don't beat me to it.

@patrickmarabeas
Copy link
Contributor Author

I'm encountering the above error (sans v3/) on master - so unsure what I'm missing.

@jcudit
Copy link
Contributor

jcudit commented Apr 6, 2020

--- FAIL: TestProvider_insecure (0.07s)

Adding something like the following to this function should get the test passing:

	mux.HandleFunc("/v3/users/hashibot", testRespondJson(userResponseBody))
	mux.HandleFunc("/v3/users/hashibot/gpg_keys", testRespondJson(gpgKeysResponseBody))
	mux.HandleFunc("/v3/users/hashibot/keys", testRespondJson(keysResponseBody))
	mux.HandleFunc("/v3/orgs/", testRespondJson("{}"))

baseURL := os.Getenv("GITHUB_BASE_URL")

I had to set this in my environment or a panic occurs during TestAccGithubRepository_defaultBranch. Can we set a sane default somewhere or update the acceptance testing documentation to include this variable?


Can't spot much else. I think this one is pretty close to shipping.

This will allow for resources and data sources to be uplifted on a case
by case basis.

* Add shurcool/graphqlv4
* Initialize a GraphQL client (v4client)
* Rename client to v3client
Update acceptance testing documentation to include the exporting of the
GITHUB_BASE_URL environment variable.
@patrickmarabeas
Copy link
Contributor Author

patrickmarabeas commented Apr 12, 2020

I don't think that's it. I get the same error on master. Are you seeing similar errors on your end? Prepending /v3 did not resolve the issue.

  • Added update to README re. GITHUB_BASE_URL
  • Squash / rebased

Is it possible to get acceptance tests running on this PR? I notice they seem to run on some, but not others...

@anGie44
Copy link
Contributor

anGie44 commented Apr 14, 2020

@patrickmarabeas I gotcha! i just added the acceptance test label - let's see if/how they run :)

ah i see the tests erroring out b/c the ENV GITHUB_TOKEN isn't getting set apparently 😞 ..might be related to https://help.github.com/en/actions/configuring-and-managing-workflows/authenticating-with-the-github_token#permissions-for-the-github_token as I've seen the tests run when a PR isn't from a forked repo

@patrickmarabeas
Copy link
Contributor Author

Ahh. of course.

Copy link
Contributor

@jcudit jcudit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I propose we track the failing TestProvider_insecure test elsewhere and move forward with these changes as-is since master looks to be failing similar to how this branch fails. Other feature work can then proceed with the foundation this work lays and we can revisit additional functionality when a better design emerges.

I'll leave this approval around until you confirm this is an acceptable approach and hope to merge by end of week.

@jcudit jcudit merged commit e8af748 into integrations:master Apr 15, 2020
kfcampbell pushed a commit to kfcampbell/terraform-provider-github that referenced this pull request Jul 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants