feat: add clientID support using options

[jamestelfer]

Very early prototype as spitballing possible API changes.

Currently not liking the amount of deprecation left behind, but it may be unavoidable. WithSigner is not supported: WithSignerContext from #119 would be the preference

Update: the approach is now far more compatible with the existing implementation, and has less deprecation.

[wlynch]

Thanks for tackling this!

Initial feedback is this looks good - I'd probably back off labeling AppID as legacy at least for now based on this part of the blog post.

The application ID is not being deprecated at this time, nor are their plans to remove it.

The least invasive option would likely be to move the error components into their own funcs that can be called out of band of the options. e.g.

signer, err := ghinstallation.NewSignerFromPath(...)

at, err :=  NewAppsTransportWithAllOptions(tr, WithClientID(appID), WithSigner(signer))

@bradleyfalzon The moving towards a model where every configurable setting is passed in as an option might be a compelling reason to cut a v3 - this would help us consolidate the number of ways to construct a client and allow us to hide more of the fields within the transport (i.e. BaseURL, Client) so that we can make changes to the AppsTransport in a backwards compatible way. 🤔

[jamestelfer]

The least invasive option would likely be to move the error components into their own funcs

This is true. I don't like having options that can produce an error either.

Another option (haha) is to use a separate config struct that includes a file path, byte array and private key struct, allowing them to be interpreted in the constructor after the options have been applied. If the config struct field for providing a key was a function, we might meet a middle ground for the API between the two current proposals.

Something like:

type AppTransportConfig struct {
    keyProvider func() (*rsa.PrivateKey, error)
    signer SignerWithContext
    // etc
}

type AppsTransportOption func(*AppsTransportConfig)

func WithPrivateKeyFile(string fileName) AppsTransportOption {
    return func (config *AppsTransportConfig) {
        config.keyProvider = fileKeyProvider(filename)
    }
}

func fileKeyProvider(string fileName) {
    return func() (*rsa.PrivateKey, error) {
        // load the file
    }
}

This is a code sketch: I'll give this a real go and see if it makes sense. Otherwise, the separate function would definitely be better than an Option func that returns an error.

[jamestelfer]

compelling reason to cut a v3

I've been wondering about this: not only seeking to keep more internal details internal, but also moving towards a TokenProvider style model, where the HttpTransport is used to call one of the TokenProviders.

This would make it somewhat easier to supply wrapping implementations that support remote KMS signers with JWT caching (to reduce the number of signing calls). I would say that this is more of a secondary use case for this library, but the API shape should remain fairly simple, while separating the token concern away from the HttpTransport itself.

[jamestelfer]

back off labeling AppID as legacy at least for now

Fair enough 😄 will do

[jamestelfer]

Out of the feedback (thank you!) I've done some remodelling to try and hash out something less invasive.

By introducing a signerFunc field that is used at construction time to create the Signer instance, the error is again emitted in the constructor function instead of in the option. This keeps the API fluent (no separate loader function and error handling required).

I prototyped the thought of a separate AppTransportConfig struct, but this felt way too big a lift for the benefit. I don't think construction-time fields are a great plan in general, but weighed up against the other ways of doing it I think the tradeoff is worth it. We don't push the complexity to the API consumer, it's kept in the implementation details, and it allows for the existing API surface to be strongly compatible.

@wlynch would you mind taking another look when you have a chance? If this is the rough shape of something that will be acceptable, I'll polish it further and add the necessary tests. So far I've been looking to get the API right more than concentrating on test coverage.

[jamestelfer]

I'm aware this may raise some eyebrows. While they're hidden in the hairline, let me explain 😀

This is roughly equivalent to

type SignerFunc[T any] struct {
	F func(T) (Signer, error)
	T T
}

func (sf SignerFunc[T]) Create() (Signer, error) {
	return sf.F(sf.T)
}

Without this, each of the createSigner* methods would have to return a func() (Signer, error) instead, and calling from createSignerFromFile to createSignerFromBytes would need to be written as createSignerFromBytes(privateKey)().

All this said, my goal here is to write something others are happy to maintain, so I'm very open to changing this.

[jamestelfer]

@wlynch / @bradleyfalzon if this approach meets with your agreement, I'll push forward and make sure it is properly tested, allowing it to be ready for proper review.

Appreciate your input so I know whether to keep going!

[jamestelfer]

Hey @wlynch / @bradleyfalzon as with #117, following to see if there's any steer from you as maintainers on whether this change fits with what you'd like to happen. I realise it's a larger API surface being changed: happy to hear that you would like something different.

Feel free to close this if it isn't what you're looking for, or indicate ways that you'd like me to proceed.

As it stands, it's more of a change proposal: if this looks right I'll solidify and test the behaviour thoroughly so it's properly ready for review and merge.