Go-github: Add convenience helper for creating client with custom (enterprise) URL.

[chriskingnet]

I am trying to write a little app that connects to a Github Enterprise instance. I would like to be able to pass in the base enterprise URL, and have it override the default one.

As far as I can see (from the tests and research I have done) the API for Github Enterprise includes all of the regular functionality, plus some more Enterprise-only features (but perhaps someone knows different?). Because of this, just adding the ability to pass in the URL would already be of great benefit.

I have created this issue to discuss, but have also already made an attempt at the changes and will submit a PR, in case this issue is accepted.

[dmitshur]

go-github already has support for GitHub Enterprise. See the exported BaseURL field of Client struct:

https://godoc.org/github.com/google/go-github/github#Client

[chriskingnet]

Hi @shurcooL, thanks for getting back to me. You're right, I totally didn't notice this! I don't have any better argument for doing it my way - I missed this mostly because of my relative inexperience with Go, I think.

I will close the issue - thanks for your work on this lib :)

[dmitshur]

No problem.

I think we can still put up the NewEnterpriseClient from #757 for consideration as an optional helper. It could be rewritten to use the existing API, but accept all URLs and add trailing slash if neccessary. E.g., something like:

// NewEnterpriseClient returns a new GitHub API client with provided
// base URL and upload URL.
// If either URL does not have a trailing slash, one is added automatically.
// If a nil httpClient is provided, http.DefaultClient will be used.
//
// Note that NewEnterpriseClient is a convenience helper only;
// its behavior is equivalent to using NewClient, followed by setting
// the BaseURL and UploadURL fields.
func NewEnterpriseClient(baseURL, uploadURL string, httpClient *http.Client) (*Client, error) {
    base, err := url.Parse(baseURL)
    if err != nil {
        return nil, err
    }
    if !strings.HasSuffix(base.Path, "/") {
        base.Path += "/"
    }
    upload, err := url.Parse(uploadURL)
    if err != nil {
        return nil, err
    }
    if !strings.HasSuffix(upload.Path, "/") {
        upload.Path += "/"
    }
    client := NewClient(httpClient)
    client.BaseURL = base
    client.UploadURL = upload
    return client, nil
}

I'm not completely sure if it's worth adding such a helper, but let's consider/discuss it. What do you think? /cc @gmlewis @elliott-beach

[chriskingnet]

Thanks for the response - I think that sounds good. I would be for such a helper, as I think it helps with clarity and guiding the lib user that overriding the base url with the enterprise URL works (perhaps not clear to those new to the Github API).

If we are in agreement that this helper would be worthwhile, I would be more than happy to alter the existing PR to like you said, and not go the roundabout way I did as I didn't notice the exported fields :)

[dmitshur]

it helps with clarity and guiding the lib user that overriding the base url with the enterprise URL works (perhaps not clear to those new to the Github API).

An alternative way of achieving that could be to expand the documentation of NewClient method to mention that BaseURL/UploadURL can/should be modified for use with GitHub Enterprise instances.

[gmlewis]

Since we are fully committed to supporting Enterprise customers, I'm also fine with adding the new endpoint.

One part of me says "don't add another endpoint that needs support which can't be solved in another simple way"... and that part of me votes for just updating the documentation...

But then another part of me says "hey, it is a simple endpoint and probably won't require a lot of maintenance down the road, and it is a whole lot easier for a new person to see the NewEnterpriseClient in the godocs and immediately say, 'That's what I want!' than to force them to read through more text, as if they don't already have enough to read."

So I think that the latter part of me is winning this argument, but I'm fine to go either way.

[dmitshur]

One part of me says "don't add another endpoint that needs support which can't be solved in another simple way"... and that part of me votes for just updating the documentation...

But then another part of me says "hey, it is a simple endpoint and probably won't require a lot of maintenance down the road, and it is a whole lot easier for a new person to see the NewEnterpriseClient in the godocs and immediately say, 'That's what I want!' than to force them to read through more text, as if they don't already have enough to read."

@gmlewis Agreed, that's my thinking too.

We ended up being quite strict about BaseURL having a trailing slash because there was no good time to do automatic conversions (i.e., doing it per method call didn't feel good). But doing it once at client creation time in NewEnterpriseClient is absolutely perfect.

The main reasons I'm starting to lean in favor of adding NewEnterpriseClient is that it can provide the following value:

• parse URL in the form of a string into *url.URL for you
• take care of missing trailing slash for you

If not those two things, its value would not justify adding it, but they make it more worthwhile IMO.

[chriskingnet]

I agree that while documentation would be sufficient, documenting with code here might be better because of the seemingly small amount of work / maintenance.

• parse URL in the form of a string into *url.URL for you
• take care of missing trailing slash for you

These are great points. I hadn't thought about that at all, but that would be a definite benefit.

One other thing I was thinking further about today was the BaseURL and UploadURL aspect. From the enterprise API documentation, it seemed to me that the same base url is used for both types of API interactions. If that is the case, this would allow us to override both fields while only passing in one parameter - which would in my opinion definitely be a clearer way configuring an enterprise client. However, unless anyone here knows for sure, I will have to do some further research to ensure that is actually the way it works.

[dmitshur]

BaseURL and UploadURL aspect. From the enterprise API documentation, it seemed to me that the same base url is used for both types of API interactions. If that is the case, this would allow us to override both fields while only passing in one parameter - which would in my opinion definitely be a clearer way configuring an enterprise client.

Let's investigate that. If it's always the same URL, that can be yet another benefit. I've never used GHE so I can't be sure.

[chriskingnet]

I have done a lot of searching, but this seems a little tricky to figure out as the documentation isn't specific. https://developer.github.com/enterprise/2.11/v3/repos/releases/#upload-a-release-asset

However I just made the following request to an enterprise api endpoint (in this case https://<host>/api/v3): https://developer.github.com/enterprise/2.11/v3/repos/releases/#get-a-single-release. In the response it returns the upload url, which was https://<host>/api/upload.

I will see if I can ask around if anyone knows if this is configurable, or if it is always like this. If it is we could perhaps detect if /api/v3 has been added to the base url and format as necessary. Or we could just allow two endpoints to be passed in. Any thoughts?

[gmlewis]

Seems to me like keeping two args leaves things flexible, and a comment could be added that they are "typically the same". Your call.

[chriskingnet]

That sounds like a good approach, thanks for the input. I should have some time later, will put together a PR then, see how it looks :)