feat: Add new gh client implementation#3448
Conversation
stevehipwell
added
Type: Feature
|
👋 Hi! Thank you for this contribution! Just to let you know, our GitHub SDK team does a round of issue and PR reviews twice a week, every Monday and Friday! We have a process in place for prioritizing and responding to your input. Because you are a part of this community please feel free to comment, add to, or pick up any issues/PRs that are labeled with |
stevehipwell
force-pushed
the
new-gh-client
branch
from
69a5c44 to
4e54261
Compare
deiga
left a comment
deiga
left a comment
There was a problem hiding this comment.
This is looking quite nice!
| Elem: &schema.Schema{Type: schema.TypeInt}, | ||
| Optional: true, | ||
| Description: "List of HTTP status codes that should be retried; if not set this uses the provider defaults. This setting only applies when `max_retries` is greater than `0`.", | ||
| Description: "List of HTTP status codes that should be retried; if not set this uses the provider defaults. This setting only applies when `max_retries` is greater than `0`. This is ignored for the REST API when `legacy_client` is `false` since the new client implementation handles the retry logic.", |
There was a problem hiding this comment.
suggestion: Would it make sense to add validation that these aren't set with legacy_client = false?
There was a problem hiding this comment.
Just looking at the code, I don't think this is worth the effort given they have defaults.
| return githubv4.NewClient(client), nil | ||
| } | ||
|
|
||
| return githubv4.NewEnterpriseClient(*opts.GraphQLURL, client), nil |
There was a problem hiding this comment.
question: Is NewEnterpriseClient correct here? Oh, it is, because is not nil, right?
I wonder if it would make sense to swap the condition and the "default" client to make the flow clearer?
There was a problem hiding this comment.
We could do, but we always use the enterprise path. This can change when we remove legacy support.
Signed-off-by: Steve Hipwell <steve.hipwell@gmail.com>
stevehipwell
force-pushed
the
new-gh-client
branch
from
632524d to
ded4e69
Compare
|
@deiga I've addressed most of your comments. |
stevehipwell
force-pushed
the
new-gh-client
branch
from
ce45d1f to
7d0de33
Compare
2 participants
Warning
This is a work in progress. There is a known issue for tests on resources that rely on other resources to mutate them not passing due to internal GitHub logic. These would be fixed by adopting a test strategy where the fixtures are created outside of Terraform.
Resolves #2925
Resolves #3103
Resolves #2709
Resolves #849
Before the change?
After the change?
legacy_client = falseprovider setting)cache_pathprovider settingPull request checklist
Does this introduce a breaking change?
Please see our docs on breaking changes to help!