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

plugin: support GitHub Enterprise Server as plugin source #1751

Merged
merged 6 commits into from
May 4, 2023

Conversation

bendrucker
Copy link
Member

GitHub Enterprise Server is a private installation of GitHub. It provides a compatible API, but on a hostname other than github.com. This PR removes the requirement that plugin sources begin with github.com and assumes any other hostname is a GHES server hostname. If it's not, the client would likely receive a 404, which hopefully is enough "validation." But if needed, we could make a request an look for a header to explicitly fail with "this server is not GitHub."

I factored out the request logs into a logging transport to avoid the need to to reverse engineer URLs. Otherwise, client.BaseURL.JoinPath is the next best way to generate a URL relative to the client.

Still thinking about how to test this. The only reasonable way would seem to be to run a mock server using httptest and then pass its URL, or at least the hostname part, as the SourceHost. I believe this is always localhost on a random port, which technically isn't expected in SourceHost, but should work in practice right now. However, given the amount of boilerplate involved in mocking out the responses from GitHub, we could potentially just merge this and let users try it, knowing that we at least have regression testing for github.com.

Closes #1643

@bendrucker bendrucker requested a review from wata727 April 26, 2023 00:09
Copy link
Member

@wata727 wata727 left a comment

Choose a reason for hiding this comment

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

Nice 👍

@wata727
Copy link
Member

wata727 commented Apr 26, 2023

For testing, I'm a little wondering if we should use a mock server. A mock that returns complicated responses can make tests unreliable and obscure the original intent of tests.

As far as this implementation is concerned, the only difference between GitHub.com and GHES is the behavior of newGitHubClient. In this case, an integration test against GitHub.com might be sufficient.

@bendrucker
Copy link
Member Author

Yeah definitely don't want to remove the existing internet-dependent tests, maybe just have a dedicated test for GHES. I guess I can add a unit test against the GitHub client function to exercise some of the hostname logic. Otherwise right now there's no coverage against that branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Support installing plugins from GitHub Enterprise
2 participants