-
Notifications
You must be signed in to change notification settings - Fork 762
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
Switch to API v4 (GraphQL) #83
Comments
AFAIK, repository creation (https://developer.github.com/v3/repos/#create 30) and content retrieval (https://developer.github.com/v3/repos/contents/#get-contents 11) are still only supported in the v3 (REST) API. |
We hit our rate limits daily (150 Repos, 30 teams, and ~200 users in the org) and a lot of it is just because of plans fetching repos/teams/team memberships one by one. Switching to graphQL will help alleviate this significantly. |
They're adding additional functionality into the graphQL/v4 api that isn't being added to v3. (traige and maintain permissions for example) @joebowbeer looks like repo creation has been added, didn't see a get-contents equivalent though. |
A simple GraphQL API based cache layer that is used while refreshing any resource will speed up operations by a lot. (One call to fetch a list of all repositories and then cache this for when a repository information is fetched. That way it could use both? |
I've done a fair amount of work on a GraphQL version of the provider. The two major foundational pieces look to be:
This would then allow resources to be uplifted as required or when possible - the GraphQL API is still pretty barebones. I'll start to look at merging the work I've done back in. |
Would it be possible to select the API version for a resource, if v3 and v4 are supported? The v3 API has conditional requests, which allows for read requests to be cached, so it would be good to keep resources at v3 (at least the reads). |
It is certainly possible; for example, in the OpenStack provider many resources have _v1, _v2, and _v3 flavors in order to accommodate users with different needs. This provider could provide _v4 flavors of the resources for users who want to upgrade. |
What would the "upgrade" / "downgrade" path be between the different resources? State migration? Won't this be fairly painful with differences in IDs between the resources? At least when it's the same resource you can include an upgrade function which manages this stuff... |
I would like to keep momentum on this work - who needs to be involved in deciding how this should proceed? |
I've done some preliminary tests on handling the two different APIs using two separate providers. It seems to be the better choice. This way the resources themselves remain consistent and should allow for upgrade functions to work if we start v4 schema versions from say, v100, so there's no possible clash of a resource version between the two providers. Every version can have an update function - hopefully in both directions as well.
How does this sit with everyone? |
PR to help interface v3 and v4 data sources and resources: https://github.com/terraform-providers/terraform-provider-github/pull/383 |
As long as resources all use GitHub's IDs to track objects, then switching between v3 and v4 should be easy. e.g. use |
It would need to be the Relevant issue / PR: https://github.com/terraform-providers/terraform-provider-github/issues/55 & https://github.com/terraform-providers/terraform-provider-github/pull/65 Additionally, not all resources have IDs in v3. Branch protection is one such example. State ID should not be |
What's the process for creating a new official provider, if this is the route we are taking? This is the v4 provider I've pulled together thus far: https://github.com/patrickmarabeas/terraform-provider-github-v4 |
👋 @patrickmarabeas - I am unaware of a process to migrate from one official provider to another but can check with other provider owners if they've had to solve breaking changes in that manner. I have seen discussion around cutting a new major version of this provider over in https://github.com/terraform-providers/terraform-provider-github/issues/354, which would align with the work you've been driving on the GraphQL front. Would a |
So it looks like there is a desire to pick the API being used. It also looks like v3 and v4 APIs are being developed in parallel - so depending on the features you require.... Google has Alignment of either the IDs between the two providers would go a long way to provide easy swapping without needing upgrade/downgrade functions to alter the state ID - BUT unfortunately v3 API seems to be lacking in IDs. I'm not sure how to navigate that hurdle unless it's just accepted that some resources will get recreated. An alternate approach is to feed Implementing v4 into the v3 provider to look up resource IDs where needed could also be a solution. |
My vote goes to this one ☝️ . Being selective about which resources receive the update is a path I have time to support. A quick look at the request logs from an acceptance test suite run shows where low hanging fruit may be: $ cat test.log | grep -A2 REQUEST | grep HTTP/ | cut -d '/' -f 2 | sort | uniq -c
4 meta HTTP
81 organizations
710 orgs
33 projects
684 repos
18 search
35 teams
16 user
25 users Working backwards from the goal of not being rate limited, I support a
As for how we make adoption optional for the provider "github" {
version = "2.3.0"
organization = var.organization
enabled_graphql_resources = ["github_repository"]
} |
Happy to take the above approach @jcudit and start working on this if there's going to be some degree of traction... Please get the following branches merged:
Once these are in I'll look at adding:
|
@jcudit It isn't looking good. I don't see a way to dynamically set the schema resource. Best seems to be checking at the CRUD function level, but that ultimately means the schema has to be the same between APIs. |
@patrickmarabeas understood. I propose we push to merge the changes made so far to unblock other efforts. I will follow up on the PR with next steps for closing it out. We can then open up discussion around a better design to achieve feature flagging. |
Sounds good. Do we really need to increase complexity and provide a long lasting option between APIs? The simplest approach would be having a |
Ah, my inexperience with schema versioning is showing here. I now understand the I can take some time to read further on this front, but currently prefer to merge and investigate flagging in functionality due to the reasons above. |
Another approach would be to release feature versions on branches while they are in "beta". If the user requires multiple features, they would need to include multiple versions of the provider. Could you detail your thinking behind why v4 resources should be opt in? Is this just for beta testing, or is it a long lived option? |
I see it as a beta testing workflow, similar to how the My biggest fear with this change is breaking existing state due to incompatibilities with resource identifiers before and after GraphQL support. I see the adoption of GraphQL as a performance improvement to work around API request rate limiting. I think trading durability for performance is valued by some users but not all and I want to leave an option for the use cases where rate limiting is not a pain point. That being said, there are other ways to resolve my main concern and the uplifting approach with state migration coverage is likely the correct strategy. I appreciate you pausing to explore available approaches. |
Understood. #305 also highlights continued feature discrepancy between v3 and v4. I think it's going to have to be done via feature toggling, but perhaps not so black and white - some resources:
In the case of other resources, e.g. teams (#339 - I anticipate the resource would be similar to that of the data source linked), where the data set is/can be a lot richer via the v4 API - the schema would need to reflect that of the v4 return, culling a lot of the garbage data v3 returns, but also returning nil for many data points if only the v3 API is used. Hopefully that brain dump makes some sense. |
I've done some more reading and feel comfortable with the original approach where we uplift individual resources without feature toggling. The only change I can think to add to our original plan is an intermediate step to address the concern around resource identifiers changing and breaking a user's configuration. At this point we could update all resource identifiers to use global node IDs. The original IDs, if different from the new global IDs, can be archived to a What do you think? Would this extra step make further uplifts easier / safer? Or do you think we should continue with the original plan as-is and initially focus on the resources with feature parity? |
My only concern is the feature differential between APIs - which might fluctuate over time between v3 and v4. What are your thoughts on resources using both APIs to achieve the maximum feature footprint when required? I might draw up a table of We can also use the branch protection uplift as a test bed for this - as it definitely appears to be the edge case in all this. |
💯 on this and your other suggestions. We should harness both, defaulting to Whatever ends up being written will likely be a model for new resources to follow. Keep that in mind as this is an opportunity to DRY things up over time. |
Any news on this? |
Any news on this? This is really hitting us hard. It's really overdue that the provider utilizes the features introduced several years ago. Are there any plans on moving to the GraphQL API? |
@spr-mweber3 are there particular resources you'd like to see the GraphQL API used in? We're using it in a number of places so far. |
@kfcampbell, yes sure. I'm particularly interested in having the resource |
Is that behavior you might be interested in submitting a PR to change? |
@nickfloyd @kfcampbell Sorry for the late response, guys. Unfortunately neither time nor skills provide that I fix this in a PR. Not my area of expertise (yet), but I'm happy to see that it already got picked up with high priority. 💯 If I can be of any assistance in shaping the requirements of this feature or any other way, just let me know. I'd be really grateful if the |
@spr-mweber3 can I talk you into creating an issue for the particular resource(s) you want migrated over to the GraphQL API? I'd like to close this issue as-is, since we're not going to be doing a single PR to cut over every resource the GraphQL API would support. |
The GitHub v4 (GraphQL) api is now official.
Since we're doing large queries across lots of objects in GitHub, using GraphQL will be significantly faster and use up less of the API Rate Limit.
Both reading and writing (mutating) are supported.
The text was updated successfully, but these errors were encountered: