Expose the token of the installation client

[hermanbanken]

The go-githubapp strives to enable easy setup of GitHub Apps. For several GitHub Apps there is a use case to checkout the git repository, next to the existing business of acting upon the GitHub API. Especially for more complex use cases, having access to the working tree and other repository state than the Pull Request diff is crucial.

This PR is a draft, as I am still working on the example. However, to speed up the discussion on whether this should be included I'm already creating the PR now.

The use case I'm specifically looking at is using the Access Token of the installation that is already retrieved & cached to clone the target repository. This is easily done using go-git, but is harder to do using the existing go-githubapp. This PR makes it much much simpler, and the example is intended to demonstrate how to do it exactly.

[palantirtech]

Thanks for your interest in palantir/go-githubapp, @hermanbanken! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request.

[jamestoyer]

@hermanbanken I'm not sure this is required. You can already get an installation token:

func getToken() string {
	owner := "repo-owner"
	appClient, _ := h.NewAppClient()
	installation, _ := getInstallationClient(ctx, appClient, owner)
	var repoIDs []int64
	token, _, err := appClient.Apps.CreateInstallationToken(ctx, installation.ID, &github.InstallationTokenOptions{
		RepositoryIDs: repoIDs,
	})
	return token
}

func getInstallationClient(ctx context.Context, appClient *github.Client, owner string) (githubapp.Installation, error) {
	installations := githubapp.NewInstallationsService(appClient)
	installation, err := installations.GetByOwner(ctx, owner)
	if err != nil {
		return installation, errors.Wrapf(err, "unable to get installation for '%s'", owner)
	}

	return installation, nil
}

Where h is a githubapp.ClientCreator

This is the method we use internally to get the token to check out repositories.

[jamestoyer]

Actually, on closer inspection I see what you're trying to do here. It would make things more simple

[hermanbanken]

I’m trying to prevent another token being created. No need to do that roundtrip twice right?

[hermanbanken]

The fact that github.Client exposes “Do” is a hint to me that they intent on giving some freedom in what you do using this token.

In theory one hacky alternative would be to spin up a localhost server and capture the token from that by using client.Do. But that is so hacky I’m proposing this constructive solution.

People can just ignore the second returned value (TokenSource) if they don’t need it.

[bluekeyes]

Thanks for proposing this. After some thought, I have two points I'd like to discuss before moving forward with an implementation.

First, a user of TokenSource does not know when the token they get will expire. With the current implementation of ghinstallation, the token will be valid for at least a minute and at most an hour (the GitHub limit.) This isn't an issue if you call Token(ctx) immediately before making a request, but could lead to problems when passing the token to another library or system, as in the example. I can imagine cloning a large repository or doing some processing before trying to push exceeding the minute expiration and leading to random failures that are hard to debug.

While the code James suggested makes an additional request, it gives the user control over the the expiration time. In the specific go-git case, I think you can implement a custom authentication type that callsToken(ctx) each time to avoid this, but I'm worried this is setting a trap (the obvious API has special conditions you must be aware of) for users in the general case.

Second, while easy to fix as far as API breaking changes go, the proposal is still a breaking change to a core API of this library that will require fixes in almost all consumers. I'd like to think about ways we can achieve this with fewer or no breaks in the API.

Adding a NewTokenSource method to ClientCreator is the first thing I thought of. In a naive implementation, this would not share a token with the installation clients, but would still give you caching between uses of aTokenSource. The extra HTTP requests would be amortized across many uses, which might be sufficient.

If we do want to minimize extra requests, the problem with a new method is that CachingClientCreator cannot implement this sharing as a simple decorator of another ClientCreator implementation. My suggestion in this case is that we deprecate CachingClientCreator and add a new ClientOption (maybe WithInstanceCaching) that enables http.Client caching in DefaultClientCreator. I think the real benefit of caching is reuse of the http.Client objects (with the transport, middleware, response cache, etc.) rather than the actual github.Client objects, which I believe are cheap to create. With a cache in DefaultClientCreator, NewInstallationClient(...) and NewTokenSource(...) could easily share the same client/transport/token.

Does my concern about token expiration seem valid to you? Do you have other ideas for how to implement this without breaking the API?

[hermanbanken]

@bluekeyes those concern seem very reasonable and would still allow us to use the token. Large/longrunning clones would indeed fail in my proposal. For me that wasn’t a problem, but it makes sense to add this correctly.

I probably won’t have the time to work on this next week, but feel free to update the branch/create a new one. Otherwise I might work on it later than next week.