quarkiverse / quarkus-github-app

Develop your GitHub Apps in Java with Quarkus.
https://docs.quarkiverse.io/quarkus-github-app/dev/index.html
Apache License 2.0
60 stars 27 forks source link

Allow for configuration of the composed Hub4j GitHub instance #628

Closed ryandens closed 1 week ago

ryandens commented 2 weeks ago

Thank you for all the work in maintaining this excellent Quarkus extension! I'd like to use it more, but am currently having a hard time customizing the Hub4j GitHub API that is managed by this extension. Right now, my use case is primarily the following

However, I could envision needing to customize the GitHubclient further with any of the other APIs or experimenting with other GitHubConnectors.

I know that for some of these other options, like instance endpoint, you provide an option for a configuration directly via this extension. Are you open to other configuration options for all of the various ways to configure a Hub4j GitHub instance? Would you rather allow the user to optionally have a CDI Provider for the GitHub instance that this extension optionally consumes? or would you support a seam for the consumer of this extension to configure the GitHubBuilder after the defaults have been set by this extension but before the GitHubBuilder.build() method is called? Let me know what you think makes sense; I would be happy to contribute this change or provide feedback on possible implementations.

gsmet commented 1 week ago

Thanks for the kind words :).

I think it would have value for sure.

Maybe we could support a GitHubCustomizer bean in the same way we do for ObjectMapperCustomizer in core Quarkus.

The GitHubCustomizer would have a customize(GitHubBuilder) method.

What I'm not entirely sure of is if we would want the same for the installation client and the application client.

Maybe we should have something like that:

Note that we could also start by only implementing the first one and see if people have further needs.

If that works for you and you're willing to implement it, you're very welcome!

gsmet commented 1 week ago

Also, maybe a good idea to apply the hardcoded config last as you could easily shoot yourself in the foot if you override them. We might have to separate the ones we apply first from the ones we apply last at some point, but with the current values we set, I think we should set all the hardcoded/configured ones last.

ryandens commented 1 week ago

The GitHubCustomizer approach makes sense to me, I like the flexibility this gives us!

Also, maybe a good idea to apply the hardcoded config last as you could easily shoot yourself in the foot if you override them.

If you're open to it, I'd ideally like to implement this the other way around (allowing people to potentially shoot themselves in the foot for the purpose of customizability). We've previously had success running a GitHub app with HTTP/2 and the OkHttpConnector rather than the prescribed defaults of HTTP 1.1. In general, having the flexibility to experiment and override the defaults would be helpful for understanding GitHub behavior as it evolves.

gsmet commented 1 week ago

Fine by me! We can start with that and adjust if people complain.

The ones I wanted to enforce were:

                .withAppInstallationToken(installationToken.getToken())
                .withEndpoint(checkedConfigProvider.restApiEndpoint())

Because you shouldn't have any need to customize this.

As for the rest, I agree you should be able to override them.

ryandens commented 1 week ago

Agreed!