Add client.WithClient method

[ashi009]

When authenticating with github app creds, we need to attach a tokensource to the http.Client, and provide it to the github.NewClient. However, a github app needs to switch between app cred, app install cred and perhaps user cred. Which means we need to call github.NewClient repeatedly with different http.Client when switching between creds.

This is very painful when using github enterprise, as we need a follow up call c.WithEnterpriseURLs at each callsite. It would be nice if we can add a method to do the necessary config copying when just changing the underlying http.Client.

[gmlewis]

Thank you, @ashi009 . What would the ideal API look like from the user perspective? In other words, could you please write a short code snippet of how you would like it to look like? That would help me understand. Thanks again. 😁💜

[ashi009]

This is what we have today:

// NewAppTokenHTTPClient creates a new http client that performs requests
// authenticated as a GitHub App.
func NewAppTokenHTTPClient(ctx context.Context, privateKeyPEM []byte, appID int64) (*http.Client, error) {
	...
	return oauth2.NewClient(ctx, oauth2.ReuseTokenSource(nil, ts)), nil
}

// NewAppInstallationTokenHTTPClient creates a new http client that performs requests
// authenticated as a GitHub App installation. gc must be a GitHub client authenticated
// as a GitHub App.
func NewAppInstallationTokenHTTPClient(ctx context.Context, gc *github.Client, installationID int64) (*http.Client, error) {
	...
	return oauth2.NewClient(ctx, oauth2.ReuseTokenSource(nil, ts)), nil
}

func main() {
        ctx := context.Background()
	ahc, _ := NewAppTokenHTTPClient(ctx, ...)
	agc := github.NewClient(ahc).WithEnterpriseURLs(..., ...)
	// the following might be in another file/struct, say, handling webhook requests, which sees nothing but the `agc`
	aihc, _ := NewAppInstallationTokenHTTPClient(ctx, agc, ...)
	aigc := github.NewClient(aihc).WithEnterpriseURLs(..., ...)
	// If I need to use user credential, I'll need to do this all over again.
	ugc := github.NewClient(uhc).WithEnterpriseURLs(..., ...)
}

It will be much cleaner to do:

func main() {
	...
	aigc := agc.WithClient(aihc)
	ugc := agc.WithClient(uhc)
}

I was also thinking about using ctx to carry the optional token source, which will make the code even cleaner. Then I realized that the current Client holds the rate limit data which ties to the creds it uses, and it will require lots of refactor.

[gmlewis]

Thank you, @ashi009 for the extra details!

This would be a great PR for any new contributor to this repo or a new Go developer. All contributions are greatly appreciated!

Feel free to volunteer for any issue and the issue can be assigned to you so that others don't attempt to duplicate the work.

Please check out our CONTRIBUTING.md guide to get started. (In particular, please remember to go generate ./... and don't use force-push to your PRs.)

Thank you!

[ashi009]

My understanding is that go-github is removing deps in recent releases. It would be super nice to include gh app cred support, instead of relying on ghinstallation (no offense). To be honest, ghinstallation builds on the go-github api, which means if not using the correct version, people will end up bundling 2 versions in the final binary...

[WillAbides]

I considered adding WithHTTPClient when working on #2904. I ended up not doing it because it could result in unexpected behavior when used after NewTokenClient or Client.WithAuthToken because those modify the http client's Transport to add an auth header.

This wouldn't be an issue for your use case because you wouldn't expect the auth header to be there after replacing the client with another one that handles auth. However, if WithHTTPClient is called with a client that doesn't handle auth, the result would be a possible-unexpected loss of token auth.

Before adding WithHTTPClient we should consider other interfaces that don't have that issue or perhaps changing how Client.WithAuthToken works.

[gmlewis]

My understanding is that go-github is removing deps in recent releases. It would be super nice to include gh app cred support, instead of relying on ghinstallation (no offense). To be honest, ghinstallation builds on the go-github api, which means if not using the correct version, people will end up bundling 2 versions in the final binary...

For the record, ghinstallation is only mentioned in this repo's docs and in an examples folder, in the hopes that it will help people. Note that it does not appear at all in our top-level go.mod, and therefore this repo does not rely on that package as a dependency.

I personally have no experience working with gh app cred, so it sounds to me like the best advice I can give is for you to open up a brand new issue (since I believe this is an orthogonal issue to this one), and we can have any of our contributors work on adding examples of how to use it. Does that sound good to you?

[WillAbides]

@gmlewis I'm not sure this will make a good first issue considering the compatibility issues I mentioned above.

[ashi009]

My understanding is that go-github is removing deps in recent releases. It would be super nice to include gh app cred support, instead of relying on ghinstallation (no offense). To be honest, ghinstallation builds on the go-github api, which means if not using the correct version, people will end up bundling 2 versions in the final binary...

For the record, ghinstallation is only mentioned in this repo's docs and in an examples folder, in the hopes that it will help people. Note that it does not appear at all in our top-level go.mod, and therefore this repo does not rely on that package as a dependency.

I personally have no experience working with gh app cred, so it sounds to me like the best advice I can give is for you to open up a brand new issue (since I believe this is an orthogonal issue to this one), and we can have any of our contributors work on adding examples of how to use it. Does that sound good to you?

Sorry for the misunderstanding. I saw #2895 and #2914 in the recent changes. So I assume adding WithTokenSource(oauth2.TokenSource) will not be an option. If that's still on the table, I guess the most straightforwad option would be use that directly.

But it's definitely a good idea to create another issue to cover the gh app auth mania. A bunch of APIs requires that to work, e.g. checks API. So It's probably a good idea to have a full story supporting those use-cases.

[WillAbides]

I think I have something that will work for this. We could add WithRoundTripper instead of WithClient. The http client would have a Transport that first adds the auth header if configured, then it would call the RoundTripper provided by WithRoundTripper.

Here's a function that uses token auth as well as authenticating as both an app and an installation.

func listAppRepos(ctx context.Context, gheURL, gheUploadURL, authToken, appSlug string, key []byte) ([]string, error) {
	client, err := github.NewClient(nil).WithEnterpriseURLs(gheURL, gheUploadURL)
	if err != nil {
		return nil, err
	}
	client = client.WithAuthToken(authToken)

	app, _, err := client.Apps.Get(ctx, appSlug)
	if err != nil {
		return nil, err
	}

	appsTransport, err := ghinstallation.NewAppsTransport(http.DefaultTransport, app.GetID(), key)
	if err != nil {
		return nil, err
	}

	client = client.WithRoundTripper(appsTransport)

	installations, _, err := client.Apps.ListInstallations(ctx, nil)
	if err != nil {
		return nil, err
	}

	var result []string
	var repos *github.ListRepositories
	for _, inst := range installations {
		client = client.WithRoundTripper(ghinstallation.NewFromAppsTransport(appsTransport, inst.GetID()))
		repos, _, err = client.Apps.ListRepos(ctx, nil)
		if err != nil {
			return nil, err
		}
		for _, repo := range repos.Repositories {
			result = append(result, repo.GetFullName())
		}
	}

	return result, nil
}

[WillAbides]

I would like to work on this PR

[gmlewis]

Thank you, @WillAbides ! It's yours.