-
Notifications
You must be signed in to change notification settings - Fork 769
fix: adjust enterprise api url for graphql use case #2180
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
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2180 +/- ##
=========================================
Coverage 85.02% 85.03%
Complexity 2496 2496
=========================================
Files 237 237
Lines 7354 7355 +1
Branches 388 388
=========================================
+ Hits 6253 6254 +1
Misses 871 871
Partials 230 230 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| @Nonnull | ||
| Requester createGraphQLRequest(String query) { | ||
| return createRequest().method("POST") | ||
| .withApiUrl(getApiUrl().replace("/api/v3", "/api")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This changed behavior isn't really tested by any existing test. Because of how it is written, code coverage doesn't catch it. On the plus side, we know this doesn't break the GitHub.com scenario, but we don't have any verification of the enterprise scenario.
Please add a test to the enterprise github tests that calls createGraphQLRequest and verifies the Requester has the new expected /api/graphql URL. You don't have actually make the http call, just check the value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the review @bitwiseman!
I pushed the changes.
Meanwhile, could you please invite me to hub4j-test-org for later PRs that I want to make? I need to run the tests locally for those.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Invited
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a test that exercises the new behavior. Thanks for contributing!
Description
For
GitHub.comthis ishttps://api.github.com/graphql. ForGitHub Enterprise Server, the GraphQL endpoint is at/api/graphql (not /api/v3/graphql).This change makes sure the URL constructed appropriately.
https://docs.github.com/en/enterprise-cloud@latest/graphql/guides/managing-enterprise-accounts#3-setting-up-insomnia-to-use-the-github-graphql-api-with-enterprise-accounts
Before submitting a PR:
@linkJavaDoc entries to the relevant documentation on https://docs.github.com/en/rest .mvn -D enable-ci clean install site "-Dsurefire.argLine=--add-opens java.base/java.net=ALL-UNNAMED"locally. If this command doesn't succeed, your change will not pass CI.main. You will create your PR from that branch.When creating a PR: