-
Notifications
You must be signed in to change notification settings - Fork 504
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
✨ Enable GitHub Enterprise Server (GHES) support #2788
Conversation
Signed-off-by: GitHub <noreply@github.com>
Stale pull request message |
I'm going to work on this again later this week. Not stale. |
@raghavkaul could you help review this PR whenever its ready? |
Yes, happy to review whenever this is ready. |
Signed-off-by: GitHub <noreply@github.com>
Tested against GHES 3.8.1 today and it works: the field in the GraphQL search exists. Two things left to discuss:
|
Tagging @raghavkaul for visiblity. |
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.
Thanks!
@@ -126,7 +128,7 @@ func (client *Client) InitRepo(inputRepo clients.Repo, commitSHA string, commitD | |||
|
|||
// URI implements RepoClient.URI. | |||
func (client *Client) URI() string { | |||
return fmt.Sprintf("github.com/%s/%s", client.repourl.owner, client.repourl.repo) | |||
return fmt.Sprintf("%s/%s/%s", client.repourl.host, client.repourl.owner, client.repourl.repo) |
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.
Can you please write a test for this?
@@ -259,8 +261,33 @@ func CreateGithubRepoClientWithTransport(ctx context.Context, rt http.RoundTripp | |||
httpClient := &http.Client{ | |||
Transport: rt, | |||
} | |||
client := github.NewClient(httpClient) | |||
graphClient := githubv4.NewClient(httpClient) | |||
|
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.
Can you please include tests for these?
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.
@naveensrinivasan , can you give me some pointers / help on where to get started? (not a Go programmer, so testing in Go is new to me).
I see a generic setup for creating the clients during the tests here: clients/githubrepo/githubrepo_suite_test.go, and another generic mock here clients/mockclients/repo_client.go.
Stale pull request message |
Stale pull request message |
Still want to work on this Stale bot! Looking for the time to do so 😄 |
Stale pull request message |
The error messages were not obvious to me: go run main.go --repo https://github.ibm.com/<myorg>/<myproject>
2023/06/05 08:05:42 error during command execution: RunScorecard: repo unreachable: Get "/api/v3/repos/<myorg>/<myproject>": internal error: innerTransport.RoundTrip: internal error: innerTransport.RoundTrip: unsupported protocol scheme "" Could this message remind people that you expect GITHUB_API_URL=https://api.github.ibm.com go run main.go --repo https://github.ibm.com/<myorg>/<myproject>
Error: RunScorecard: repo unreachable: GET https://api.github.ibm.com/repos/<myorg>/<myproject>: 401 Must authenticate to access this API. [] Could this message include the usual one GITHUB_AUTH_TOKEN=<mytoken> GITHUB_API_URL=https://api.github.ibm.com go run main.go --repo https://github.ibm.com/<myorg>/<myproject>
Error: RunScorecard: internal error: ListCommits:error during graphqlHandler.setup: internal error: githubv4.Query: non-200 OK status code: 404 Not Found body: "{\"message\":\"Not Found\",\"documentation_url\":\"https://docs.github.com/enterprise-server@3.3/rest\"}" I gave up before solving this one. Does it want the enterprise URL instead of docs.github.com? How do I provide that? |
Stale pull request message |
Missed the feedback from @esnible, will try to address that. |
} | ||
// trim trailing slash to prevent issues with the graphql client | ||
githubSERVERURL = strings.TrimSuffix(githubSERVERURL, "/") | ||
githubGRAPHQLURL := fmt.Sprintf("%s/api/graphql", githubSERVERURL) |
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 line didn't work for me. To run the scorecards, I used GITHUB_API_URL=https://api.github.ibm.com
and changed this line to
githubGRAPHQLURL := fmt.Sprintf("%s/graphql", githubSERVERURL)
Using "%s/api/graphql" caused 404s, using /graphql allowed the scorecard to generate. I only have access to my own company's GHE, so I don't know if you should always use /graphql or if the path needs to be parameterized for different configurations.
Using the current HEAD I was able to I had incorrectly tried
That confused me because
In terms of functionality the code in HEAD is great. |
Stale pull request message |
I've validated with v4.11.0 and it works against our internal GHES environment, so I am ready to close this issue. Thanks for adding the support! |
All the thanks goes to you and Niket! |
What kind of change does this PR introduce?
Support GHES by using the Enterprise client for the GitHub calls.
What is the current behavior?
Which issue(s) this PR fixes
Special notes for your reviewer
There is one error that needs to be fixed, since this returns a non-zero exit code at the moment, hence the
Draft
status.I expect this is coming from our GHES instance (3.7) that does not support this field yet. If I remove the field requireLastPushApproval all checks run normally, so we might need to check the GHES version and include/exclude this field based on that.
Does this PR introduce a user-facing change?
Not for existing users, as it did not support GHES before. No changes needed by existing users.
Discussion
I am using
os.Getenv("GITHUB_API_URL")
to pick up the API url for the server, as this is where I'd expect this to run from an automation context. We could pick up the url from thescorecard --repo github.corp.net/nv35/myrepo
command, but that means wiring up all the places from front to back to pick this up and pass it through and then update CreateGithubRepoClient calls everywhere.