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

Allows API URL override via environment variable to support GitHub Enterprise #24

Merged
merged 6 commits into from Aug 19, 2021
Merged

Allows API URL override via environment variable to support GitHub Enterprise #24

merged 6 commits into from Aug 19, 2021

Conversation

retiman
Copy link
Contributor

@retiman retiman commented Aug 18, 2021

Fixes #23.

@retiman retiman changed the title add support for apiurl Adds an api_url input to support GitHub Enterprise. Aug 18, 2021
@retiman retiman changed the title Adds an api_url input to support GitHub Enterprise. Adds an api_url input to support GitHub Enterprise Aug 18, 2021
@retiman
Copy link
Contributor Author

retiman commented Aug 18, 2021

I bumped the version to 1.4.0 to avoid potentially breaking existing users.

@retiman retiman marked this pull request as ready for review August 18, 2021 02:14
@peter-evans
Copy link

There is a much simpler way to support GHE in actions. I've done the following in my own actions.

const githubUrl = process.env['GITHUB_SERVER_URL'] || 'https://github.com'

This is also how it's done in the official actions like checkout:
https://github.com/actions/checkout/blob/main/src/url-helper.ts#L22-L29

@retiman
Copy link
Contributor Author

retiman commented Aug 18, 2021

Thanks for the advice Peter. I can't get this to work on my end, though. I see your action is mainly working with the actions toolkit (actions/core in the case you linked); however, this action is leveraging octokit directly. The octokit/auth-app.js library delegates to octokit/endpoint.js, which hard codes api.github.com as the default baseUrl:

https://github.com/octokit/endpoint.js/blob/master/src/defaults.ts#L12

I don't see any place where auth-app.js, request.js, or endpoint.js are reading GITHUB_SERVER_URL or GITHUB_URL to override this value. Do you have any examples where github-app-token is generating a token with the appropriate environment variables set?

@peter-evans
Copy link

For overriding api.github.com to support GHE you can do this:

    const options: OctokitOptions = {}
    if (token) {
      options.auth = `${token}`
    }
    options.baseUrl = process.env['GITHUB_API_URL'] || 'https://api.github.com'
    this.octokit = new Octokit(options)

See: https://github.com/peter-evans/create-pull-request/blob/main/src/github-helper.ts#L27

@retiman retiman changed the title Adds an api_url input to support GitHub Enterprise Allows API URL override via environment variable to support GitHub Enterprise Aug 18, 2021
@retiman
Copy link
Contributor Author

retiman commented Aug 18, 2021

Sure. That simplifies things.

README.md Outdated Show resolved Hide resolved
@peter-evans
Copy link

Nice. LGTM but I'm not the maintainer 😄

@retiman
Copy link
Contributor Author

retiman commented Aug 18, 2021

Nice. LGTM but I'm not the maintainer 😄

Thanks for the help though :)

@tibdex tibdex self-assigned this Aug 18, 2021
@tibdex tibdex assigned retiman and unassigned tibdex Aug 18, 2021
@peter-evans
Copy link

After looking at getOctokit in the github package I'm not sure if this change is necessary. The toolkit already includes this env var lookup for the base URL here: https://github.com/actions/toolkit/blob/f0b00fd201c7ddf14e1572a10d5fb4577c4bd6a2/packages/github/src/internal/utils.ts#L23-L25

@retiman Are you sure this action doesn't work on GHE already?

@retiman
Copy link
Contributor Author

retiman commented Aug 18, 2021

After looking at getOctokit in the github package I'm not sure if this change is necessary. The toolkit already includes this env var lookup for the base URL here: https://github.com/actions/toolkit/blob/f0b00fd201c7ddf14e1572a10d5fb4577c4bd6a2/packages/github/src/internal/utils.ts#L23-L25

@retiman Are you sure this action doesn't work on GHE already?

Yea, I verified it doesn't work if GITHUB_API_URL is present (verified last night based on your suggestion). If that variable is leveraged by octokit things would "just work".

The link you sent me is for the actions toolkit, where that makes more sense. The toolkit code is meant to be run in build agents and the GITHUB_API_URL is expected to be present. Octokit is a general use library, and its consumers are not necessarily running in a GHA runner so there's no guarantee that the var would be present.

Co-authored-by: Thibault Derousseaux <6574550+tibdex@users.noreply.github.com>
@retiman retiman requested a review from tibdex August 18, 2021 19:11
@peter-evans
Copy link

@retiman Let me try and break down the route this action takes to build it's Octokit instance and you'll see that GITHUB_API_URL is already being handled.

getOctokit is called from the @actions/github package of the actions toolkit:
https://github.com/tibdex/github-app-token/blob/main/src/fetch-installation-token.ts#L17

A new GitHub is created from .utils:
https://github.com/actions/toolkit/blob/f0b00fd201c7ddf14e1572a10d5fb4577c4bd6a2/packages/github/src/github.ts#L19

GitHub is a new Octokit instance with defaults set:
https://github.com/actions/toolkit/blob/f0b00fd201c7ddf14e1572a10d5fb4577c4bd6a2/packages/github/src/utils.ts#L20-L23

defaults includes baseUrl, which requires a call to Utils.getApiBaseUrl():
https://github.com/actions/toolkit/blob/f0b00fd201c7ddf14e1572a10d5fb4577c4bd6a2/packages/github/src/utils.ts#L12-L18

getApiBaseUrl() handles GITHUB_API_URL with a fallback:
https://github.com/actions/toolkit/blob/f0b00fd201c7ddf14e1572a10d5fb4577c4bd6a2/packages/github/src/internal/utils.ts#L23-L25

You can see the code exists in the dist minified release of the action:
image

So the Octokit instance used by this action should already have the correct base URL set if GITHUB_API_URL is available in the environment that it's running in.

@retiman
Copy link
Contributor Author

retiman commented Aug 19, 2021

@peter-evans I don't deny this is happening at the actions toolkit level; all the links you gave are code paths into actions/toolkit.

However, when using a non-modified version of this action in GHE, I'm hitting the 404 error on line 15 in fetch-installation-token.ts, before it even gets to getOctokit:

https://github.com/tibdex/github-app-token/blob/main/src/fetch-installation-token.ts#L15

The createAppAuth function in octokit/auth-app.js calls octokit/request.js:

https://github.com/octokit/auth-app.js/blob/master/src/index.ts#L49
https://github.com/octokit/request.js/blob/master/src/index.ts#L7
https://github.com/octokit/request.js/blob/master/src/with-defaults.ts#L15

You'll see in octokit/endpoint.js (which is referenced from with-defaults.ts), the URL is hard coded:

https://github.com/octokit/endpoint.js/blob/master/src/index.ts#L4
https://github.com/octokit/endpoint.js/blob/master/src/defaults.ts

Stepping back a bit, do you believe that the env var should be checked in octokit?

EDIT: grammar

@peter-evans
Copy link

Ah, sorry. 🙏 I was getting confused because I didn't read the code properly and assumed this was about the octokit instance in use, but it's about the createAppAuth call. So in that case your change is necessary. LGTM!

(No I don't think GITHUB_API_URL should be checked in Octokit. As you rightly said, it's a general purpose lib.)

@peter-evans
Copy link

homer

@retiman
Copy link
Contributor Author

retiman commented Aug 19, 2021

Ah, sorry. 🙏 I was getting confused because I didn't read the code properly and assumed this was about the octokit instance in use, but it's about the createAppAuth call. So in that case your change is necessary. LGTM!

(No I don't think GITHUB_API_URL should be checked in Octokit. As you rightly said, it's a general purpose lib.)

No worries, thanks for following up.

Copy link
Owner

@tibdex tibdex left a comment

Choose a reason for hiding this comment

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

Thanks, let's see how this works.

@tibdex tibdex merged commit 516ef60 into tibdex:main Aug 19, 2021
@retiman
Copy link
Contributor Author

retiman commented Aug 19, 2021

Thanks, let's see how this works.

Just tested against GHE; works great.

@retiman retiman deleted the feature/issue-10-github-url branch August 26, 2021 06:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide ability to support GitHub Enterprise
3 participants