-
Notifications
You must be signed in to change notification settings - Fork 33
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
Github enterprise support #158
base: master
Are you sure you want to change the base?
Conversation
df43809
to
ba76f85
Compare
- add a config value spr.githubApiDomain - collect this during spr init - update regex to not expect 'github.com' - generate PR urls correctly Test plan: Manually tested on my companies github enterprise
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.
I have been running this branch for a while and am very happy with the GHES support. Thanks for publishing this.
|
||
let api_base_url; | ||
if github_api_domain == "api.github.com" { | ||
api_base_url = "https://api.github.com/v3/".into() |
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 should be without the /v3/
to make it work for me.
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.
@andrewhamon can you check if this is correct? I haven't tested this PR personally yet, but it looks like this is the non-enterprise case, so we obviously need to not break that too :)
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 doing this and sending the PR. Apologies that it has taken considerable time to get to reviewing it. Please see inlines below.
|
||
let api_base_url; | ||
if github_api_domain == "api.github.com" { | ||
api_base_url = "https://api.github.com/v3/".into() |
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.
@andrewhamon can you check if this is correct? I haven't tested this PR personally yet, but it looks like this is the non-enterprise case, so we obviously need to not break that too :)
config | ||
.get_string("spr.githubApiDomain") | ||
.ok() | ||
.unwrap_or_else(|| "api.github.com".to_string()), |
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.
It seems we're special-casing api.github.com
consistently, almost using it as a sentinel value, as opposed to actually using the value itself. (i.e., every place we read spr.githubApiDomain
we check for api.github.com
and then do something different if that is the value, as opposed to using that value itself.)
So instead, can we make this an Option<string>
, with None
meaning "the default non-enterprise", and Some<x>
meaning "use x
as the enterprise base URL" -- since we are special-casing api.github.com
100% of the time anyway? That seems more consistent with what is actually going on here.
Remove the manual use of reqwest::Client for graphql queries and use octocrab instead. This removes the need to specify the github api url multiple times. We instead leverage the global octocrab instance for which we configure the api url once. This is beneficial for a cleaner implementation of Github Enterprise support (spacedentist#158), as we can configure the base url once.
This commit builds on the work down and feedback posted in [spacedentist#158](spacedentist#158). It also builds on using Octocrab for GraphQL changes in [spacedentist#187](spacedentist#187) It only sets a base_url if we explicitly specify one for Github Enterprise, otherwise it will use the base url from octocrab for Github Cloud.
Github enterprise support
I did not add a CLI flag - it seems like not all options have CLI flags and I don't see much of a use case to change this on-the-fly within a repo. Happy to add it if desired, though.
Test plan:
Manually tested on my companies github enterprise instance