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 support for GHE #618

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

sochotnicky
Copy link

By providing github-api-url we can enable support for Github Enterprise.
In some cases it might be available for octokit automatically but it's
safer to provide an override.

Copy link
Owner

@kachick kachick left a comment

Choose a reason for hiding this comment

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

Thanks for addressing around this feature!

I don't have experience to use GHE, but the logic and suggestion looks good.
I have reviewed about some CI failures and parameters. 🙏

src/github-api.ts Outdated Show resolved Hide resolved
action.yml Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@@ -9,6 +9,10 @@ inputs:
description: 'The GITHUB_TOKEN secret'
required: true
default: ${{ github.token }}
github-api-url:
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
github-api-url:
github-graphql-url:

I think this will be better to clarify the difference of REST endpoints

Copy link
Author

@sochotnicky sochotnicky Oct 19, 2023

Choose a reason for hiding this comment

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

In this case - octokit graphql actually does "magic" to figure out graphql endpoint from rest endpoint here:
https://github.com/octokit/graphql.js/blob/main/src/graphql.ts#L67

If we pass graphql endpoint as baseUrl it would produce url such as "https://ghe.example.net/graphql/graphql".

So in this case we need to leave it as github-api-url. Or if you'd prefer github-rest-api-url ?

Copy link
Owner

Choose a reason for hiding this comment

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

https://github.com/octokit/graphql.js/blob/9c0643d34f36ed558e55193438d7aa8b031ca43d/src/graphql.ts#L23
https://github.com/octokit/graphql.js/blob/9c0643d34f36ed558e55193438d7aa8b031ca43d/src/graphql.ts#L67-L72

const GHES_V3_SUFFIX_REGEX = /\/api\/v3\/?$/;

  // workaround for GitHub Enterprise baseUrl set with /api/v3 suffix
  // https://github.com/octokit/auth-app.js/issues/111#issuecomment-657610451
  const baseUrl = parsedOptions.baseUrl || request.endpoint.DEFAULTS.baseUrl;
  if (GHES_V3_SUFFIX_REGEX.test(baseUrl)) {
    requestOptions.url = baseUrl.replace(GHES_V3_SUFFIX_REGEX, "/api/graphql");
  }

If we pass graphql endpoint as baseUrl it would produce url such as "https://ghe.example.net/graphql/graphql".

The workaround seems to focus only on the /api/v3 suffix. Did it actually append nested graphql suffix for your URL?

Copy link
Author

Choose a reason for hiding this comment

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

So we have https://ghe.example.com/api/v3 for the api_url. When I use it I end with following error (redacted):

const error2 = new import_request_error.RequestError(toErrorMessage(data), status, {
  RequestError [HttpError]: Not Found
      at main/dist/index.js:18839:26
      at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
      at async Object.next (main/dist/index.js:21920:32)
      at async Function.paginate (main/dist/index.js:21968:26)
      at async getCheckRunSummaries (main/dist/index.js:25927:55)
      at async fetchOtherRunStatus (main/dist/index.js:26018:27)
      at async run (main/dist/index.js:26183:20) {
    status: 404,
    response: {
      url: 'https://ghe.example.com/api/graphql/graphql',
      status: 404,

Copy link
Owner

Choose a reason for hiding this comment

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

#618 (comment)

github.api_url may raise the error, but your ${{ github.graphql_url }} have same suffix? As I understand it, GitHub GraphQL API is v4. (I may not know GHE backgrounds 🙇‍♂️ )

Copy link
Author

Choose a reason for hiding this comment

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

In our GHE instance ${{ github.graphql_url }} resolves to something like: https://ghe.example.net/api/graphql (which is correct endpoint for it). So the octokit graphql takes: https://ghe.example.net/api/v3 and replaces api/v3 with api/graphql. Otherwise it appends graphql (assuming baseURL is api.github.com). Which is why we need to just give octokit ${{ github.api_url }}. It figures out graphql endoint on its own.

Copy link
Owner

Choose a reason for hiding this comment

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

Thanks for sharing! I'm reading how some other actions are handling this ....

https://github.com/google-github-actions/release-please-action/pull/532

@sochotnicky
Copy link
Author

Not sure if there's anything more I should be fixing here.

@kachick
Copy link
Owner

kachick commented Nov 2, 2023

Sorry for my delayed reply 🙇

Your implementation looks good to me 👍 , but I have stacked to determine the input naming github-api-url or others.
We don't currently use REST API here, so I would like to clarify that the input is used for graphql.

A fundamental problem is that I don't have an environment to test GitHub Enterprise Edition...

Without this support octokit always tries to query api.github.com
instead of GitHub Enterprise where it's running
@sochotnicky sochotnicky force-pushed the add-ghe-support branch 2 times, most recently from 6790c36 to 6214189 Compare October 11, 2024 08:11
@sochotnicky
Copy link
Author

I've rebased the changes on latest main (things are a bit simpler now). I'm still interested in getting this merged but if you don't have access to GHE and don't want to merge this support without it I'd prefer if you just rejected the PR so I can make other arrangements.

@kachick
Copy link
Owner

kachick commented Oct 11, 2024

Sorry for the delay in pending the state for a long time.
And thank you for keeping the interest and updating the codebase. 🙏

I have not forgotten this PR, but I could not determine any action or direction regarding it.

Basically, if I cannot maintain the feature, I would exclude it from my personal project.
However, this suggestion is valuable and similar URLs are found in several repositories.
Also, this does not include a huge difference.

So I'm planning to merge this PR in this or next month.
Please give me some days to take review current code again.

@sochotnicky
Copy link
Author

Sorry for the delay in pending the state for a long time. And thank you for keeping the interest and updating the codebase. 🙏

You are an open-source maintainer, providing tools/code for others to reuse for free. You have no obligation to reply to anyone but I appreciate the gesture :-)

I have not forgotten this PR, but I could not determine any action or direction regarding it.

Basically, if I cannot maintain the feature, I would exclude it from my personal project.

That is absolutely a fair point and would be absolutely acceptable reason to reject my PR.

However, this suggestion is valuable and similar URLs are found in several repositories. Also, this does not include a huge difference.

Yeah, I think with your refactoring the changes became much simpler

So I'm planning to merge this PR in this or next month. Please give me some days to take review current code again.

Great, no rush. For what it's worth I've been using this version on our GH enterprise instance for a while without issues. New version fixed a few issues we had with the matrix jobs etc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 🙋‍♂
Development

Successfully merging this pull request may close these issues.

2 participants